Make Node.js pthreads behavior in line with native #19073

kleisauke · 2023-03-26T11:06:08Z

By marking workers as weakly referenced. This corresponds to the ideal scenario as mentioned in commit a9cbf47.

(originally based on commit e6f86a8 and 4b4faf9)

~~This passes the test suite locally, except for test_pthread_abort_interrupt.~~ Fixed with commit ca5f97c.

RReverser · 2023-03-26T15:46:38Z

FWIW I'm definitely in favour of this change, but, to be clear, one concern was/is that it's potentially a breaking one: someone who isn't even using PROXY_TO_PTHREAD, might rely on current Emscripten behaviour of app staying alive as long as some pthread is and using some out-of-band communication between the two.

Hopefully no one is really doing this in the wild, and there isn't really that many Node.js users of Emscripten anyway, but, unlike with previous linked iterations, there is some small risk involved.

This partially reverts commit 9cdd7b3.

system/include/emscripten/threading.h

sbc100 · 2023-03-27T20:55:41Z

test/test_core.py

@@ -2884,7 +2884,7 @@ def test_pthread_abort(self):

  @node_pthreads
  def test_pthread_abort_interrupt(self):
-    self.set_setting('PTHREAD_POOL_SIZE', 1)
+    self.set_setting('PROXY_TO_PTHREAD')


I think we would want to investigate why this is needed.

It's indeed a bit curious that this is needed. Linking with -sPTHREAD_POOL_SIZE=1 and -sEXIT_RUNTIME doesn't work either.

This particular test case is calling abort() in its pthread entry point, which causes a WebAssembly.RuntimeError to be thrown that is supposed to be propagated back to the main thread via the 'error' event. However, the worker is calling _emscripten_thread_crashed() just before throwing this exception:

emscripten/src/worker.js

Lines 278 to 281 in 574f4f4

if (Module['__emscripten_thread_crashed']) {

Module['__emscripten_thread_crashed']();

}

throw ex;

As a result, the main thread immediately exits while the runtime is kept alive:

emscripten/system/lib/pthread/emscripten_yield.c

Lines 31 to 35 in 574f4f4

if (thread_crashed) {

// Return the event loop so we can handle the message from the crashed

// thread.

emscripten_exit_with_live_runtime();

}

Since the worker is weakly referenced, the program exits too early, causing the test to fail.

A possible solution to this is to make the thread strongly referenced during pthread_join():

emscripten/system/lib/libc/musl/src/thread/pthread_join.c

Lines 21 to 23 in 42df09e

// Mark the thread as strongly referenced so that Node.js doesn't exit

// while the pthread is running.

_emscripten_thread_set_strongref(t, 1);

This ensures that the program does not exit too early when a joinable thread crashes.

Commit ca5f97c improves this.

src/library_pthread.js

sbc100 · 2023-03-27T21:06:04Z

system/lib/libc/crt1_proxy_main.c

