Fix async futex wait in WASM worker dose not keep runtime alive

[medns]

Fix asynchronous futex wait emscripten_atomic_wait_async in WASM worker dose not keep runtime alive.

[kripken]

I believe this is not needed. The compiler adds the runtime-keepalive stuff automatically.

[medns]

Unfortunately, after testing, the compiler does not automatically add runtime-keepalive stuff, because library_wasm_worker.js is not in the core system libraries of modules.js, so need to add a reference here manually.

emscripten/src/modules.js

Line 37 in f9ce05b

// Core system libraries (always linked against)

[kripken]

Hmm, it's not in the core system libraries always linked against, but when you enable wasm workers then it is added to the list. And then it should be processed like any other library. That is, there should be no difference between a core library and a non-core library if both are linked in. Is that not so?

[medns]

Hmm, it's not in the core system libraries always linked against, but when you enable wasm workers then it is added to the list. And then it should be processed like any other library. That is, there should be no difference between a core library and a non-core library if both are linked in. Is that not so?

According to the modules.js, when enable WASM worker support, it doesn't seem to be automatically added to the core library.
As a result, dependencies must be added manually.

[kripken]

Where do you see that? (I see no mention of wasm workers in modules.js)

[sbc100]

I think @kripken is right, we hopefully don't need these __deps here.. if you remove the __deps here what error do you see?

[kripken]

Other than the comment this looks right to me.

@sbc100 what's the best way to test keepalive usage?

[sbc100]

I think this could be the right fix. I guess we don't have a lot of testing for this API. If this is a bug it should also effect pthreads too, not just wasm workers right?

[sbc100]

Oh I guess emscripten_atomic_wait_async is currently defined in wasm_worker.h and library_wasm_worker.js so maybe its a wasm worker specific API.

We do have some tests for it in test/wasm_worker/wait_async.c and test/wasm_worker/cancel_wait_async.c .. do you know why those don't need this?

@juj for wasm worker input.

[medns]

If the emscripten_atomic_wait_async method is used in a pthread thread without adding a keepalive runtime counter by runtimeKeepalivePush, __emscripten_thread_exit will be called when pthread invoke completes the entrypoint, and resources will be released, resulting the pthread resource like a pthread_self() returning 0 in the emscripten_atomic_wait_async callback.

In addition, although the thread created by wasm worker does not have this problem, but it is still incorrect for the runtime keepalive.

[juj]

Fix asynchronous futex wait emscripten_atomic_wait_async in WASM worker dose not keep runtime alive.

Wasm Workers do not start with a "thread main" function, so they don't have a concept of the runtime liveness tracking. (in other words, the runtime is always alive)

If you are seeing the runtime being shut down in a wasm worker, that would be a bug. The runtime shutdown functions should not be getting called in wasm workers.

[juj]

Oh now I think I understand - the fix here is intended to be "Fix async futex wait in pthreads does not keep runtime alive"?

LGTM.

[sbc100]

Oh now I think I understand - the fix here is intended to be "Fix async futex wait in pthreads does not keep runtime alive"?

LGTM.

@medns, if this is correct, can you replace WASM worker with pthread in the title and description?

Also, can we add a test for this? I'm guessing a new test in new test in test/pthead that runs in test_core.py would make the most sense.

[sbc100]

I think that #20404 subsumes this change, I hope thats, it kind of fell out the fact that I moved that API and had to make it testable under pthreads.

[medns]

I think that #20404 subsumes this change, I hope thats, its kind of feel out the fact that I moved that API and had to make it testable under pthreads.

#20404 does subsumes the modifications of this PR, so should I close this PR?

[sbc100]

I think so yes. Thanks for working on this!