Async wrappers can throw

[dev7355608]

class Foo {
    async bar() {
        return 0;
    }
}

libWrapper.register("some-module", "Foo.prototype.bar", async function (wrapped) {
    await new Promise(resolve => setTimeout(resolve, 1000));
    const a = await wrapped();
    return a + 1;
});

const foo = new Foo();

await foo.bar();

Uncaught libWrapper: This wrapper function is no longer valid, and must not be called. This is most likely caused by an issue in the 'Foo.prototype.bar' wrapper registered by module 'some-module'.

It's probably necessary to add a new suffix to support asynchronous functions, because we can't know whether wrapped is called asynchronously or the function simply returns a promise but wrapped is called synchronously.

libWrapper.register("some-module", "Foo.prototype.bar#async", ...)

[dev7355608]

Or we assume that, if the function returns a promise, wrapped is called asynchronously (i.e. called in that promise) as well, which is probably almost always the case. And we add the suffix #sync instead to indicate the opposite behavior. This way we would be backwards compatible, i.e., probably nobody has to change their code to get the correct behavior.

[ruipin]

Thanks for the report. You are right, this is actually related to the limitations that used to exist (lifted in 1.0.7) regarding multiple chaining.

I did not think of the situation where people would await for something other than a single call to the wrapped function, so never planned for this.

The problem is that as soon as you return from the wrapper, the wrapped method gets invalidated. This is to prevent breakage if e.g. the wrapper order changes, or other libwrapper internals change unexpectedly. As such, once the promise resolves, calling wrapped throws.

I think it might be possible to lift the restrictions regarding calling wrapper functions asynchronously. There is already a mechanism to detect when a wrapped needs to become invalid because of e.g. a wrapper order change, I could make it so that only those invalidate the wrapper.

However, fully preventing this kind of invalidation is difficult. I can easily foresee an interaction between two modules, where one awaits for some event, and meanwhile the second registers a new wrapper to the same method, causing the previous wrapper to throw once it resolves.

I have some ideas about how to further improve this, but I don't see an easy way. I'll do the above for now while making sure to document this behaviour, add a test case, and then will revisit it later.

[dev7355608]

This doesn't work as well in 1.0.7.0, I noticed.

libWrapper.register("midi-qol", "Foo.prototype.bar", async function (wrapped) {
    const a = await wrapped();
    const b = await wrapped();
    return a + b + 1;
});

Is it guaranteed that the first awaited function is called before the function returns? That's why it works if the first await is wrapped?

[ruipin]

Yeah, ends up something more or less equivalent to

return wrapped().then(
    /* rest of code here */
);

The first method is called immediately, the expectation being that it returns a promise.

Only the code after the await will be done asynchronously.

You could get both promises first, and then resolve them asynchronously later, e.g.:

const p1 = wrapped();
const p2 = wrapped();
const a = await p1;
const b = await p2;
return a + b + 1;

This avoids ever calling the wrapped method asynchronously.

[ruipin]

With v1.0.8.0, I've removed the hardcoded limitation preventing asynchronous chaining of wrappers, i.e. the automatic invalidation on the return. Now, they will only be invalidated if the internal data structures are changed.

I've also updated the documentation to discuss this use case and the limitations here.

I'll keep this issue open so that I can come back and revisit improving this remaining limitation once I have more time.

[dev7355608]

const p1 = wrapped();
const p2 = wrapped();
const a = await p1;
const b = await p2;

This would solve my problem actually if I could guarantee the order of p1 and p2 (p1 needs to be executed before p2). But that's not possible, I think.

[ruipin]

You can sort of guarantee the order of execution by the order of await. The Promises will only be resolved once you await them, and the JS engine should prioritize the one awaited on, thus in the above example p1 will resolve before p2.

The issue is if the side-effects before the Promise was constructed break the second await - for example, a common use for promises is waiting for dialog results, and said dialog would open as soon as you call the method. The above solution would cause two dialogs to open simultaneously.

[dev7355608]

A promise can be executed before await. p2 might be executed before p1 even if we await p1 and then p2. This example prints hello! without await:

async function say(msg) {
    console.log(msg);
}
say("hello!");

[ruipin]

That method isn't executing a Promise though, or to be more exact, the Promise is auto-resolved. It is not a good example of your point.

It is important to remember async/await is just synctactic sugar on top of Promise. That function is more or less equivalent to:

function say(msg) {
    console.log(msg);
    return Promise.resolve();
}

What you're seeing is that that method auto-resolves its promise synchronously and immediately.

Notice how it returned a promise (also using your code, which gives the exact same result):

let x = say("bla");
// 'bla'
// -> Promise { <state>: "fulfilled", <value>: undefined }

A better example would be a function that doesn't auto-resolve, e.g.:

function say(msg) {
    console.log(msg);
    return new Promise(resolve => resolve());
}

let x = say("bla");
// 'bla'
// -> Promise { <state>: "pending" }

await x;
// -> Promise { <state>: "fulfilled", <value>: undefined }

The promise is not resolved until my await, even though the console.log showed up.

What we're seeing is that the side-effects of the method (i.e. code before returning a Promise, in this case the console.log) execute immediately, synchronously. Everything inside the Promise (or the first await) still executes asynchronously.

This is what I was saying - as long as the synchronous side-effects are fine (in this case a console.log), then the workaround would work. If the synchronous side-effects aren't desirable (e.g. they open a user-visible dialog window, or kicks off something else in the background), then there's no way to avoid the asynchronous wrapper chaining, unfortunately.

This only applies as long as you're running synchronous code, since the engine will not resolve promises while running fully synchronous code. Awaiting explicitly on a Promise in the main thread should cause the JS engine to prioritize this promise first. It will only allow other Promises to resolve if that Promise still can't. However, this workaround will also not work if the Promise has further asynchronous dependencies - i.e. if it will not be able to resolve as soon as the main thread yields, as then other Promises will be given a chance to resolve.

I agree with you in that the workaround will not work in the large majority of cases - only in cases where the calls are essentially fully independent, and order of effects does not matter.

[dev7355608]

I see. I'm not too familiar with async/await.

The code in Midi QOL that causes this problem can probably removed anyway. But it's not the same problem that's causing all these Midi issues lately. I messed something else up when I was adding libWrapper compatibility. I uncovered this issue after I fixed the other problems.

Thanks for your quick response and effort to fix this problem!

[ruipin]

v1.1.0.0 (specifically commit affdc96) adds a bunch of async tests to the test suite and fixes an issue related to async WRAPPER-type wrappers.

The mechanism to detect failure to chain the wrapper further would fire before the Promise was allowed to run, therefore unregister wrappers incorrectly. This is now fixed.

There is also some cleanup of the relevant code, in preparation for possible future changes. Regressions should be easier to avoid now that the test suite is testing a lot more async code, too.

[ruipin]

v1.3.0.0 (among other things) refactors the test suite such that separate tests are not required to test the async code path. Now, every test will run twice - once synchronously, and a second time asynchronous.

This should make it much easier to touch this section of code in the future, and ensures the async code path is as well tested as the sync code path without duplicating work.

This version also prepares a lot of the ground work that will be necessary to fix this issue.

[ruipin]

Fixed in v1.3.1.0.

Starting with this version, modifying a wrapper (e.g. using libWrapper.register/unregister) will not trigger invalidation of any outstanding wrapper chains. Instead, outstanding wrapper chains will continue execution as if nothing had happened, i.e. they will not call the new wrappers or see the new wrapper chaining order.

Closing.