raidmulator: incomplete Promise check about cactbot HOT 6 CLOSED

If you check the call stack, in all cases where it errors the top-level of the call stack will be in dexie's get function. Any function running under this scope will have Promise overridden with the DexiePromise object.

From the dexie Promise documentation:

window.Promise is always safe to use within transactions, as Dexie will patch the global Promise within the transaction zone, but leave it untouched outside the zone.

Basically, this code is calling the promise resolver while still within the transaction zone:

from cactbot.

This is happening because the current zone when importing encounters from the indexedDB is within the dexie scope, so dexie's Promise is overriding the built-in Promise.

I believe the correct resolution for this would be to escape that zone after fetching the encounter from Persistor, in the two cases where it's relevant in raidemulator.ts (lines 222 and 233). I'm not sure exactly how to go about this other than wrapping the if (enc) block in a setTimeout or something, though, and that seems like a pretty ugly solution.

from cactbot.

Oh, I see. Dexie overwrites the Promise global object (as well as the Promise constructor even on the previous object???), and so Promise.resolve is really DexiePromise.resolve and it wraps a native promise in a DexiePromise. Normally Promise.resolve returns the original object if it's instanceof Promise. DexiePromise.resolve does the same but only for DexiePromise objects. Triggers with delays get an extra constructed delay promise that wraps everything else and so skip this issue. I ran a normal log through this and see plenty of StartsUsing trigger errors as well, so there isn't anything special there.

If I breakpoint the Promise.resolve(promise) line in popup-text.js, it seems to be fine while going through different character perspectives (I see a lot of Loading id souma_test in the console) and then at some point the Promise global object gets overwritten. It's unclear to me when this happens as dexie.mjs seems to run that code immediately. It seems like really bad form to overwrite the global Promise object but it looks like even current versions of Dexie still use this due to IndexedDB promise incompatibility.

One nice thing about checking for Promise exactly is that we know that both it thennable but also that it is asynchronous. It would be nice to have stronger guarantees about which functions created asynchronicity vs were always synchronous when looking at a cactbot trigger. Unfortunately, there's no way to guarantee that a function is asynchronous without running it and checking that it returns a promise, but promises themselves are guaranteed to be asynchronous.

I guess I see a couple of options:

• explicitly check for instanceof DexiePromise as well (like the commit mentioned in the description)
• just check that the value of a trigger promise field has a then function and live with (unlikely but possibly) synchronous trigger promise fields?

(cc @valarnin)

from cactbot.

  public async loadEncounter(id: number): Promise<Encounter | undefined> {
    return new Promise<Encounter | undefined>((res) => {
      let encounter: Encounter | undefined = undefined;
      void this.transaction('readwrite', [this.encounters, this.encounterSummaries], async () => {
        encounter = await this.encounters.get(id);
      }).then(() => res(encounter));
    });
  }

What about this? Based on https://dexie.org/docs/Dexie/Dexie.transaction(), it seems like the callback to transaction is in the zone, but if you defer until after the transaction is done then it will no longer be in the zone.

from cactbot.

Maybe this commented version will help with understanding?

  public async loadEncounter(id: number): Promise<Encounter | undefined> {
    return new Promise<Encounter | undefined>((res) => {
      // Start a new transaction with the `encounters` and `encounterSummaries` tables
      void this.transaction('readwrite', [this.encounters, this.encounterSummaries],
        // Running from within that transaction, so now `window.Promise` has been overwritten with `DexiePromise`
        async () => {
          // Call `resolve` handler with the result of the `get` for the encounter
          // Any code directly resulting from this call is executed within the transaction's scope/zone
          res(await this.encounters.get(id));
        }
      );
    });
  }

I was thinking something even more simplified as such:

  public async loadEncounter(id: number): Promise<Encounter | undefined> {
    let enc: Encounter | undefined;
    await this.transaction('readwrite', [this.encounters, this.encounterSummaries], async () => {
      enc = await this.encounters.get(id);
    });
    return enc;
  }

But I have no time right now to actually test any of these changes.

from cactbot.

I think we're on the same page. That seems like it works. I'll put up a PR.

from cactbot.