@@ -49,5 +49,8 @@ EMSCRIPTEN_KEEPALIVE int _emscripten_proxy_main(int argc, char** argv) {
  pthread_t thread;
  int rc = pthread_create(&thread, &attr, _main_thread, NULL);
  pthread_attr_destroy(&attr);
+  if (rc == 0) {
+    emscripten_thread_set_strongref(thread, 1);
+  }


We already have a call to runtimeKeepalivePop in exitOnMainThread that is supposed to take are of bringing down the app once the proxy'd main ends.

I have an stalled change that tried to recfactor that: #18770

The corresponding push is in callMain:

#if PROXY_TO_PTHREAD // With PROXY_TO_PTHREAD make sure we keep the runtime alive until the // proxied main calls exit (see exitOnMainThread() for where Pop is called). {{{ runtimeKeepalivePush() }}} #endif

On other words aren't already forcing the runtime to stay alive as long as the proxied main is running?

On other words aren't already forcing the runtime to stay alive as long as the proxied main is running?

IIUC, the runtimeKeepalivePush() / runtimeKeepalivePop() mechanism would only keep the Emscripten runtime alive, but still allows Node.js to exit (i.e. it doesn't keep the event loop of Node.js alive). For example:

$ cat <<EOT > test.c #include <emscripten/emscripten.h> int main() { emscripten_exit_with_live_runtime(); // Should never get here __builtin_trap(); } EOT $ emcc -O3 test.c -o test.js -sEXIT_RUNTIME $ node test.js $ echo $? 0

Not in scope of this PR, but I think one should imply the other. I was pretty surprised when I ran into that behaviour for the first time and noticed that runtimeKeepAlivePush didn't keep Node from exiting.

sbc100 · 2023-03-27T21:09:20Z

src/library_pthread.js

-      worker.ref();
+      // Once we start executing a pthread, we need to mark the worker as weakly
+      // referenced, this ensures that Node.js may exit while a pthread is running.
+      worker.unref();


Shouldn't we just set them to unreferenced on first creation then? Or are you saying that an unused worker is strongly referenced and used one only weakly? That seems a odd to me.

Shouldn't we just set them to unreferenced on first creation then?

I considered making the worker weakly referenced during the 'loaded' event, i.e.:

Patch

--- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -287,11 +287,9 @@ var LibraryPThread = { } else if (cmd === 'loaded') { worker.loaded = true; #if ENVIRONMENT_MAY_BE_NODE - // Check that this worker doesn't have an associated pthread. - if (ENVIRONMENT_IS_NODE && !worker.pthread_ptr) { - // Once worker is loaded & idle, mark it as weakly referenced, - // so that mere existence of a Worker in the pool does not prevent - // Node.js from exiting the app. + if (ENVIRONMENT_IS_NODE) { + // Once the worker is loaded, mark it as weakly referenced, so + // that its existence does not prevent Node.js from exiting. worker.unref(); } #endif @@ -686,13 +684,6 @@ var LibraryPThread = { // in this file, and not from the external worker.js. msg.moduleCanvasId = threadParams.moduleCanvasId; msg.offscreenCanvases = threadParams.offscreenCanvases; -#endif -#if ENVIRONMENT_MAY_BE_NODE - if (ENVIRONMENT_IS_NODE) { - // Mark worker as weakly referenced once we start executing a pthread, - // so that its existence does not prevent Node.js from exiting. - worker.unref(); - } #endif // Ask the worker to start executing its pthread entry point function. worker.postMessage(msg, threadParams.transferList);

But this 'loaded' event could occur after pthread_create() returns, which would mean it could break such constructions:

pthread_create(&thread, NULL, worker_thread, NULL); _emscripten_thread_set_strongref(thread, 1);

(i.e. the same construction that PROXY_TO_PTHREAD currently uses).

Or are you saying that an unused worker is strongly referenced and used one only weakly?

I'm not sure if I understand this question, but idle/unused workers (i.e. workers not hosting a pthread) are already weakly referenced within the pool.

src/library.js

test/core/pthread/test_pthread_weak_ref.c

sbc100 · 2023-04-04T16:24:16Z

system/lib/libc/musl/src/thread/pthread_join.c

@@ -18,6 +18,9 @@ static int __pthread_timedjoin_np(pthread_t t, void **res, const struct timespec
 	// TODO: The detached check here is just to satisfy the
 	// `other.test_{proxy,main}_pthread_join_detach` tests.
 	if (t->detach_state != DT_DETACHED && __pthread_self() == t) return EDEADLK;
+	// Mark the thread as strongly referenced so that Node.js doesn't exit
+	// while the pthread is running.
+	_emscripten_thread_set_strongref(t, 1);


Why does one thread joining with another thread effect whether we want to keep node alive or not?

Sure if the main thread returns in brings down all threads even if they are waiting on each other?

This was a workaround for the failing test_pthread_abort_interrupt testcase. Commit ca5f97c improves this and removes the need to do this in pthread_join().

Context: #19073 (comment)

sbc100 · 2023-04-04T16:25:25Z

src/library_pthread.js

@@ -575,6 +575,15 @@ var LibraryPThread = {
    else postMessage({ 'cmd': 'cleanupThread', 'thread': thread });
  },

+  _emscripten_thread_set_strongref: function(thread, strongRef) {


Should this be marked as __proxy: sync since I don't see how it could work on anything but the main runtime thread?

Sure, marked as __proxy: 'sync' with commit d123a49. Though, we can be sure that the current call sites are only executed on the main runtime thread after commit ca5f97c.

I removed this __proxy: 'sync' annotation with commit 059448a as this function is now only called on the main runtime thread.

src/library_pthread.js

sbc100 · 2023-04-04T16:29:21Z

src/library_pthread.js

-      // Mark worker as strongly referenced once we start executing a pthread,
-      // so that Node.js doesn't exit while the pthread is running.
-      worker.ref();
+      // Mark worker as weakly referenced once we start executing a pthread,


What I don't understand about this comment "Mark worker as weakly referenced once we start executing a pthread" is that workers are already weakly referenced when they are in the pool, so why does anything need to change once we start executing a pthread. They are weakly referenced both before and after they start executing a thread, no?

IIUC, only idle/unused workers are weakly referenced within the pool (i.e. when -sPTHREAD_POOL_SIZE=x is specified).

emscripten/src/library_pthread.js

Lines 292 to 295 in 22893f5

// Once worker is loaded & idle, mark it as weakly referenced,

// so that mere existence of a Worker in the pool does not prevent

// Node.js from exiting the app.

worker.unref();

emscripten/src/library_pthread.js

Lines 216 to 219 in 22893f5

// Once a pthread has finished and the worker becomes idle, mark it

// as weakly referenced so that its existence does not prevent Node.js

// from exiting.

worker.unref();

But when -sPTHREAD_POOL_SIZE=x is omitted (which should be safe on Node.js after commit 73eaf81) or when an application spawns more threads than specified within that setting (i.e. thread pool exhaustion), workers are strongly referenced by default, so this .unref() is still necessary for those cases.

Can we instread fix the workers are strongly referenced by default thing?

IRC the intent prior this change is that they should be weakly referenced until they are running a pthread, and then strongly referenced one a pthread is running.

After this change they should never be strongly referenced so should be .unref() as soon as we create them?

I tried to simplify this with commit 059448a.

After this change they should never be strongly referenced so should be .unref() as soon as we create them?

AFAIK it's only safe to .unref() the worker during the 'loaded' event or just before executing the pthread entry point function (i.e. only after the .on('message') listeners are attached), it will not work if you .unref() too early. See for example this program:

Details

const { Worker, isMainThread, parentPort } = require('worker_threads'); if (isMainThread) { const worker = new Worker(__filename); worker.on('error', (err) => { throw err; }); worker.on('exit', () => console.log('exit')); worker.on('message', (msg) => console.log(msg)) worker.postMessage('From main thread!') /* setTimeout(() => */worker.unref()/* , 100); */ } else { parentPort.on('message', (msg) => console.log(msg)); parentPort.postMessage('From worker!'); }

As a possible enhancement, perhaps we could make PROXY_TO_PTHREAD no-op on Node.js (i.e. just call the original main)? This would be safe after commit 73eaf81, and would allow further simplification.

sbc100 · 2023-04-17T20:19:31Z

system/lib/pthread/emscripten_yield.c


 void _emscripten_thread_crashed() {
-  thread_crashed = true;
+  crashed_thread_id = pthread_self();


Can we not just call _emscripten_thread_set_strongref(pthread_self()) here and avoid the change to _emscripten_yield below?

I considered this, but then I noticed this comment:

emscripten/system/lib/pthread/emscripten_yield.c

Lines 25 to 29 in 96d3452

// When a secondary thread crashes, we need to be able to interrupt the main

// thread even if it's in a blocking/looping on a mutex. We want to avoid

// using the normal proxying mechanism to send this message since it can

// allocate (or otherwise itself crash) so use a low level atomic primitive

// for this signal.

So since this function is called from a crashed thread, I don't think we can use the normal proxying mechanism here (i.e. __proxy: 'sync') .

sbc100 · 2023-04-17T20:28:46Z

src/library_pthread.js

-        // as weakly referenced so that its existence does not prevent Node.js
-        // from exiting.
+#if ENVIRONMENT_MAY_BE_NODE && PROXY_TO_PTHREAD
+      if (ENVIRONMENT_IS_NODE && worker.proxied_main) {


How about if this read more like:

// Even threads that are strongly referenced, such as the proxied main thread // or threads that are being kept alive long enough to proxy their error state // should be unref() once that are returned to the poll if (worker.strongly_referenced) { worker.unref(); worker.is_strongly_referenced = false; }

Then _emscripten_thread_set_strongref could set is_strongly_referenced to true and PROZY_TO_PTHREAD could explictly call _emscripten_thread_set_strongref on main's pthread instead of adding the magic extra argument to __pthread_create_js. ?

Even better I think we can just unconditionally call worker.unref(); here! Since according the docs "If the worker is already unref()ed calling unref() again has no effect." (https://nodejs.org/api/worker_threads.html#workerunref)

Indeed, that extra argument added to __pthread_create_js was a bit of an weird way to identify the proxied main. I reverted those commits and instead added a comment to clarify that .ref()/.unref() is backed by a boolean.

By marking workers as weakly referenced. This corresponds to the ideal scenario as mentioned in commit a9cbf47. (originally based on commit e6f86a8 and 4b4faf9)

This change was generated using: $ ./test/runner other.*code_size* other.*metadce* --rebase

src/library_pthread.js

sbc100

I've very excited to see this change land!

sbc100 · 2023-04-18T15:45:17Z

src/library_pthread.js

+  _emscripten_thread_set_strongref: function(thread) {
+#if ENVIRONMENT_MAY_BE_NODE
+    if (!ENVIRONMENT_IS_NODE) return;
+    PThread.pthreads[thread].ref();


For a two line function like this I think maybe the positive check is more readable:

if (ENVIRONMENT_IS_NODE) { PThread.pthreads[thread].ref(); }

(and maybe fewer bytes of code?)

Addressed with 8f2bc84, there were no changes in the code size tests.

sbc100 · 2023-04-18T15:48:51Z

system/lib/libc/crt1_proxy_main.c

+  if (rc == 0) {
+    // Mark the thread as strongly referenced, so that Node.js doesn't exit
+    // while the pthread is running.
+    _emscripten_thread_set_strongref(thread);


I guess one alternative to adding this new API would be to add a custom pthread attr to signal this? However, since this is an internal API we can always experiment with changing that later.

I experimented with a custom pthread attr (similar to emscripten_pthread_attr_settransferredcanvases()), but that was more complicated than doing this. _emscripten_thread_set_strongref() is also used for crashed threads, so it can't be removed in favor of a custom pthread attr.

sbc100 · 2023-04-18T15:51:40Z

src/library_pthread.js

-        // Once a pthread has finished and the worker becomes idle, mark it
-        // as weakly referenced so that its existence does not prevent Node.js
-        // from exiting.
+        // Once the proxied main thread has finished, mark it as weakly


The comment here is specific to the proxied main but the _emscripten_thread_set_strongref seems like a generic function.

Perhaps the comment could say

// In case a thread was marked as strongly referenced using // _emscripten_thread_set_strongref (for example, the proxied main thread) // we need to ensure that its un-referenced when we the thread exits and // the works is returned to the pool

This .unref() is conditional guarded by PROXY_TO_PTHREAD, so I think the comment still makes sense.

In other words, we can be sure that all threads are already weakly referenced when the worker is returned to the pool, expect for the "main" thread in PROXY_TO_PTHREAD mode; since that one is strongly referenced.

Fair enough. I hadn't noticed that this block was now only PROXY_TO_PTHREAD.

sbc100 · 2023-04-18T15:52:28Z

src/library_pthread.js

@@ -571,6 +572,13 @@ var LibraryPThread = {
    else postMessage({ 'cmd': 'cleanupThread', 'thread': thread });
  },

+  _emscripten_thread_set_strongref: function(thread) {


Perhaps a comment here about how the sole current use case for this function is for keeping the "main" thread alive in PROXY_TO_PTHREAD mode?

Addressed with 8f2bc84.

sbc100 · 2023-04-18T15:54:06Z

system/lib/pthread/emscripten_yield.c

+      // Mark the crashed thread as strongly referenced so that Node.js doesn't
+      // exit while the pthread is propagating the uncaught exception back to
+      // the main thread.
+      _emscripten_thread_set_strongref(crashed_thread_id);


I actually think this change might help with the race conditions we've been seeing in certain pthread tests.

Is is possible that by the time we get here, the crashed_thread_id has already be cleaned up and that crashed_thread_id is no longer a valid thread pointer?

I actually think this change might help with the race conditions we've been seeing in certain pthread tests.

Did you mean issue #15014? I think that race condition is still there, I just responded to that issue.

Is is possible that by the time we get here, the crashed_thread_id has already be cleaned up and that crashed_thread_id is no longer a valid thread pointer?

Given that crashed threads are never cleaned up (i.e. they do not call _emscripten_thread_exit), I think we can be sure that the pthread and corresponding worker are still alive up once it arrives here.

There could be a possibility that the uncaught exception will be re-thrown on the main thread before it ever arrives here, but since the program will be finished by then, I don't think that would be an issue.

sbc100

Is this worth adding a ChangeLog entry for?

Feel free to merge either way, we can always add one later. I'm excited to see this land.

kleisauke force-pushed the node-weak-ref branch from dac5e90 to fbbc36d Compare March 28, 2023 09:01

kleisauke added a commit to kleisauke/wasm-vips that referenced this pull request Mar 28, 2023

Test build for emscripten-core/emscripten#19073

9be0035

This partially reverts commit 9cdd7b3.

sbc100 reviewed Mar 28, 2023

View reviewed changes

sbc100 reviewed Mar 29, 2023

View reviewed changes

src/library.js Outdated Show resolved Hide resolved

kleisauke force-pushed the node-weak-ref branch from a32d320 to b5cc5de Compare April 2, 2023 12:54

sbc100 reviewed Apr 4, 2023

View reviewed changes

kleisauke force-pushed the node-weak-ref branch from a3c75cb to 22893f5 Compare April 13, 2023 09:08

sbc100 reviewed Apr 17, 2023

View reviewed changes

kleisauke added 2 commits April 18, 2023 09:50

Make Node.js pthreads behavior in line with native

57b00e5

By marking workers as weakly referenced. This corresponds to the ideal scenario as mentioned in commit a9cbf47. (originally based on commit e6f86a8 and 4b4faf9)

Rebaseline codesize tests. NFC

96d3452

This change was generated using: $ ./test/runner other.*code_size* other.*metadce* --rebase

kleisauke force-pushed the node-weak-ref branch from 8025fdd to 96d3452 Compare April 18, 2023 08:02

RReverser reviewed Apr 18, 2023

View reviewed changes

src/library_pthread.js Outdated Show resolved Hide resolved

kleisauke added 2 commits April 18, 2023 14:42

Address review comment

16e738e

Add comment

599441e

sbc100 approved these changes Apr 18, 2023

View reviewed changes

sbc100 reviewed Apr 18, 2023

View reviewed changes

Address review comments

8f2bc84

sbc100 approved these changes Apr 20, 2023

View reviewed changes

sbc100 reviewed Apr 20, 2023

View reviewed changes

kleisauke added 2 commits April 21, 2023 12:43

Merge branch 'main'

6b45b4b

Add changelog note

2a09f71

sbc100 approved these changes Apr 21, 2023

View reviewed changes

Merge branch 'main'

04f23f6

kleisauke enabled auto-merge (squash) April 22, 2023 09:37

kleisauke merged commit 9757ac1 into emscripten-core:main Apr 22, 2023

kleisauke deleted the node-weak-ref branch April 22, 2023 11:28

kleisauke mentioned this pull request Sep 10, 2024

Fix keepRuntimeAlive in the case where EXIT_RUNTIME=0 and noExitRuntime is not referenced #22542

Merged

bakkot mentioned this pull request Dec 6, 2024

keeping node alive while workers are working #23092

Open

	if (Module['__emscripten_thread_crashed']) {
	Module['__emscripten_thread_crashed']();
	}
	throw ex;

	if (thread_crashed) {
	// Return the event loop so we can handle the message from the crashed
	// thread.
	emscripten_exit_with_live_runtime();
	}

	// Mark the thread as strongly referenced so that Node.js doesn't exit
	// while the pthread is running.
	_emscripten_thread_set_strongref(t, 1);

	// Once worker is loaded & idle, mark it as weakly referenced,
	// so that mere existence of a Worker in the pool does not prevent
	// Node.js from exiting the app.
	worker.unref();

	// Once a pthread has finished and the worker becomes idle, mark it
	// as weakly referenced so that its existence does not prevent Node.js
	// from exiting.
	worker.unref();

	// When a secondary thread crashes, we need to be able to interrupt the main
	// thread even if it's in a blocking/looping on a mutex. We want to avoid
	// using the normal proxying mechanism to send this message since it can
	// allocate (or otherwise itself crash) so use a low level atomic primitive
	// for this signal.

Make Node.js pthreads behavior in line with native #19073

Make Node.js pthreads behavior in line with native #19073

Conversation

kleisauke commented Mar 26, 2023 • edited Loading

RReverser commented Mar 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleisauke Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

kleisauke commented Mar 26, 2023 •

edited

Loading

kleisauke Apr 20, 2023 •

edited

Loading