Skip to content

Ensure Node worker threads are exited gracefully #12963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 18, 2021
Merged

Ensure Node worker threads are exited gracefully #12963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0b9031
Ensure Node worker threads are exited gracefully
kleisauke Dec 4, 2020
eee663a
posixtest.test_pthread_create_11_1 seems to work again
kleisauke Jun 18, 2021
20a7b8a
Explicitly link test_pthread* tests with EXIT_RUNTIME
kleisauke Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/library_pthread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1055,11 +1055,6 @@ var LibraryPThread = {
if (!ENVIRONMENT_IS_PTHREAD) _exit(status);
else PThread.threadExit(status);
// pthread_exit is marked noReturn, so we must not return from it.
if (ENVIRONMENT_IS_NODE) {
// exit the pthread properly on node, as a normal JS exception will halt
// the entire application.
process.exit(status);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know why this was here in the first place? I don't really understand the comment or why process.exit would ever maskde sense for exiting a thread. (unless perhaps process.exit only applies to the current worker thread which seem unlikely).

In order works I agree this change looks good I just can't see why it was ever any different. Perhaps @kripken can lend some background. I believe this code dates back to the very first pthread node code: #9745

Also, can you mention this fix in the title and/or PR description so it describes what you are fixing exacly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change was discovered by trial-and-error as a attempt to fix the failing Node posix tests. So, I'm not entirely sure why it was added (with commit 5a8c86d) and whether this change is correct.

I'll fix the title and PR description, after I've investigated it a bit further.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is adding EXIT_RUNTIME enough to make this test keep working?

Copy link
Collaborator Author

@kleisauke kleisauke Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

After some further research, it looks like the process.exit() call in a Node worker will immediately cause the thread to exit, and not the whole program. Somehow this call also prevents from new threads to be created(?) and causes the program to hang indefinitely (see for example the added test_pthread_exit.c test case in 38b4bde which hangs on master). The failing stress-test mentioned in #9763 now also seems to work correctly on Node.

EXIT_RUNTIME doesn't make the test_pthread_create test case pass after this change, but calling explicitly exit(0); after cancelling the main loop does (see PR #13211).

Copy link
Member

@kripken kripken Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think I added this because indeed the throw on the next line would shut down the entire program (threads + main runtime), as the comment says. And process.exit seemed to just shut down the thread, but I can't find any docs to justify that, so it may have been a reliance on undefined behavior...

But this testcase generally shows that:

const { Worker, isMainThread, parentPort } = require('worker_threads');
if (isMainThread) {
  // This code is executed in the main thread and not in the worker.
  
  // Create the worker.
  const worker = new Worker(__filename);

  setInterval(() => { console.log('time') }, 1000);
} else {
  // This code is executed in the worker and not in the main thread.

  process.exit();
  //throw "foo";
}

Replace the process.exit with the throw to see the result (that is, the throw stops everything, while the process.exit just stops the worker, but the main thread can keep printing "time" every second).

But, it does look good to me to remove the process.exit here, assuming the throw does not halt the entire program. Do we have tests checking that, that is, that a node pthread can exit while things keep running?

Copy link
Member

Choose a reason for hiding this comment

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

(If things work then I assume we catch that throw higher up.)

throw 'unwind';
},

Expand Down
3 changes: 3 additions & 0 deletions tests/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8246,6 +8246,7 @@ def test_pthread_cxx_threads(self):
def test_pthread_create_pool(self):
# with a pool, we can synchronously depend on workers being available
self.set_setting('PTHREAD_POOL_SIZE', '2')
self.set_setting('EXIT_RUNTIME')
self.emcc_args += ['-DALLOW_SYNC']
self.do_run_in_out_file_test('core/pthread/create.cpp')

Expand All @@ -8261,12 +8262,14 @@ def test_pthread_create_proxy(self):
def test_pthread_create_embind_stack_check(self):
# embind should work with stack overflow checks (see #12356)
self.set_setting('STACK_OVERFLOW_CHECK', 2)
self.set_setting('EXIT_RUNTIME')
self.emcc_args += ['--bind']
self.do_run_in_out_file_test('core/pthread/create.cpp')

@node_pthreads
def test_pthread_exceptions(self):
self.set_setting('PTHREAD_POOL_SIZE', '2')
self.set_setting('EXIT_RUNTIME')
Copy link
Member

Choose a reason for hiding this comment

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

If we need EXIT_RUNTIME in basically all the pthreads tests on node, I wonder if maybe we should just enforce that, that is, error if node pthreads are run without it. There may not be a good enough reason to support pthreads without that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if we need this in all our unit test I don't think that necessarily means that every program wants that behaviour. Basically its always desirable for our unittests to exit the process when they are done, but that isn't true for JS programs in general.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I worry this might be confusing but I agree we can't simplify it as easily as I thought.

self.emcc_args += ['-fexceptions']
self.do_run_in_out_file_test('core/pthread/exceptions.cpp')

Expand Down
24 changes: 2 additions & 22 deletions tests/test_posixtest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ def get_pthread_tests():
}

# Mark certain tests as flaky, which may sometimes fail.
# TODO invesigate these tests.
# TODO investigate these tests.
flaky = {
'test_pthread_cond_signal_1_1': 'flaky: https://github.com/emscripten-core/emscripten/issues/13283',
}
Expand All @@ -145,27 +145,7 @@ def get_pthread_tests():
disabled = {
**unsupported,
**flaky,
'test_pthread_create_11_1': 'never returns',
'test_pthread_barrier_wait_2_1': 'never returns',
'test_pthread_attr_setscope_5_1': 'internally skipped (PTS_UNTESTED)',
'test_pthread_create_5_1': 'never returns',
'test_pthread_exit_1_2': 'never returns',
'test_pthread_exit_2_2': 'never returns',
'test_pthread_exit_3_2': 'never returns',
'test_pthread_exit_4_1': 'never returns',
'test_pthread_getcpuclockid_1_1': 'never returns',
'test_pthread_key_create_1_2': 'never returns',
'test_pthread_rwlock_rdlock_1_1': 'fails with "main: Unexpected thread state"',
'test_pthread_rwlock_timedrdlock_1_1': 'fails with "main: Unexpected thread state"',
'test_pthread_rwlock_timedrdlock_3_1': 'fails with "main: Unexpected thread state"',
'test_pthread_rwlock_timedrdlock_5_1': 'fails with "main: Unexpected thread state"',
'test_pthread_rwlock_timedwrlock_1_1': 'fails with "main: Unexpected thread state"',
'test_pthread_rwlock_timedwrlock_3_1': 'fails with "main: Unexpected thread state"',
'test_pthread_rwlock_timedwrlock_5_1': 'fails with "main: Unexpected thread state"',
'test_pthread_rwlock_wrlock_1_1': 'fails with "main: Unexpected thread state"',
'test_pthread_rwlock_trywrlock_1_1': 'fails with "main: Unexpected thread state"',
'test_pthread_spin_destroy_3_1': 'never returns',
'test_pthread_spin_init_4_1': 'never returns',
}


Expand All @@ -180,7 +160,7 @@ def f(self):
'-Wno-int-conversion',
'-sUSE_PTHREADS',
'-sEXIT_RUNTIME',
'-sTOTAL_MEMORY=268435456',
'-sTOTAL_MEMORY=256mb',
'-sPTHREAD_POOL_SIZE=40']
if browser:
# Only are only needed for browser tests of the was btest
Expand Down