Skip to content

Commit 21a3a81

Browse files
authored
Don't execute the proxy queue while adding to it from same thread. (#24565)
test_pthread_cancel was intermittently failing with a deadlock. From the backtrace, I found we were adding to the queue which can sometimes call malloc->emscripten_yield->emscripten_proxy_execute_queue which also tries to lock the same queue on the same thread.
1 parent bb1ee9b commit 21a3a81

7 files changed

+74
-11
lines changed

system/lib/pthread/proxying.c

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ static em_proxying_queue system_proxying_queue = {
3535
.capacity = 0,
3636
};
3737

38+
static _Thread_local bool system_queue_in_use = false;
39+
3840
em_proxying_queue* emscripten_proxy_get_system_queue(void) {
3941
return &system_proxying_queue;
4042
}
@@ -106,22 +108,28 @@ static em_task_queue* get_or_add_tasks_for_thread(em_proxying_queue* q,
106108
return tasks;
107109
}
108110

109-
static _Thread_local bool executing_system_queue = false;
110-
111111
void emscripten_proxy_execute_queue(em_proxying_queue* q) {
112112
assert(q != NULL);
113113
assert(pthread_self());
114114

115-
// Recursion guard to avoid infinite recursion when we arrive here from the
116-
// pthread_lock call below that executes the system queue. The per-task_queue
117-
// recursion lock can't catch these recursions because it can only be checked
118-
// after the lock has been acquired.
115+
// Below is a recursion and deadlock guard: The recursion guard is to avoid
116+
// infinite recursion when we arrive here from the pthread_lock call below
117+
// that executes the system queue. The per-task_queue recursion lock can't
118+
// catch these recursions because it can only be checked after the lock has
119+
// been acquired.
120+
//
121+
// This also guards against deadlocks when adding to the system queue. When
122+
// the current thread is adding tasks, it locks the queue, but we can
123+
// potentially try to execute the queue during the add (from emscripten_yield
124+
// when malloc takes a lock). This will deadlock the thread, so only try to
125+
// take the lock if the current thread is not using the queue. We then hope
126+
// the queue is executed later when it is unlocked.
119127
bool is_system_queue = q == &system_proxying_queue;
120128
if (is_system_queue) {
121-
if (executing_system_queue) {
129+
if (system_queue_in_use) {
122130
return;
123131
}
124-
executing_system_queue = true;
132+
system_queue_in_use = true;
125133
}
126134

127135
pthread_mutex_lock(&q->mutex);
@@ -134,14 +142,21 @@ void emscripten_proxy_execute_queue(em_proxying_queue* q) {
134142
}
135143

136144
if (is_system_queue) {
137-
executing_system_queue = false;
145+
system_queue_in_use = false;
138146
}
139147
}
140148

141149
static int do_proxy(em_proxying_queue* q, pthread_t target_thread, task t) {
142150
assert(q != NULL);
143151
pthread_mutex_lock(&q->mutex);
152+
bool is_system_queue = q == &system_proxying_queue;
153+
if (is_system_queue) {
154+
system_queue_in_use = true;
155+
}
144156
em_task_queue* tasks = get_or_add_tasks_for_thread(q, target_thread);
157+
if (is_system_queue) {
158+
system_queue_in_use = false;
159+
}
145160
pthread_mutex_unlock(&q->mutex);
146161
if (tasks == NULL) {
147162
return 0;

test/other/codesize/test_codesize_minimal_pthreads.funcs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ $emscripten_futex_wake
7272
$emscripten_stack_get_current
7373
$emscripten_stack_set_limits
7474
$free_ctx
75+
$get_or_add_tasks_for_thread
7576
$get_tasks_for_thread
7677
$init_active_ctxs
7778
$init_file_lock
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
19540
1+
19590

test/other/codesize/test_codesize_minimal_pthreads_memgrowth.funcs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ $emscripten_futex_wake
7272
$emscripten_stack_get_current
7373
$emscripten_stack_set_limits
7474
$free_ctx
75+
$get_or_add_tasks_for_thread
7576
$get_tasks_for_thread
7677
$init_active_ctxs
7778
$init_file_lock
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
19541
1+
19591
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// Copyright 2025 The Emscripten Authors. All rights reserved.
2+
// Emscripten is available under two separate licenses, the MIT license and the
3+
// University of Illinois/NCSA Open Source License. Both these licenses can be
4+
// found in the LICENSE file.
5+
6+
#include <pthread.h>
7+
#include <stdlib.h>
8+
#include <unistd.h>
9+
#include <emscripten/console.h>
10+
#include <emscripten/heap.h>
11+
#include <emscripten/proxying.h>
12+
#include <emscripten/threading.h>
13+
14+
// In the actual implementation of malloc the system queue may be executed
15+
// non-deterministically if malloc is waiting on a mutex. This wraps malloc and
16+
// executes the system queue during every allocation to make the behavior
17+
// deterministic.
18+
void *malloc(size_t size) {
19+
if (emscripten_is_main_runtime_thread()) {
20+
emscripten_proxy_execute_queue(emscripten_proxy_get_system_queue());
21+
}
22+
void *ptr = emscripten_builtin_malloc(size);
23+
return ptr;
24+
}
25+
26+
void task(void* arg) {
27+
emscripten_out("task\n");
28+
}
29+
30+
int main() {
31+
// Tests for a deadlock scenario that can occur when sending a task.
32+
// The sequence of events is:
33+
// 1. Sending a task locks the queue.
34+
// 2. Allocating a new task queue calls malloc.
35+
// 3. Malloc then attempts to execute and lock the already-locked queue,
36+
// causing a deadlock.
37+
// This test ensures our implementation prevents this re-entrant lock.
38+
emscripten_proxy_async(emscripten_proxy_get_system_queue(), pthread_self(), task, NULL);
39+
emscripten_out("done\n");
40+
}

test/test_core.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2564,6 +2564,12 @@ def test_pthread_cancel(self):
25642564
def test_pthread_cancel_async(self):
25652565
self.do_run_in_out_file_test('pthread/test_pthread_cancel_async.c')
25662566

2567+
@no_asan('cannot replace malloc/free with ASan')
2568+
@no_lsan('cannot replace malloc/free with LSan')
2569+
@node_pthreads
2570+
def test_pthread_proxy_deadlock(self):
2571+
self.do_runf('pthread/test_pthread_proxy_deadlock.c')
2572+
25672573
@no_asan('test relies on null pointer reads')
25682574
def test_pthread_specific(self):
25692575
self.do_run_in_out_file_test('pthread/specific.c')

0 commit comments

Comments
 (0)