Restore the musl pthread circular thread list, in order to fix the pthread key recreation zero-initialization issue #18111
Conversation
…nitialization behavior, and add relevant test case
mannk123
changed the title
mannk123
changed the title
…tialization behavior - add __tl_sync, and also handle cleanup of the circular list in the cleanupThread case
…tialization behavior - remove unused variable
…tialization behavior - update expectations
|
@kleisauke Thanks for the review, I have made the changes. After some investigation, also modified _emscripten_thread_free_data to adjust the thead list. |
system/lib/pthread/pthread_create.c
Outdated
| __tl_lock(); | ||
|
|
||
| t->next->prev = t->prev; | ||
| t->prev->next = t->next; |
There was a problem hiding this comment.
Why is this needed in addition to the block in _emscripten_thread_exit? All threads should call _emscripten_thread_exit shouldn't they?
There was a problem hiding this comment.
There was a race condition, and that block was supposed to be a fix for it.
What was happening was that, inside pthread_create, the new pthread gets added to the circular list after the call to __pthread_create_js. This leads to a race where the new thread starts running and exits (i.e. calls _emscripten_thread_exit), before it gets added to the list inside pthread_create, meaning the list now contains a DT_EXITED thread that should not be there.
The proper fix here is for the list to be updated before calling __pthread_create_js. If the call fails, the new pthread is then removed from the list. I have made that change.
However, I think it may still make sense to keep the block above, as there are cases where a pthread structure is freed using _emscripten_thread_free_data, without having called _emscripten_thread_exit:
i) In exitRuntime, terminateAllThreads is called which returns all workers to the thread pool and calls _emscripten_thread_free_data on them, regardless of whether they have exited.
ii) Any thread that calls exit() does not call _emscripten_thread_exit either (see e=='unwind' in worker.js), which may be potentially problematic when EXIT_RUNTIME is not enabled. Notably, in the PROXY_TO_PTHREAD case, the "main" thread is itself a pthread that ends up calling exit(), meaning it won't perform _emscripten_thread_exit cleanup either.
Given the above, it seems more correct to have that block above in _emscripten_thread_free_data.
Anyway, the above is just my beginner's understanding of the code, eager to know what you think!
There was a problem hiding this comment.
If a thread doesn't exit with _emscripten_thread_exit that would be a bug, and I don't think we should try to work around it unless we have to.
I suggest we leave _emscripten_thread_free_data alone, unless that change is needs to make a particular test pass?
mannk123
force-pushed
the
main
branch
2 times, most recently
from
4efc174 to
e04d7a3
Compare
…tialization behavior - in pthread_create, fix sequencing of when the new pthread is added to the list
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Thanks for working on this! Looks good aside from a few of comments.
| int __pthread_key_create(pthread_key_t *k, void (*dtor)(void *)) | ||
| { | ||
| pthread_t self = __pthread_self(); | ||
| pthread_t self = __pthread_self(); |
test/test_browser.py
Outdated
|
|
||
| @requires_threads | ||
| def test_pthread_key_recreation(self): | ||
| self.btest_exit(test_file('pthread/test_pthread_key_recreation.cpp'), args=['-sUSE_PTHREADS', '-sPTHREAD_POOL_SIZE']) |
There was a problem hiding this comment.
-sPTHREAD_POOL_SIZE=1 I think would be more clear here.
| static pthread_key_t key; | ||
|
|
||
| static std::mutex mutex; | ||
| static std::condition_variable cond_var; |
There was a problem hiding this comment.
It seems a little odd to make pthread-API stuff with C++ threed API stuff, but maybe its fine?
The alternative would be to use pthread_mutex_t and pthread_cond_t here (you could also then make this in a pure C test without any C++).
…tialization behavior - make changes based on PR review
…tialization behavior - fix indentation
|
@sbc100 Thanks, I have made all the suggested changes. |
3 participants
Restored the musl circular thread list, and added a test case.
One notably adjustment needed for emscripten is, in pthread_create, to add the new thread to the list prior to handing off to a worker using __pthread_create_js, as there is a race condition otherwise since the new thread can potentially run and exit via _emscripten_thread_exit, before the call returns to pthread_create.
On top of that, there is one code path specific to emscripten (and not musl) - when workers are returned to the pool (returnWorkerToPool), _emscripten_thread_free_data is called to free the associated __pthread structure. It's possible for this to happen without the _emscripten_thread_exit cleanup code (removal from the list) having been called. So I also modified _emscripten_thread_free_data to perform the necessary adjustments to the thread list pointers.
Fixes: #8740