Run atexit handlers before terminating all threads - #14481
Merged
Conversation
kleisauke
commented
Jun 18, 2021
sbc100
reviewed
Jun 18, 2021
sbc100
left a comment
sbc100
left a comment
Collaborator
There was a problem hiding this comment.
The primary change (moving terminateAllThreads) lgtm!
Collaborator
|
@RReverser what do you think about the lsan change here? |
Collaborator
|
I would have though that lsan would register it own exit handler or somehow.. |
Collaborator
I don't understand its implications tbh. |
kleisauke
force-pushed
the
atexit-thread
branch
2 times, most recently
from
June 20, 2021 09:06
d19a715 to
94b61a9
Compare
sbc100
reviewed
Jun 20, 2021
| #endif | ||
| #if USE_PTHREADS | ||
| PThread.terminateAllThreads(); | ||
| #endif |
Collaborator
There was a problem hiding this comment.
I think the per-thread exist handler should go up before the program-wide exit handlers.. (so up on line 25). If you do that perhaps we can avoid the lsan changes? I'm not sure.
So maybe the overall order should be:
- per-thread-exit handler for this thread (
PThread.runExitHandlers();) - kill all other threads (PThread.terminateAllThreads())
- program-wide exit handlercallRuntimeCallbacks(ATEXIT);s (callRuntimeCallbacks(ATEXIT);
- program-wide JS exit handlers (<<< ATEXITS >>>)
Collaborator
Author
There was a problem hiding this comment.
PThread.terminateAllThreads() (2) needs to be run after the program-wide exit handlers (3) and possibly also after the program-wide JS exit handlers (4). To test this, see the added test_pthread_atexit in this PR which deadlocks on the main branch.
I've revised the order to:
- program-wide exit handlers (
callRuntimeCallbacks(__ATEXIT__)). - program-wide JS exit handlers (
<<< ATEXITS >>>). - terminate all other threads (
PThread.terminateAllThreads()). - call exit handlers for the main thread (
PThread.runExitHandlers();).
(this could probably be removed after PR Move__cxa_thread_atexitto native code #14489, but I'm not sure)
I've reverted the LSan changes for now, and just suppressed _emscripten_builtin_pthread_create from being leaky with commit 95e23fa.
Collaborator
There was a problem hiding this comment.
PThread.terminateAllThreads()(2) needs to be run after the program-wide exit handlers (3) and possibly also after the program-wide JS exit handlers (4). To test this, see the addedtest_pthread_atexitin this PR which deadlocks on the main branch.
I still need to take a look at that test but this seems logically incorrect to me. The program wide exit handler run, for example, the C++ static destructors. If we don't kill all the threads before running these then we could have situation where there are threads still running with access to static C++ objects which have been destroyed. For example imagine std::cout has a static destructor and one of the background threads tries to write to cout during this time?
I've revised the order to:
- program-wide exit handlers (
callRuntimeCallbacks(__ATEXIT__)).- program-wide JS exit handlers (
<<< ATEXITS >>>).- terminate all other threads (
PThread.terminateAllThreads()).- call exit handlers for the main thread (
PThread.runExitHandlers();).
(this could probably be removed after PR Move__cxa_thread_atexitto native code #14489, but I'm not sure)I've reverted the LSan changes for now, and just suppressed
_emscripten_builtin_pthread_createfrom being leaky with commit 95e23fa.
Collaborator
There was a problem hiding this comment.
Actually I think maybe you are right. Looking at the musl code for exit() is currently does:
_Noreturn void exit(int code)
{
__funcs_on_exit();
__libc_exit_fini();
__stdio_exit();
_Exit(code)
}
I think that secondary threads are not killed until _Exit is called (which calls __syscall(SYS_exit_group, ec) which I think terminates all the threads).
I guess that static destruction can run while there still threads running.
Collaborator
There was a problem hiding this comment.
Yup .. i ran quick test :
class Foo {
public:
Foo() {
printf("Foo\n");
}
~Foo() {
printf("~Foo\n");
alive = false;
}
bool alive = true;
};
static Foo foo;
static _Atomic int threadCounter = 0;
static _Atomic int running = 1;
static pthread_t thread;
void *workerThread(void* arg) {
threadCounter++;
while (running) {
printf("foo: %d\n", foo.alive);
}
threadCounter--;
return NULL;
}
void mainAtExit() {
printf("mainAtExit\n");
sleep(1);
}
int main(int argc, char* argv[]) {
int rc = atexit(mainAtExit);
assert(rc == 0);
rc = pthread_create(&thread, NULL, workerThread, NULL);
assert(rc == 0);
return 0;
}
Collaborator
There was a problem hiding this comment.
So you are correct terminateAllThreads should come last.. and threads that are still alive that point might have a bad time but that is to be expected.
Collaborator
Author
There was a problem hiding this comment.
Thanks for the investigation. Given that PThread.terminateAllThreads() isn't last now, do you think PThread.runExitHandlers() should occur before PThread.terminateAllThreads() (i.e. swapping 3 and 4 in the above order)?
Also, let me know if you prefer commit 5805465 as a separate PR.
Collaborator
There was a problem hiding this comment.
Yes, that would make a good separate PR I think. IIUC that is not a file that exists upstream right?
Collaborator
There was a problem hiding this comment.
terminateAllThreads() should come last of all I think.
Collaborator
Author
There was a problem hiding this comment.
asan_emscripten.cpp does indeed not exist upstream. I've moved that commit to PR #14516, and moved PThread.terminateAllThreads() to occur as last.
Collaborator
|
I just uploaded #14512 which I'm pretty sure overlaps with this change. I can wait for this to land first if you prefer. |
Collaborator
Author
|
No worries, feel free to land that PR earlier. |
Collaborator
Author
|
PTAL, I've rebased it now that PR #14512 has landed. Note that commit 6ecd0d3 somehow didn't work, see:
|
This was referenced Jul 12, 2021
Collaborator
Author
|
... I've split the ASan/LSan related changes into separate PRs. |
kleisauke
force-pushed
the
atexit-thread
branch
from
September 2, 2021 17:30
b6fe214 to
8640757
Compare
Collaborator
Author
sbc100
approved these changes
Sep 9, 2021
sbc100
left a comment
sbc100
left a comment
Collaborator
There was a problem hiding this comment.
lgtm with a few comments.
Thanks again for all your work on this!
| // ready for such calls, as this can be called after the sanitizers | ||
| // are finalized. Instead, call free directly. | ||
| withBuiltinMalloc(function () { | ||
| #endif |
Collaborator
There was a problem hiding this comment.
The other places that we use withBuiltinMalloc are not guarded with if like this. Instead withBuiltinMalloc becomes a no-op. So I think you can avoid the if checks here.
Collaborator
Author
There was a problem hiding this comment.
I tried that initially, but got this exception when I compiled without sanitizers (for e.g. when running wasm2.test_pthread_atexit).
exception thrown: ReferenceError: withBuiltinMalloc is not defined
| }, | ||
| #if USE_ASAN || USE_LSAN | ||
| freeThreadData__deps: ['$withBuiltinMalloc'], | ||
| #endif |
Collaborator
There was a problem hiding this comment.
Is this an essential part of this change?
Collaborator
Author
There was a problem hiding this comment.
Did you mean this __deps line? Or the withBuiltinMalloc in general? The comment above withBuiltinMalloc should explain why that is necessary, but perhaps it can be elaborated on further.
(Perhaps this could be moved to native code and mark that function with the no_sanitize("address") attribute(?))
Collaborator
There was a problem hiding this comment.
The __deps line is necessary, but none of the conditionals should be needed.
Collaborator
Author
There was a problem hiding this comment.
Ah, I had to move the $withBuiltinMalloc to $PThread__deps instead, otherwise it caused this exception when building without sanitizers:
exception thrown: ReferenceError: withBuiltinMalloc is not defined
|
|
||
| static _Atomic int threadCounter = 0; | ||
| static _Atomic int running = 1; | ||
| static pthread_t thread; |
Collaborator
There was a problem hiding this comment.
We should probably be consistent and either drop the static here or add it to all he functions below.
Collaborator
Author
There was a problem hiding this comment.
I dropped those static variables with commit 786473b.
| threadCounter++; | ||
|
|
||
| while (running) | ||
| emscripten_thread_sleep(1000); |
Collaborator
There was a problem hiding this comment.
Why 1000 here? If we want to test to run as fast as possible should we use a shorter value? Or maybe pthread_cond_wait instead?
Collaborator
Author
There was a problem hiding this comment.
Originally this test case was based on the example from https://www.luke1410.de/blog/2017/02/the-trouble-of-separate-module-atexit-stacks/. But indeed, this can be adjusted by using pthread_cond_wait instead, which is much neater.
Done with commit 786473b.
| self.do_run_in_out_file_test('pthread/test_pthread_dispatch_after_exit.c', interleaved_output=False) | ||
|
|
||
| @node_pthreads | ||
| def test_pthread_atexit(self): |
Collaborator
There was a problem hiding this comment.
Can we add a comment either here or on the C source file describing exactly what we are testing?
Collaborator
Author
|
The diff is now probably best viewable with the |
sbc100
approved these changes
Sep 9, 2021
sbc100
reviewed
Sep 9, 2021
| // sanitizer code that checks some things about pthreads. We are not | ||
| // ready for such calls, as this can be called after the sanitizers | ||
| // are finalized. Instead, call free directly. | ||
| withBuiltinMalloc(function () { |
Collaborator
There was a problem hiding this comment.
Actually I think we can can instead just replace the calls to _free below with calls to _emscripten_builtin_free.
The withBuiltinMalloc is mostly useful for calling entire functions that need to operate in both builtin and non-builtin modes.
Collaborator
|
Can you try running bunch of asan and lsan tests locally to be sure this isn't going to generate any leaks/failures. I suggest |
Collaborator
Author
|
After applying this patch (which disables some LSan tests that never returns on the --- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -2323,6 +2323,7 @@ The current type of b is: 9
self.do_run_in_out_file_test('pthread/test_pthread_dispatch_after_exit.c', interleaved_output=False)
@node_pthreads
+ @no_lsan('never returns when using LSan')
def test_pthread_nested_work_queue(self):
self.set_setting('EXIT_RUNTIME')
self.set_setting('PTHREAD_POOL_SIZE', 1)
@@ -2336,6 +2337,7 @@ The current type of b is: 9
self.do_run_in_out_file_test('pthread/test_pthread_thread_local_storage.cpp')
@node_pthreads
+ @no_lsan('never returns when using LSan')
def test_pthread_cleanup(self):
self.set_setting('EXIT_RUNTIME')
self.set_setting('PTHREAD_POOL_SIZE', 4)
@@ -8306,6 +8308,7 @@ NODEFS is no longer included by default; build with -lnodefs.js
self.do_run_in_out_file_test('pthread/test_pthread_c11_threads.c')
@node_pthreads
+ @no_lsan('never returns when using LSan')
def test_pthread_cxx_threads(self):
self.set_setting('PROXY_TO_PTHREAD')
self.clear_setting('ALLOW_MEMORY_GROWTH')I see this (tested commit 51dcc28): DetailsAnd with this PR (tested commit 4eb2200) DetailsSo, it seems that |
Collaborator
Author
|
With commit b477ef7, I now see: DetailsNote that Line 2314 in c25daad |
Collaborator
|
Oh sorry I didn't mean to send you on an lsan fixing adventure, I had assumed that lsan tests were previously passing but its only asan I guess that we have coverage for. I think this lsan work is good to have but maybe in separate PR? |
Collaborator
Author
|
No problem, it would indeed be better to resolve that in a separate PR. I'll try to open a PR for that next week. I've reverted most of those changes for now, except for the change that regressed the |
Collaborator
|
Can we rebase this and get it landed? |
Although not defined by the POSIX standard, it seems that on most systems, atexit handlers are run before threads are terminated.
kleisauke
force-pushed
the
atexit-thread
branch
from
September 17, 2021 15:03
124726d to
ab85ec9
Compare
Collaborator
Author
|
Rebased as requested. Note that the Should I investigate that failure further? Or do you think we should revert that change for now (resulting in a regression for the |
sbc100
reviewed
Sep 17, 2021
| if (!keepRuntimeAlive()) { | ||
| #if USE_PTHREADS | ||
| PThread.terminateAllThreads(); | ||
| #endif |
Collaborator
There was a problem hiding this comment.
I think this terminateAllThreads is actually still useful for the case when _exit or _Exit is called with multiple threads running.
On node is probably doesn't matter since quit_t will bring down the whole process, but on the web it will mean we don't leak workers in this case.
This will always happen after any atexit processing has occurred.
Collaborator
Author
There was a problem hiding this comment.
I thought it was redundant to call terminateAllThreads here, since the only callers of procExit are:
Line 449 in 565fb36
(which will be called just after
exitRuntime, in case we don't need to keep the runtime alive)
And:
emscripten/src/library_wasi.js
Line 14 in cecc2fe
(which will be only called by
MINIMAL_RUNTIME and STANDALONE_WASM, AFAIK)
I added the terminateAllThreads in postamble_minimal.js to make sure that it still terminates all threads during exit when linking with -sMINIMAL_RUNTIME. But for STANDALONE_WASM pthreads are not supported at all, so this seemed redundant.
Lines 2060 to 2061 in 565fb36
Collaborator
There was a problem hiding this comment.
I thought it was redundant to call
terminateAllThreadshere, since the only callers ofprocExitare:
Line 449 in 565fb36
(which will be called just after
exitRuntime, in case we don't need to keep the runtime alive)
And:
emscripten/src/library_wasi.js
Line 14 in cecc2fe
(which will be only called by
MINIMAL_RUNTIMEandSTANDALONE_WASM, AFAIK)
This callsite is used if the user calls _exit or _Exit. Its basically the exit syscall. Its not just used in STANDALONE_WASM IIUC.
Collaborator
Author
There was a problem hiding this comment.
Ah, yes. I missed this one:
(where
__wasi_proc_exit is just an alias for proc_exit)
Commit ea90c63 fixes this.
Collaborator
Author
There was a problem hiding this comment.
Ah, this also seems to fix those asan.*thread* failures without having to change _free to _emscripten_builtin_free in PThread.freeThreadData. So, I've reverted that with commit 0f6f592.
sbc100
reviewed
Sep 17, 2021
|
|
||
| @node_pthreads | ||
| def test_pthread_atexit(self): | ||
| # Test whether we can terminate a running thread during atexit. |
Collaborator
There was a problem hiding this comment.
How about "Test that secondary threads are still running during atexit handlers"?
Collaborator
Author
There was a problem hiding this comment.
I've rephrased it as "Test to ensure threads are still running when atexit-registered functions are called" with commit ea90c63.
Collaborator
Yes, lets just revert that change and live with the lsan regression. It seems that we only currently run |
Collaborator
Oh.. I see the problem. When lsan we just link This seems like a pre-existing condition that we don't need to fix in this PR. |
sbc100
added a commit
that referenced
this pull request
Sep 17, 2021
This is second attempt at landing a version of d5d5f69 The first time we tried it #13837 we ran into issues with test_pthread_exit_process deadlocking which I tracked down to an issue with `system/lib/libc/musl/src/thread/__wait.c` where it was blocking the main thread forever rather then looping and calling `emscripten_main_thread_process_queued_calls`. Includes a version of #14481 so that should land before this does. Fixes: #13194
Collaborator
Author
The call to `PThread.terminateAllThreads()` in `procExit` that was reintroduced made this unnecessary.
sbc100
approved these changes
Sep 20, 2021
sbc100
left a comment
sbc100
left a comment
Collaborator
There was a problem hiding this comment.
Nice! So is this good to land now?
I left a couple of nits but otherwise looks good to me.
| @@ -1 +1 @@ | |||
| 16368 | |||
Collaborator
There was a problem hiding this comment.
Can this file be reverted now without causes a test failure? (looks like its within the tolerance maybe?).
Collaborator
Author
There was a problem hiding this comment.
It seems to be within the expected tolerance. I reverted it to its original state with commit 4f5be19.
| pthread_mutex_lock(&count_lock); | ||
| if (count == 0) | ||
| pthread_cond_signal(&count_nonzero); | ||
| count++; |
Collaborator
There was a problem hiding this comment.
I think this can be simplified to just:
count = 1;
pthread_cond_signal(&count_nonzero);
I might even just rename int count to bool should_exit since we don't need to actually count anything.
Collaborator
Author
There was a problem hiding this comment.
Indeed, counting isn't necessary here (there are not multiple threads). Simplified with commit 4f5be19.
Collaborator
Author
|
I think this is good now, feel free to land. |
Member
|
Great to see this land, thanks @kleisauke ! |
sbc100
added a commit
that referenced
this pull request
Sep 20, 2021
This is second attempt at landing a version of d5d5f69 The first time we tried it #13837 we ran into issues with test_pthread_exit_process deadlocking which I tracked down to an issue with `system/lib/libc/musl/src/thread/__wait.c` where it was blocking the main thread forever rather then looping and calling `emscripten_main_thread_process_queued_calls`. Includes a version of #14481 so that should land before this does. Fixes: #13194
sbc100
added a commit
that referenced
this pull request
Sep 20, 2021
This is second attempt at landing a version of d5d5f69 The first time we tried it #13837 we ran into issues with test_pthread_exit_process deadlocking which I tracked down to an issue with `system/lib/libc/musl/src/thread/__wait.c` where it was blocking the main thread forever rather then looping and calling `emscripten_main_thread_process_queued_calls`. Includes a version of #14481 so that should land before this does. Fixes: #13194
sbc100
added a commit
that referenced
this pull request
Sep 20, 2021
This is second attempt at landing a version of d5d5f69 The first time we tried it #13837 we ran into issues with test_pthread_exit_process deadlocking which I tracked down to an issue with `system/lib/libc/musl/src/thread/__wait.c` where it was blocking the main thread forever rather then looping and calling `emscripten_main_thread_process_queued_calls`. Includes a version of #14481 so that should land before this does. Fixes: #13194
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Although not defined by the POSIX standard, it seems that on most
systems1, atexit handlers are run before threads are terminated.
Helps: #13837.
Fixes: #14993.
1: AFAIK, only on Windows when linking against Microsoft's Universal
C runtime (UCRT), threads are terminated before atexit handlers are
called. See:
https://www.luke1410.de/blog/2017/02/the-trouble-of-separate-module-atexit-stacks/