Possible deadlock in `_emscripten_dlsync_threads()` since #26659

[kleisauke]

I may have found a possible deadlock introduced by PR #26659 (v5.0.7) that affects browser builds with pthread + dlopen. So far I've only been able to reproduce it in the browser, the same test passes under Node.js.

Reproducer:

Run:

./test/runner browser.test_pthread_dlopen

on commit kleisauke@6025c46.

Stacktrace

Script terminated by timeout at:
_emscripten_get_now@http://localhost:8888/test.js:6232:35
test.wasm.futex_wait_main_browser_thread@http://localhost:8888/test.wasm:wasm-function[122]:0x2f2a
test.wasm._do_futex_wait@http://localhost:8888/test.wasm:wasm-function[121]:0x2da0
test.wasm.emscripten_futex_wait@http://localhost:8888/test.wasm:wasm-function[120]:0x2d3d
test.wasm.__timedwait_cp@http://localhost:8888/test.wasm:wasm-function[183]:0x4731
test.wasm.__pthread_cond_timedwait@http://localhost:8888/test.wasm:wasm-function[185]:0x4942
test.wasm.pthread_cond_wait@http://localhost:8888/test.wasm:wasm-function[197]:0x4d7c
test.wasm.emscripten_proxy_sync_with_ctx@http://localhost:8888/test.wasm:wasm-function[145]:0x38fa
test.wasm.emscripten_proxy_sync@http://localhost:8888/test.wasm:wasm-function[152]:0x3c51
test.wasm._emscripten_proxy_dlsync@http://localhost:8888/test.wasm:wasm-function[69]:0x19f5
createExportWrapper/<@http://localhost:8888/test.js:1014:12
__emscripten_dlsync_threads@http://localhost:8888/test.js:6067:36
test.wasm.load_library_done@http://localhost:8888/test.wasm:wasm-function[79]:0x1e31
test.wasm._dlopen@http://localhost:8888/test.wasm:wasm-function[74]:0x1c64
test.wasm.dlopen@http://localhost:8888/test.wasm:wasm-function[73]:0x1bd0
test.wasm.__original_main@http://localhost:8888/test.wasm:wasm-function[42]:0xdaf
test.wasm.main@http://localhost:8888/test.wasm:wasm-function[44]:0x11a6
callMain@http://localhost:8888/test.js:7792:28
doRun@http://localhost:8888/test.js:7852:32
run/<@http://localhost:8888/test.js:7861:7
setTimeout handler*run@http://localhost:8888/test.js:7859:15
removeRunDependency@http://localhost:8888/test.js:1318:11
initMainThread/<@http://localhost:8888/test.js:1542:30
async*callRuntimeCallbacks@http://localhost:8888/test.js:1280:26
preRun@http://localhost:8888/test.js:905:23
run@http://localhost:8888/test.js:7827:3
removeRunDependency@http://localhost:8888/test.js:1318:11
loadDylibs@http://localhost:8888/test.js:5480:26
async*receiveInstance@http://localhost:8888/test.js:1133:5
receiveInstantiationResult@http://localhost:8888/test.js:1152:12
createWasm@http://localhost:8888/test.js:1186:43
async*@http://localhost:8888/test.js:7878:1

Additional context:

I suspect there's a missing _emscripten_thread_notify() call somewhere in either system/lib/pthread/proxying.c or system/lib/libc/dynlink.c.

[sbc100]

I wonder if this is related to the limitations of the main browser thread compared to node?

i.e. if you modified emscripten_futex_wait.c so that the main node thread used futex_wait_main_browser_thread can you repro under node?

[kleisauke]

I cannot reproduce on Node.js when short-circuiting to futex_wait_main_browser_thread() with:

--- a/system/lib/pthread/emscripten_futex_wait.c
+++ b/system/lib/pthread/emscripten_futex_wait.c
@@ -159,7 +159,7 @@ static int _do_futex_wait(volatile void *addr, uint32_t val, double max_wait_ms)
 
   // For the main browser thread and audio worklets we can't use
   // __builtin_wasm_memory_atomic_wait32 so we have busy wait instead.
-  if (!_emscripten_thread_supports_atomics_wait()) {
+  if (1) {
     return futex_wait_main_browser_thread(addr, val, max_wait_ms, cancelable);
   }
 

or:

--- a/src/lib/libpthread.js
+++ b/src/lib/libpthread.js
@@ -718,14 +718,7 @@ var LibraryPThread = {
   },
 
   _emscripten_init_main_thread_js: (tb) => {
-    var can_block = !ENVIRONMENT_IS_WEB;
-#if ENVIRONMENT_MAY_BE_WEB
-    // Feature detect whether the main thread can block.
-    try {
-      Atomics.wait(HEAP32, 0, 0, 0)
-      can_block = true;
-    } catch (e) {}
-#endif
+    var can_block = false;
     // Pass the thread address to the native code where they are stored in wasm
     // globals which act as a form of TLS. Global constructors trying
     // to access this value will read the wrong value, but that is UB anyway.

[kleisauke]

Commit kleisauke@ac0a406 appears to fix this, although I'm not sure why. It was found through trial and error.

[kleisauke]

Ah, I was able to reproduce this on Node.js by removing the -sPROXY_TO_PTHREAD setting from that test:
HEAD...kleisauke:issue-26913
(I replaced it with -sPTHREAD_POOL_SIZE=8 to keep it aligned with the browser test)

Reproducer steps:

$ git clone -b issue-26913 https://github.com/kleisauke/emscripten.git issue-26913
$ cd issue-26913/
$ ./bootstrap
$ export EM_CONFIG=$EMSDK/.emscripten
$ ./test/runner core2.test_pthread_dlopen
Running 1 tests
[1/1] test_pthread_dlopen (test_core.core2.test_pthread_dlopen) ... ^Crunner:WARNING: KeyboardInterrupt

And cherry-picking commit kleisauke@ac0a406 fixes the issue:

$ git cherry-pick ac0a406c2d97fa6a5dd2a18b75e3e124a633ddc8
$ ./emcc --clear-cache
$ ./test/runner core2.test_pthread_dlopen
Running 1 tests
[1/1] test_pthread_dlopen (test_core.core2.test_pthread_dlopen) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.776s

OK

[kleisauke]

Downstream issue at kleisauke/wasm-vips#116.