Repository navigation
Conversation
Previously, `do_proxy` held `q->mutex` across `get_or_add_tasks_for_thread`, which called realloc and malloc. If an allocation contended on the malloc lock, it could call `emscripten_futex_wait` -> `_emscripten_yield` -> `emscripten_proxy_execute_queue`, attempting to re-lock `q->mutex` on the same thread (or blocking another thread trying to execute the queue). PR #24565 worked around the same-thread deadlock for the system proxying queue by setting `system_queue_in_use = true` during `do_proxy` and skipping `emscripten_proxy_execute_queue` when set. However, that left non-system queues (including user queues and `_dlopen_proxying_queue`) vulnerable to both same-thread and cross-thread deadlocks. Skipping `emscripten_proxy_execute_queue` during a contended `malloc` inside `emscripten_futex_wait` could also consume `NOTIFY_BIT` without executing the system queue, causing subsequent waits to hang due to a lost wakeup. Fix both issues by moving `q->mutex` acquisition into `get_or_add_tasks_for_thread` and performing all allocations and frees outside the lock, re-checking whether another thread (or re-entrant execution) added the task queue or grew the array while the lock was released. This allows `system_queue_in_use` to be reverted back to `executing_system_queue` as a pure recursion guard in `emscripten_proxy_execute_queue`.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the pthread proxying queue implementation in system/lib/pthread/proxying.c to prevent deadlocks during memory allocation. Specifically, it modifies get_or_add_tasks_for_thread to perform allocations outside of the queue's mutex lock, preventing re-entrant locking issues when malloc or realloc triggers queue execution. Additionally, the deadlock test suite in test/pthread/test_pthread_proxy_deadlock.c has been significantly expanded and parameterized in test/test_core.py to cover multiple deadlock and lost-wakeup scenarios. There are no review comments, so I have no feedback to provide.
| static bool do_proxy(em_proxying_queue* q, pthread_t target_thread, task t) { | ||
| assert(q != NULL); | ||
| pthread_mutex_lock(&q->mutex); | ||
| bool is_system_queue = q == &system_proxying_queue; |
There was a problem hiding this comment.
Can we keep is_system_queue.. even though it only has a single use? That keeps the diff smaller and make this condition easier to read ?
| def test_pthread_proxy_deadlock(self): | ||
| self.do_runf('pthread/test_pthread_proxy_deadlock.c') | ||
| @parameterized({ | ||
| 'system_queue': (['-DTEST_SYSTEM_QUEUE'],), |
There was a problem hiding this comment.
I like to always keep the first variant names with the empty string. Is there one of these that can be considered the "default" mode?
That way one can always run the test as names in the source code (e.g. ./test/runner core0.test_pthread_proxy_deadlock actually works).
There was a problem hiding this comment.
Yeah, we can make the first one the default.
| return tasks; | ||
| } | ||
|
|
||
| static _Thread_local bool executing_system_queue = false; |
There was a problem hiding this comment.
So executing_system_queue is like the old system_queue_in_use but a more narrow meaning?
There was a problem hiding this comment.
Yes, and in fact this is just going back to the name and functionality we had before #24565.
| static _Thread_local bool wait_for_worker_in_malloc = false; | ||
| static _Atomic int in_malloc = 0; | ||
| static _Atomic int worker_proxied = 0; | ||
| static _Atomic int task_done = 0; |
There was a problem hiding this comment.
Drop all these static keywords if you can.
| pthread_t worker; | ||
| int rc = pthread_create(&worker, NULL, lost_wakeup_worker_main, &main_thread); | ||
| assert(rc == 0); | ||
| while (!worker_proxied) { |
There was a problem hiding this comment.
Can you use a pthread_barrier_wait here instead, or would that interfere with the test?
There was a problem hiding this comment.
Using pthread_barrier_wait here would interfere with the test: when the first thread to reach pthread_barrier_wait waits for the other thread, it calls emscripten_futex_wait, which calls _emscripten_yield -> emscripten_proxy_execute_queue and consumes NOTIFY_BIT (executing task) before the main thread reaches emscripten_proxy_async(..., worker, nop, NULL). Added a comment explaining why this spins.
| pthread_mutex_unlock(&q->mutex); | ||
| if (tasks != NULL) { | ||
| return tasks; | ||
| } |
There was a problem hiding this comment.
this if (get_tasks_for_thread) return tasks; pattern also seem to occur at the start of the loop below? Perhaps we somehow avoid duplicating up here?
Previously,
do_proxyheldq->mutexacrossget_or_add_tasks_for_thread, which called realloc and malloc. If an allocation contended on the malloc lock, it could callemscripten_futex_wait->_emscripten_yield->emscripten_proxy_execute_queue, attempting to re-lockq->mutexon the same thread (or blocking another thread trying to execute the queue).PR #24565 worked around the same-thread deadlock for the system proxying queue by setting
system_queue_in_use = trueduringdo_proxyand skippingemscripten_proxy_execute_queuewhen set. However, that left non-system queues (including user queues and_dlopen_proxying_queue) vulnerable to both same-thread and cross-thread deadlocks. Skippingemscripten_proxy_execute_queueduring a contendedmallocinsideemscripten_futex_waitcould also consumeNOTIFY_BITwithout executing the system queue, causing subsequent waits to hang due to a lost wakeup.Fix both issues by moving
q->mutexacquisition intoget_or_add_tasks_for_threadand performing all allocations and frees outside the lock, re-checking whether another thread (or re-entrant execution) added the task queue or grew the array while the lock was released. This allowssystem_queue_in_useto be reverted back toexecuting_system_queueas a pure recursion guard inemscripten_proxy_execute_queue.