From fdf56ef1cab8605f42a4ace04f6d44583e03c931 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Fri, 13 Jun 2025 21:30:46 +0000 Subject: [PATCH 01/10] Don't execute the proxy queue while adding to it from same thread. 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. --- system/lib/pthread/proxying.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/system/lib/pthread/proxying.c b/system/lib/pthread/proxying.c index 34a102199c823..7b11c344e4be8 100644 --- a/system/lib/pthread/proxying.c +++ b/system/lib/pthread/proxying.c @@ -21,6 +21,8 @@ struct em_proxying_queue { // Protects all accesses to em_task_queues, size, and capacity. pthread_mutex_t mutex; + // If the mutex is locked this is the thread that is using it. + pthread_t active_thread; // `size` task queue pointers stored in an array of size `capacity`. em_task_queue** task_queues; int size; @@ -30,6 +32,7 @@ struct em_proxying_queue { // The system proxying queue. static em_proxying_queue system_proxying_queue = { .mutex = PTHREAD_MUTEX_INITIALIZER, + .active_thread = NULL, .task_queues = NULL, .size = 0, .capacity = 0, @@ -47,6 +50,7 @@ em_proxying_queue* em_proxying_queue_create(void) { } *q = (em_proxying_queue){ .mutex = PTHREAD_MUTEX_INITIALIZER, + .active_thread = NULL, .task_queues = NULL, .size = 0, .capacity = 0, @@ -124,6 +128,16 @@ void emscripten_proxy_execute_queue(em_proxying_queue* q) { executing_system_queue = true; } + // When the current thread is adding tasks it locks the queue, but we can + // potentially try to execute the queue during the add (from + // emscripten_yield). This will deadlock the thread, so only try to take the + // lock if the current thread is not adding to the queue. We then hope the + // queue is executed later when it is unlocked. + // XXX: This could leave to starvation if we never process the queue. + if (q->active_thread == pthread_self()) { + return; + } + pthread_mutex_lock(&q->mutex); em_task_queue* tasks = get_tasks_for_thread(q, pthread_self()); pthread_mutex_unlock(&q->mutex); @@ -141,7 +155,9 @@ void emscripten_proxy_execute_queue(em_proxying_queue* q) { static int do_proxy(em_proxying_queue* q, pthread_t target_thread, task t) { assert(q != NULL); pthread_mutex_lock(&q->mutex); + q->active_thread = pthread_self(); em_task_queue* tasks = get_or_add_tasks_for_thread(q, target_thread); + q->active_thread = NULL; pthread_mutex_unlock(&q->mutex); if (tasks == NULL) { return 0; From f9af064680229e072df069b87b24e6db1e289505 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Mon, 16 Jun 2025 21:02:22 +0000 Subject: [PATCH 02/10] use previous gaurd --- system/lib/pthread/proxying.c | 51 +++++++++++++++++------------------ 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/system/lib/pthread/proxying.c b/system/lib/pthread/proxying.c index 7b11c344e4be8..807aca63f5281 100644 --- a/system/lib/pthread/proxying.c +++ b/system/lib/pthread/proxying.c @@ -21,8 +21,6 @@ struct em_proxying_queue { // Protects all accesses to em_task_queues, size, and capacity. pthread_mutex_t mutex; - // If the mutex is locked this is the thread that is using it. - pthread_t active_thread; // `size` task queue pointers stored in an array of size `capacity`. em_task_queue** task_queues; int size; @@ -32,12 +30,13 @@ struct em_proxying_queue { // The system proxying queue. static em_proxying_queue system_proxying_queue = { .mutex = PTHREAD_MUTEX_INITIALIZER, - .active_thread = NULL, .task_queues = NULL, .size = 0, .capacity = 0, }; +static _Thread_local int system_queue_in_use = 0; + em_proxying_queue* emscripten_proxy_get_system_queue(void) { return &system_proxying_queue; } @@ -50,7 +49,6 @@ em_proxying_queue* em_proxying_queue_create(void) { } *q = (em_proxying_queue){ .mutex = PTHREAD_MUTEX_INITIALIZER, - .active_thread = NULL, .task_queues = NULL, .size = 0, .capacity = 0, @@ -110,32 +108,28 @@ static em_task_queue* get_or_add_tasks_for_thread(em_proxying_queue* q, return tasks; } -static _Thread_local bool executing_system_queue = false; - void emscripten_proxy_execute_queue(em_proxying_queue* q) { assert(q != NULL); assert(pthread_self()); - // Recursion guard to avoid infinite recursion when we arrive here from the - // pthread_lock call below that executes the system queue. The per-task_queue - // recursion lock can't catch these recursions because it can only be checked - // after the lock has been acquired. - bool is_system_queue = q == &system_proxying_queue; - if (is_system_queue) { - if (executing_system_queue) { - return; - } - executing_system_queue = true; - } - - // When the current thread is adding tasks it locks the queue, but we can + // Below is a recursion and deadlock guard: + // The recursion guard is to avoid infinite recursion when we arrive here from + // the pthread_lock call below that executes the system queue. The + // per-task_queue recursion lock can't catch these recursions because it can + // only be checked after the lock has been acquired. + // + // This also guards against deadlocks when adding to the system queue. When + // the current thread is adding tasks, it locks the queue, but we can // potentially try to execute the queue during the add (from // emscripten_yield). This will deadlock the thread, so only try to take the - // lock if the current thread is not adding to the queue. We then hope the + // lock if the current thread is not using the queue. We then hope the // queue is executed later when it is unlocked. - // XXX: This could leave to starvation if we never process the queue. - if (q->active_thread == pthread_self()) { - return; + int is_system_queue = q == &system_proxying_queue; + if (is_system_queue) { + if (system_queue_in_use) { + return; + } + system_queue_in_use = true; } pthread_mutex_lock(&q->mutex); @@ -148,16 +142,21 @@ void emscripten_proxy_execute_queue(em_proxying_queue* q) { } if (is_system_queue) { - executing_system_queue = false; + system_queue_in_use = false; } } static int do_proxy(em_proxying_queue* q, pthread_t target_thread, task t) { assert(q != NULL); pthread_mutex_lock(&q->mutex); - q->active_thread = pthread_self(); + int is_system_queue = q == &system_proxying_queue; + if (is_system_queue) { + system_queue_in_use = 1; + } em_task_queue* tasks = get_or_add_tasks_for_thread(q, target_thread); - q->active_thread = NULL; + if (is_system_queue) { + system_queue_in_use = 0; + } pthread_mutex_unlock(&q->mutex); if (tasks == NULL) { return 0; From 1b13750c374355fbd483ab81d1627a2d96ac4ca5 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Wed, 2 Jul 2025 22:48:45 +0000 Subject: [PATCH 03/10] comment fix --- system/lib/pthread/proxying.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/system/lib/pthread/proxying.c b/system/lib/pthread/proxying.c index 807aca63f5281..1167bd315989e 100644 --- a/system/lib/pthread/proxying.c +++ b/system/lib/pthread/proxying.c @@ -112,18 +112,18 @@ void emscripten_proxy_execute_queue(em_proxying_queue* q) { assert(q != NULL); assert(pthread_self()); - // Below is a recursion and deadlock guard: - // The recursion guard is to avoid infinite recursion when we arrive here from - // the pthread_lock call below that executes the system queue. The - // per-task_queue recursion lock can't catch these recursions because it can - // only be checked after the lock has been acquired. - // + // Below is a recursion and deadlock guard: The recursion guard is to avoid + // infinite recursion when we arrive here from the pthread_lock call below + // that executes the system queue. The per-task_queue recursion lock can't + // catch these recursions because it can only be checked after the lock has + // been acquired. + // // This also guards against deadlocks when adding to the system queue. When // the current thread is adding tasks, it locks the queue, but we can - // potentially try to execute the queue during the add (from - // emscripten_yield). This will deadlock the thread, so only try to take the - // lock if the current thread is not using the queue. We then hope the - // queue is executed later when it is unlocked. + // potentially try to execute the queue during the add (from emscripten_yield + // when malloc takes a lock). This will deadlock the thread, so only try to + // take the lock if the current thread is not using the queue. We then hope + // the queue is executed later when it is unlocked. int is_system_queue = q == &system_proxying_queue; if (is_system_queue) { if (system_queue_in_use) { From f192ce3996fdef7e93a1285376c210d555d60dc8 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Mon, 7 Jul 2025 23:54:24 +0000 Subject: [PATCH 04/10] bool --- system/lib/pthread/proxying.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/system/lib/pthread/proxying.c b/system/lib/pthread/proxying.c index 1167bd315989e..6feab86c5594c 100644 --- a/system/lib/pthread/proxying.c +++ b/system/lib/pthread/proxying.c @@ -35,7 +35,7 @@ static em_proxying_queue system_proxying_queue = { .capacity = 0, }; -static _Thread_local int system_queue_in_use = 0; +static _Thread_local bool system_queue_in_use = false; em_proxying_queue* emscripten_proxy_get_system_queue(void) { return &system_proxying_queue; @@ -151,11 +151,11 @@ static int do_proxy(em_proxying_queue* q, pthread_t target_thread, task t) { pthread_mutex_lock(&q->mutex); int is_system_queue = q == &system_proxying_queue; if (is_system_queue) { - system_queue_in_use = 1; + system_queue_in_use = true; } em_task_queue* tasks = get_or_add_tasks_for_thread(q, target_thread); if (is_system_queue) { - system_queue_in_use = 0; + system_queue_in_use = false; } pthread_mutex_unlock(&q->mutex); if (tasks == NULL) { From 6dde91f5ab07e0f0f3ef0f8ee9d3105c925c0a16 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Mon, 7 Jul 2025 23:55:40 +0000 Subject: [PATCH 05/10] mo bool --- system/lib/pthread/proxying.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system/lib/pthread/proxying.c b/system/lib/pthread/proxying.c index 6feab86c5594c..63030f1f3877b 100644 --- a/system/lib/pthread/proxying.c +++ b/system/lib/pthread/proxying.c @@ -124,7 +124,7 @@ void emscripten_proxy_execute_queue(em_proxying_queue* q) { // when malloc takes a lock). This will deadlock the thread, so only try to // take the lock if the current thread is not using the queue. We then hope // the queue is executed later when it is unlocked. - int is_system_queue = q == &system_proxying_queue; + bool is_system_queue = q == &system_proxying_queue; if (is_system_queue) { if (system_queue_in_use) { return; @@ -149,7 +149,7 @@ void emscripten_proxy_execute_queue(em_proxying_queue* q) { static int do_proxy(em_proxying_queue* q, pthread_t target_thread, task t) { assert(q != NULL); pthread_mutex_lock(&q->mutex); - int is_system_queue = q == &system_proxying_queue; + bool is_system_queue = q == &system_proxying_queue; if (is_system_queue) { system_queue_in_use = true; } From f6868efda987929127c4878b27869b8bf754b3f1 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Tue, 8 Jul 2025 22:13:24 +0000 Subject: [PATCH 06/10] add test --- test/pthread/test_pthread_proxy_deadlock.c | 47 ++++++++++++++++++++++ test/test_core.py | 4 ++ 2 files changed, 51 insertions(+) create mode 100644 test/pthread/test_pthread_proxy_deadlock.c diff --git a/test/pthread/test_pthread_proxy_deadlock.c b/test/pthread/test_pthread_proxy_deadlock.c new file mode 100644 index 0000000000000..9701a81e9a83f --- /dev/null +++ b/test/pthread/test_pthread_proxy_deadlock.c @@ -0,0 +1,47 @@ +// Copyright 2025 The Emscripten Authors. All rights reserved. +// Emscripten is available under two separate licenses, the MIT license and the +// University of Illinois/NCSA Open Source License. Both these licenses can be +// found in the LICENSE file. + +#include +#include +#include +#include +#include +#include +#include + +bool should_quit = false; +pthread_t looper; + +// In the actual implementation of malloc the system queue may be executed +// non-deterministically if malloc is waiting on a mutex. This wraps malloc and +// executes the system queue during every allocation to make the behavior +// deterministic. +void *malloc(size_t size) { + if (emscripten_proxy_get_system_queue() && emscripten_is_main_runtime_thread()) { + emscripten_proxy_execute_queue(emscripten_proxy_get_system_queue()); + } + void *ptr = emscripten_builtin_malloc(size); + return ptr; +} + +void run_on_looper(void* arg) { + emscripten_out("run_on_looper\n"); + should_quit = true; +} + +void* looper_main(void* arg) { + while (!should_quit) { + emscripten_proxy_execute_queue(emscripten_proxy_get_system_queue()); + sched_yield(); + } + return NULL; +} + +int main() { + pthread_create(&looper, NULL, looper_main, NULL); + emscripten_proxy_async(emscripten_proxy_get_system_queue(), looper, run_on_looper, NULL); + pthread_join(looper, NULL); + emscripten_out("done\n"); +} diff --git a/test/test_core.py b/test/test_core.py index 20dffa0cbd7e6..554b0c17b58cc 100644 --- a/test/test_core.py +++ b/test/test_core.py @@ -2564,6 +2564,10 @@ def test_pthread_cancel(self): def test_pthread_cancel_async(self): self.do_run_in_out_file_test('pthread/test_pthread_cancel_async.c') + @node_pthreads + def test_pthread_proxy_deadlock(self): + self.do_runf('pthread/test_pthread_proxy_deadlock.c') + @no_asan('test relies on null pointer reads') def test_pthread_specific(self): self.do_run_in_out_file_test('pthread/specific.c') From 24a7490af83446949ab39877eeaeb0458e9b1b84 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Tue, 8 Jul 2025 22:47:26 +0000 Subject: [PATCH 07/10] skip asan/lsan --- test/test_core.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/test_core.py b/test/test_core.py index 554b0c17b58cc..86852ec1ba291 100644 --- a/test/test_core.py +++ b/test/test_core.py @@ -2564,6 +2564,8 @@ def test_pthread_cancel(self): def test_pthread_cancel_async(self): self.do_run_in_out_file_test('pthread/test_pthread_cancel_async.c') + @no_asan('cannot replace malloc/free with ASan') + @no_lsan('cannot replace malloc/free with LSan') @node_pthreads def test_pthread_proxy_deadlock(self): self.do_runf('pthread/test_pthread_proxy_deadlock.c') From 27de7520602c759674c0f7a3f76e51108798e391 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Wed, 9 Jul 2025 21:57:33 +0000 Subject: [PATCH 08/10] remove check for static data --- test/pthread/test_pthread_proxy_deadlock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/pthread/test_pthread_proxy_deadlock.c b/test/pthread/test_pthread_proxy_deadlock.c index 9701a81e9a83f..26bb9086cc957 100644 --- a/test/pthread/test_pthread_proxy_deadlock.c +++ b/test/pthread/test_pthread_proxy_deadlock.c @@ -19,7 +19,7 @@ pthread_t looper; // executes the system queue during every allocation to make the behavior // deterministic. void *malloc(size_t size) { - if (emscripten_proxy_get_system_queue() && emscripten_is_main_runtime_thread()) { + if (emscripten_is_main_runtime_thread()) { emscripten_proxy_execute_queue(emscripten_proxy_get_system_queue()); } void *ptr = emscripten_builtin_malloc(size); From 97fb5fd63b218d0e1299b7d5bad08c1e99a04880 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Wed, 9 Jul 2025 23:26:43 +0000 Subject: [PATCH 09/10] Add comments. Remove pthread and just send the task to self. --- test/pthread/test_pthread_proxy_deadlock.c | 27 ++++++++-------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/test/pthread/test_pthread_proxy_deadlock.c b/test/pthread/test_pthread_proxy_deadlock.c index 26bb9086cc957..8e28ad8af6896 100644 --- a/test/pthread/test_pthread_proxy_deadlock.c +++ b/test/pthread/test_pthread_proxy_deadlock.c @@ -11,9 +11,6 @@ #include #include -bool should_quit = false; -pthread_t looper; - // In the actual implementation of malloc the system queue may be executed // non-deterministically if malloc is waiting on a mutex. This wraps malloc and // executes the system queue during every allocation to make the behavior @@ -26,22 +23,18 @@ void *malloc(size_t size) { return ptr; } -void run_on_looper(void* arg) { - emscripten_out("run_on_looper\n"); - should_quit = true; -} - -void* looper_main(void* arg) { - while (!should_quit) { - emscripten_proxy_execute_queue(emscripten_proxy_get_system_queue()); - sched_yield(); - } - return NULL; +void task(void* arg) { + emscripten_out("task\n"); } int main() { - pthread_create(&looper, NULL, looper_main, NULL); - emscripten_proxy_async(emscripten_proxy_get_system_queue(), looper, run_on_looper, NULL); - pthread_join(looper, NULL); + // Tests for a deadlock scenario that can occur when sending a task. + // The sequence of events is: + // 1. Sending a task locks the queue. + // 2. Allocating a new task queue calls malloc. + // 3. Malloc then attempts to execute and lock the already-locked queue, + // causing a deadlock. + // This test ensures our implementation prevents this re-entrant lock. + emscripten_proxy_async(emscripten_proxy_get_system_queue(), pthread_self(), task, NULL); emscripten_out("done\n"); } From dcfd1886a026739b6b6d3fe04ddbc5ed1cc8e24a Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Thu, 10 Jul 2025 17:55:37 +0000 Subject: [PATCH 10/10] Automatic rebaseline of codesize expectations. NFC This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (4) test expectation files were updated by running the tests with `--rebaseline`: ``` test/other/codesize/test_codesize_minimal_pthreads.funcs updated other/codesize/test_codesize_minimal_pthreads.size: 19540 => 19590 [+50 bytes / +0.26%] test/other/codesize/test_codesize_minimal_pthreads_memgrowth.funcs updated other/codesize/test_codesize_minimal_pthreads_memgrowth.size: 19541 => 19591 [+50 bytes / +0.26%] Average change: +0.26% (+0.26% - +0.26%) ``` --- test/other/codesize/test_codesize_minimal_pthreads.funcs | 1 + test/other/codesize/test_codesize_minimal_pthreads.size | 2 +- .../codesize/test_codesize_minimal_pthreads_memgrowth.funcs | 1 + .../codesize/test_codesize_minimal_pthreads_memgrowth.size | 2 +- 4 files changed, 4 insertions(+), 2 deletions(-) diff --git a/test/other/codesize/test_codesize_minimal_pthreads.funcs b/test/other/codesize/test_codesize_minimal_pthreads.funcs index b30f8e9f062e9..a0f8a8db793a7 100644 --- a/test/other/codesize/test_codesize_minimal_pthreads.funcs +++ b/test/other/codesize/test_codesize_minimal_pthreads.funcs @@ -72,6 +72,7 @@ $emscripten_futex_wake $emscripten_stack_get_current $emscripten_stack_set_limits $free_ctx +$get_or_add_tasks_for_thread $get_tasks_for_thread $init_active_ctxs $init_file_lock diff --git a/test/other/codesize/test_codesize_minimal_pthreads.size b/test/other/codesize/test_codesize_minimal_pthreads.size index 3a243e798e99c..ae0a3426bb4b8 100644 --- a/test/other/codesize/test_codesize_minimal_pthreads.size +++ b/test/other/codesize/test_codesize_minimal_pthreads.size @@ -1 +1 @@ -19540 +19590 diff --git a/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.funcs b/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.funcs index b30f8e9f062e9..a0f8a8db793a7 100644 --- a/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.funcs +++ b/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.funcs @@ -72,6 +72,7 @@ $emscripten_futex_wake $emscripten_stack_get_current $emscripten_stack_set_limits $free_ctx +$get_or_add_tasks_for_thread $get_tasks_for_thread $init_active_ctxs $init_file_lock diff --git a/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.size b/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.size index fb23a9262b601..23235d71b6a9d 100644 --- a/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.size +++ b/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.size @@ -1 +1 @@ -19541 +19591