Remove __pthread_detached_exit in favor of __emscripten_thread_cleanup. NFC. #15605
Conversation
src/library_pthread.js
Outdated
| @@ -309,7 +304,7 @@ var LibraryPThread = { | |||
| worker.on('error', function(e) { | |||
| worker.onerror(e); | |||
| }); | |||
| worker.on('detachedExit', function() { | |||
| worker.on('exit', function() { | |||
There was a problem hiding this comment.
Why this change rather than just removing this handler completely like above? What is the 'exit' message here?
There was a problem hiding this comment.
The exit -> detachedExit change was mistakenly introduced in PR #12861 (see comment #12861 (comment)), but perhaps it is indeed better to remove this listener completely (since issue #9763 may have already been resolved, test_pthread_create.cpp runs fine on Node).
There was a problem hiding this comment.
Oh I see. Can you split that out into its own fix and mention that is address an accidental change from #12861? Thanks for doing all this work BTW, its much appreciated cleanup.
There was a problem hiding this comment.
No problem! I've reverted this change with commit d69ff5b, and will make a new PR with that (or one that removes this listener completely).
There was a problem hiding this comment.
I just opened PR #19178 for this.
| @@ -236,7 +236,7 @@ void _emscripten_thread_exit(void* result) { | |||
| // object and we are done. | |||
| if (state == DT_DETACHED) { | |||
| self->detach_state = DT_EXITED; | |||
| __pthread_detached_exit(); | |||
| __emscripten_thread_cleanup(self); | |||
There was a problem hiding this comment.
I was thinking about doing this .. but I was slightly worried about free'ing the stack being used by the current thread.
Seems a little dangerous. I don't come up with better solution yet but I was thinking maybe we could instead call this from worker.js? (i.e. once the stack is unwound all the way)?
There was a problem hiding this comment.
IIUC, __emscripten_thread_free_data was also called by the detachedExit/__pthread_detached_exit, so this PR should be completely non-functional (except that it now sends the cleanupThread command, which will also eventually call returnWorkerToPool/__emscripten_thread_free_data).
There was a problem hiding this comment.
IIUC, In the current code, when __pthread_detached_exit is called the detachedExit command is sent back to the main thread which ends up freeing the data, so the data is not freed on the exit'ing thread at all.
There was a problem hiding this comment.
Oh wait, I see that __emscripten_thread_cleanup also posts a message back to the main thread.. so I agree this should be and NFC. Thanks for clarifying!
Note: this commit is no longer needed once PR emscripten-core#15604 lands.
kleisauke
force-pushed
the
remove-detached_exit
branch
from
d69ff5b
to
2eaa7d3
Compare
|
Is there anything needed to land this? I could do this myself, but I'm a little uncomfortable with merging PRs as an external contributor. |
Split from PR #13007.