Skip to content

Conversation

@kleisauke
Copy link
Collaborator

Helps: #23092.

This change was generated using:
$ ./test/runner other.test_codesize_minimal_pthreads --rebase
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we might want to consider this be more deeply before exposing / creating a new public API.

A few reason that we might not want to make this public:

  • This is very node specific and doesn't make sense on any other platform.
  • I think you can get the same effect this using a simple setInterval(() => !done) loop.

If we do want expose this, I was wondering if perhaps we should make part of pthread_attr like we did for emscripten_pthread_attr_settransferredcanvases.

@kleisauke
Copy link
Collaborator Author

I agree; I'll abandon this PR.

Here's a possible alternative patch to address that issue:

--- a/src/library_pthread.js
+++ b/src/library_pthread.js
@@ -188,11 +188,11 @@ var LibraryPThread = {
       // worker pool as an unused worker.
       worker.pthread_ptr = 0;
 
-#if ENVIRONMENT_MAY_BE_NODE && PROXY_TO_PTHREAD
+#if ENVIRONMENT_MAY_BE_NODE
       if (ENVIRONMENT_IS_NODE) {
-        // Once the proxied main thread has finished, mark it as weakly
-        // referenced so that its existence does not prevent Node.js from
-        // exiting.  This has no effect if the worker is already weakly
+        // 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.  This has no effect if the worker is already weakly
         // referenced.
         worker.unref();
       }
@@ -633,7 +633,7 @@ var LibraryPThread = {
     msg.moduleCanvasId = threadParams.moduleCanvasId;
     msg.offscreenCanvases = threadParams.offscreenCanvases;
 #endif
-#if ENVIRONMENT_MAY_BE_NODE
+#if ENVIRONMENT_MAY_BE_NODE && HAS_MAIN
     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.  This

i.e. call .unref() only if a main() function is present. To retain the current behavior in libraries with this patch, users can include the following code:

#include <emscripten/emscripten.h>

int main() {
    emscripten_exit_with_live_runtime();

    return 0;
}

However, I'm uncertain if this is the optimal solution. An alternative could be to guard it behind EXIT_RUNTIME, but this might lead to some test failures, e.g. test_pthread_weak_ref isn't built with EXIT_RUNTIME.

@kleisauke kleisauke closed this Dec 14, 2024
@sbc100
Copy link
Collaborator

sbc100 commented Dec 14, 2024

I'm not 100% against this PR. I actually think something like this might be where we end up. I just want to investigate more options before adding a new API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants