Show warning if export is not in ASYNCIFY_EXPORTS even for old Asyncify

[RReverser]

New Asyncify (JSPI) mode will require exports to be explicitly marked with ASYNCIFY_EXPORTS.

While not required by old Asyncify, I believe it would make for an easier upgrade path if Emscripten would already start showing warnings in the console when an exported method is using Asyncify but is not in ASYNCIFY_EXPORTS list.

This would show up only in ASSERTIONS mode so wouldn't affect production in any way. cc @brendandahl

[sbc100]

Sounds good if its possible.

Is it easy to detect "an exported method is using Asyncify ".. what does that mean exactly?

[RReverser]

Is it easy to detect "an exported method is using Asyncify ".. what does that mean exactly?

I mean purely at runtime. We already have codepaths for old Asyncify that check if, upon an exit from an exported method, we have a sleeping Asyncify state and, if so, it wraps the export into a Promise.

Basically it means just adding an #if ASSERTIONS-wrapped check into the same codepaths to verify that the method is in ASYNCIFY_EXPORTS and show a console warning otherwise.

[RReverser]

E.g. here is the codepath for Embind:

emscripten/src/embind/embind.js

Lines 861 to 863 in a0a3f24

	if (Asyncify.currData) {
	return Asyncify.whenDone().then(onDone);
	}

There are couple more places that will need updating, but should be a pretty straightforward change, just needs someone to do it.

[brendandahl]

I think we should probably first decide if we want asyncify and JSPI to behave the same or if we're fine with them being different. Our current thinking has been let's not invest more in asyncify. A warning does seem like a low effort path to help people migrate though.

[RReverser]

I think we should probably first decide if we want asyncify and JSPI to behave the same or if we're fine with them being different.

A warning does seem like a low effort path to help people migrate though.

Yeah, it's more about migration path, sort of like -fexceptions are "legacy" and -fwasm-exceptions are "modern way", but they were kept interchangeable to avoid breaking code that might need to compile one output for modern browsers, and another output as a fallback for older ones.

I see Asyncify vs JSPI in the same boat, especially since JSPI is currently very cutting edge and not usable in prod for most apps, so I'd like to be able (myself and others too) to use them interchangeably and compile for one or another depending on the target browsers they need to support. For that the public API differences ideally would be minimal.

Specifically for exports check that boat has sailed for Asyncify and can't be changed w/o breaking backward compat, but at least a warning would already help people upgrade the code to be forward-compatible with both modes.

[brendandahl]

One other thing note, I haven't seen much feedback yet on whether users prefer JSPI's explicit async exports or asyncify's automatic conversion.

[RReverser]

I thought it was hard requirement of JSPI?

[brendandahl]

It's a hard requirement that a JSPI'd export will return a promise, but it's not a hard requirement that we have to explicitly tell emscripten what exports suspend.

We could go asyncify's route, and automatically determine what exports suspend based on what async imports are called (indirect function calls also cause auto conversion). The main reason to do the explicit list, is that it's somewhat strange that depending on your imports your exports can suddenly change and return promises. I think with asyncify it was more common that the user would just run a main program so they wouldn't really need to worry about what parts of the api would be async.

[RReverser]

Ah I see.

I think with asyncify it was more common that the user would just run a main program so they wouldn't really need to worry about what parts of the api would be async.

When I was adding Asyncify to Embind, that wasn't necessarily true and it could've used explicit policy too I suppose, but I thought that might be a bit disruptive.

The main reason to do the explicit list, is that it's somewhat strange that depending on your imports your exports can suddenly change and return promises.

I mean, that can be a problem with any of the approaches - a rogue conditional emscripten_sleep somewhere deep in dependency tree or someone overriding one of the syscalls to provide their async implementation in a JS library.

The only difference is that with hard requirement to be in the exports list any such dynamically async action somewhere in deps becomes a spurious runtime error, whereas with auto-detection it becomes a spurious async result. IMO the latter is easier to handle - if you don't care about microticks, you can always just do await someExport() and engine will do the right thing for you regardless of whether it's a thenable or a synchronously available result.

If you do care about microticks though, it's a useful optimisation, similar to what Atomics.waitAsync does.

For example, we recently had to override fd_read in one of the projects to become async for TTY (so that it could wait for user keystrokes and not block the browser from processing key events) and keep the fast sync path for non-TTY file descriptors. I know JSPI didn't go that way in initial discussions at least, but making all imports strictly async or not makes such optimisations impossible.

And in combination with all exports having to declare themselves as async means that someone like us changing fd_read to become async, changes all exports that happen to read files async as well - even though in most cases they don't have to be asynchronous as they don't care about TTY and never hit that codepath.

