Fix Node.js exit for library builds with pthreads#26441
Fix Node.js exit for library builds with pthreads#26441brendandahl wants to merge 2 commits intoemscripten-core:mainfrom
Conversation
When building without a main function (a library build), and using pthreads to do background work, the main Node.js thread has no event loop work to keep Node.js alive. The workers are also unreferenced when they start executing, so Node.js will exit prematurely before the background thread can finish its work. To prevent this, change the worker reference logic so that we only `unref()` workers upon execution if `HAS_MAIN` is enabled. For library builds, we keep them referenced (`worker.ref()`) to keep the process alive while they are active. We also ensure idle workers returned to the pool are `unref()`'d across all Node builds (not just with `PROXY_TO_PTHREAD`) so that an idle pool doesn't leak references preventing exit. Fixes emscripten-core#23092
brendandahl
force-pushed
the
pthread-library
branch
from
c718af3 to
1559315
Compare
src/lib/libpthread.js
Outdated
| worker.pthread_ptr = 0; | ||
|
|
||
| #if ENVIRONMENT_MAY_BE_NODE && PROXY_TO_PTHREAD | ||
| #if ENVIRONMENT_MAY_BE_NODE |
There was a problem hiding this comment.
I this change needed? It seems maybe separate, and will likely effect code size of pthread-using programs slightly.
There was a problem hiding this comment.
The unref is needed in the library use case. I changed the condition to (PROXY_TO_PTHREAD || !HAS_MAIN) now
| // exiting. This has no effect if the worker is already weakly | ||
| // exiting. This has no effect if the worker is already weakly | ||
| // referenced. | ||
| worker.unref(); |
There was a problem hiding this comment.
Do you know why this was PROXY_TO_PTHREAD-only before? The above comment now sounds like its needed in all cases... but there must be some reason why its not always needed? I.e. in the normal case when we have are not PROXY_TO_PTHREAD and when we do have a main why don't we need to unref when we return workers to the pool?
There was a problem hiding this comment.
This helps reduce code size, since previously only the "main" thread in PROXY_TO_PTHREAD mode was strongly referenced at this point, see:
https://github.com/emscripten-core/emscripten/pull/19073/changes#r1172276555
Plus, _emscripten_thread_set_strongref() is internal API, so we can be sure all non-PROXY_TO_PTHREAD workers are weakly referenced.
|
Funnily enough I just ran into this exact issue in #26448. In |
I think the current behavior is reasonable. Node.js apps behave similarly to native programs: background threads themselves don't keep the process alive. Instead, the app remains running only while it explicitly blocks on I could reinstate PR #23152, so that you could write something like this: Details--- a/test/wasm_worker/lock_busyspin_waitinf_acquire.c
+++ b/test/wasm_worker/lock_busyspin_waitinf_acquire.c
@@ -6,7 +6,6 @@
// This test can be run under pthreads *or* Wasm Workers
#ifdef __EMSCRIPTEN_PTHREADS__
#include <pthread.h>
-_Atomic bool done = false;
#else
#include <emscripten/wasm_worker.h>
#endif
@@ -24,7 +23,6 @@ void worker_main() {
assert(sharedState == 1);
#ifdef __EMSCRIPTEN_PTHREADS__
emscripten_out("done");
- done = true;
exit(0);
#else
REPORT_RESULT(0);
@@ -38,12 +36,6 @@ void* pthread_main(void* arg) {
worker_main();
return NULL;
}
-
-void nothing(void* userData) {
- if (!done) {
- emscripten_set_timeout(nothing, 100, 0);
- }
-}
#else
char stack[1024];
#endif
@@ -64,10 +56,9 @@ int main() {
// Spawn a Pthread to try to take the lock. It will succeed only after
// releaseLock() gets called.
pthread_create(&t, NULL, pthread_main, NULL);
- // Add an infinite timeout to make sure the node runtime stays alive
- // after main returns.
- // See https://github.com/emscripten-core/emscripten/issues/23092
- emscripten_set_timeout(nothing, 100, 0);
+ // Mark the thread as strongly referenced, so that Node.js doesn't exit
+ // while the pthread is running.
+ emscripten_thread_set_strongref(t);
#else
// Spawn a Worker to try to take the lock. It will succeed only after releaseLock()
// gets called.However, I think the conclusion at the time was that this approach is very Node-specific. |
3 participants
When building without a main function (a library build), and using pthreads to do background work, the main Node.js thread has no event loop work to keep Node.js alive. The workers are also unreferenced when they start executing, so Node.js will exit prematurely before the background thread can finish its work.
To prevent this, change the worker reference logic so that we only
unref()workers upon execution ifHAS_MAINis enabled. For library builds, we keep them referenced (worker.ref()) to keep the process alive while they are active.We also ensure idle workers returned to the pool are
unref()'d across all Node builds (not just withPROXY_TO_PTHREAD) so that an idle pool doesn't leak references preventing exit.Fixes #23092