From 835e91800072e5c9c9c302568e459e03fe63ee48 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Fri, 13 Jun 2025 21:30:46 +0000 Subject: [PATCH 1/9] 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 91fa30fffee8d018cb5e6d8a05eccd8c77a84034 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Mon, 16 Jun 2025 21:02:22 +0000 Subject: [PATCH 2/9] 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 a76f7e2391e17b3b9fd473b461be33581bf5b912 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Wed, 2 Jul 2025 22:48:45 +0000 Subject: [PATCH 3/9] 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 973f70987594e749111f6f1272e18be427636a92 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Mon, 7 Jul 2025 23:54:24 +0000 Subject: [PATCH 4/9] 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 9528e4807874912dbce8810c7d72d0620bb73bc3 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Mon, 7 Jul 2025 23:55:40 +0000 Subject: [PATCH 5/9] 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 7c24ee88a994417857362710a77518c3aa6bf862 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Tue, 8 Jul 2025 22:13:24 +0000 Subject: [PATCH 6/9] 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 d4374ed35dc07..9e28023747fd1 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 2b6857c7de47c549b392586907dda3a506b548ec Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Tue, 8 Jul 2025 22:47:26 +0000 Subject: [PATCH 7/9] 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 9e28023747fd1..ce9fda1705a0f 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 1b8ce30b73e3efeda83492fbde03e917b6f1b5e1 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Wed, 9 Jul 2025 21:57:33 +0000 Subject: [PATCH 8/9] 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 1fd8cb8fde4f53aa3a2d29a34e3534dbf0b50c52 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Wed, 9 Jul 2025 23:26:43 +0000 Subject: [PATCH 9/9] 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"); }