FWIW I totally see the appeal of statically known types instead of having T | Promise<T> for each export when using Asyncify/JSPI, just describing the sort of scenarios that are useful with dynamic approach.

[brendandahl]

I mean, that can be a problem with any of the approaches - a rogue conditional emscripten_sleep somewhere deep in dependency tree or someone overriding one of the syscalls to provide their async implementation in a JS library.

Yeah, I don't think we can avoid the viral nature of async with any approach.

The only difference is that with hard requirement to be in the exports list any such dynamically async action somewhere in deps becomes a spurious runtime error, whereas with auto-detection it becomes a spurious async result. IMO the latter is easier to handle - if you don't care about microticks, you can always just do await someExport() and engine will do the right thing for you regardless of whether it's a thenable or a synchronously available result.

I've run into a few cases where debugging the spurious async results has been rather painful. It's been when code assumes a sync value and then the promise gets passed back into wasm and auto stringified and converted to a number and silently goes on. At least with the spurious runtime error I always know what triggered it.

If you do care about microticks though, it's a useful optimisation, similar to what Atomics.waitAsync does.

A potential solution there is you can export a second version of your function that doesn't suspend. We've also talked about auto exporting async and sync versions of every JSPI function.

Thanks for the example about fd_read. I haven't heard of anyone actually taking advantage of the sync path optimization yet and using it to avoid rewriting all code to async.

[RReverser]

Hm doubling exports is an interesting alternative I guess, although a bit unfortunate.

For now all I want is some sort of consistency between the two versions/features, but if it's too early to call either approach to exports as the better one, then at least having a common denominator would already be better than current state.

Right now Emscripten has assertions that outright prohibit using e.g async() in Embind exports if you're using Asyncify and not JSPI.

That's not ideal as it makes it outright impossible to write code that's compatible with both, especially since it's a link-time feature so not even some #ifdef workaround would help.

If it's too early to show a warning like in the initial description of the issue, then at the very least, the new explicit markers should be just ignored for regular Asyncify and not result in an error.

[RReverser]

It's been when code assumes a sync value and then the promise gets passed back into wasm and auto stringified and converted to a number and silently goes on.

FWIW I agree this does sound like a nasty issue to debug. Although I don't understand - when you say the value was passed to Wasm, does that mean it's import that was conditionally async?

I don't think that's currently possible as imports are always explicitly async or not.

I thought we were talking only about exports returning values to JS from Wasm.

[RReverser]

If it's too early to show a warning like in the initial description of the issue, then at the very least, the new explicit markers should be just ignored for regular Asyncify and not result in an error.

@brendandahl Thoughts on this approach for now?

[brendandahl]

Adding warnings when assertions are enabled sounds good.

[RReverser]

Adding warnings when assertions are enabled sounds good.

I meant also allowing no-op async() annotations in regular Asyncify.

[brendandahl]

Sorry I misread. The assertion is there because async embind bindings are broken under asyncify=1. I think it's totally fixable, but we haven't put any effort into looking into the bug since we're focusing more on JSPI.

[brendandahl]

I'm guessing it's just the binding is missing a handleAsync or handleSleep wrapper.

[RReverser]

Huh, that's odd. I think they still worked on an older project recently, but maybe I need to recheck again that it's using latest version...

[brendandahl]

I think some of it was working when not using the async annotation (which is relatively new). IIRC I tried running test_embind_jspi with ASYNCIFY=1 without the assert and it wasn't working.

[RReverser]

I think some of it was working when not using the async annotation (which is relatively new).

Ah right, I wasn't using it yet due to assertion. So you're saying it's only if I add async() & remove that assertion, then it breaks?

[brendandahl]

test_embind_jspi breaks with asyncify=1 and no assertion. There are other tests that don't use async annotation but use embind + asynciyf=1 that seem to work fine e.g. test_embind_asyncify.

[adamziel]

I'm exploring migrating WordPress Playground to JSPI. Unfortunately I just lost half a day chasing an "invalid suspender object for suspend" error that was resolved by listing my exports in ASYNCIFY_EXPORTS.

An informative warning would be excellent, and otherwise it would be lovely to at least document that option exists either of these resources:

• https://emscripten.org/docs/porting/asyncify.html
• https://v8.dev/blog/jspi
• https://v8.dev/blog/jspi-ot
• https://emscripten.org/docs/porting/asyncify.html

I only realized it's there because I started reading instrumentWasmExports, noticed a familiar exportPattern variable, and just thought "well, there's ASYNCIFY_IMPORTS, maybe there's an undocumented ASYNCIFY_EXPORTS too".

Related to WordPress/wordpress-playground#134