Exit proxied main with its status when keepalives run out - #27731
servusdei2018 wants to merge 2 commits into
Conversation
With -pthread -sPROXY_TO_PTHREAD -sEXIT_RUNTIME, a main() that returns while a runtime keepalive is held now exits with its status once the count reaches zero, instead of exiting 0 with atexit handlers skipped. Fixes emscripten-core#27721
sbc100
left a comment
There was a problem hiding this comment.
I think fixing this is great idea, but I wonder if we can simplify this solution a little?
| if (proxiedMainDone) { | ||
| proxiedMainDone = false; | ||
| exitOnMainThread(proxiedMainExitCode); | ||
| return; |
There was a problem hiding this comment.
Is this return needed? i.e. can we just rely on the return on line 2216?
There was a problem hiding this comment.
I think so; falling through calls __emscripten_thread_exit(), which tries to post a message to the main thread right as the worker is being terminated, causing a received "6" command from terminated worker error in assertion builds.
There was a problem hiding this comment.
Could we replace proxiedMainDone with isProxiedMainThread? This could be simple boolean what is true only on the proxied main thread.
This could then just be if (proxiedMainDone) exitOnMainThread(EXITSTATUS);
I don't think we need to track whether or not the main is done because if we get to this locaiton in the code its is by definition done.
There was a problem hiding this comment.
Actually, because if this I think we can maybe remove the exit(rtn) call from _main_thread, and just always rely on this maybeExit path to handle calling exit?
There was a problem hiding this comment.
Better still, maybe we can replace the if (ENVIRONMENT_IS_PTHREAD) { just above with if (ENVIRONMENT_IS_PTHREAD && !isProxiedMainThread) {`` .. then the existing (_exit`) below should work.
With -pthread -sPROXY_TO_PTHREAD -sEXIT_RUNTIME, a main() that returns while a runtime keepalive is held now exits with its status once the count reaches zero, instead of exiting 0 with atexit handlers skipped.
Fixes #27721