From 21796195994dbcc04598eebc6cf351bd0e27f95f Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Thu, 30 Sep 2021 11:49:46 -0700 Subject: [PATCH] Fix deadlock when calling fflush during application shutdown One solution to the deadlock described in #15186 is to allow nested calls to `emscripten_current_thread_process_queued_calls`. This is needed in this case because the main thread is calling `fflush` (which requires locking the stdout handle) while another thread is holding the stdout lock, waiting on the main thread to process the write action. There may be reasons we don't want to allow nested calls to `emscripten_current_thread_process_queued_calls` but technically its seems possible. This change removes all the mitigations that existed in `test_pthread_c11_threads.c` and I could no longer reprodude the deadlock. Fixes: #15186 --- system/lib/pthread/library_pthread.c | 35 ++++++++---------------- tests/jsrun.py | 2 +- tests/pthread/test_pthread_c11_threads.c | 26 +----------------- 3 files changed, 14 insertions(+), 49 deletions(-) diff --git a/system/lib/pthread/library_pthread.c b/system/lib/pthread/library_pthread.c index 0a4ee29c393eb..55ffec28bc0bb 100644 --- a/system/lib/pthread/library_pthread.c +++ b/system/lib/pthread/library_pthread.c @@ -584,45 +584,34 @@ void emscripten_current_thread_process_queued_calls() { //emscripten_current_thread_process_queued_calls(), ' + new Error().stack)); // #endif - static thread_local bool thread_is_processing_queued_calls = false; - - // It is possible that when processing a queued call, the control flow leads back to calling this - // function in a nested fashion! Therefore this scenario must explicitly be detected, and - // processing the queue must be avoided if we are nesting, or otherwise the same queued calls - // would be processed again and again. - if (thread_is_processing_queued_calls) - return; - // This must be before pthread_mutex_lock(), since pthread_mutex_lock() can call back to this - // function. - thread_is_processing_queued_calls = true; - pthread_mutex_lock(&call_queue_lock); CallQueue* q = GetQueue(pthread_self()); if (!q) { pthread_mutex_unlock(&call_queue_lock); - thread_is_processing_queued_calls = false; return; } - int head = q->call_queue_head; - int tail = q->call_queue_tail; - while (head != tail) { + while (1) { + int head = q->call_queue_head; + int tail = q->call_queue_tail; + if (head == tail) { + break; + } + + // Remove the item from the queue before running it. + em_queued_call* to_run = q->call_queue[head]; + q->call_queue_head = (head + 1) % CALL_QUEUE_SIZE; + // Assume that the call is heavy, so unlock access to the call queue while it is being // performed. pthread_mutex_unlock(&call_queue_lock); - _do_call(q->call_queue[head]); + _do_call(to_run); pthread_mutex_lock(&call_queue_lock); - - head = (head + 1) % CALL_QUEUE_SIZE; - q->call_queue_head = head; - tail = q->call_queue_tail; } pthread_mutex_unlock(&call_queue_lock); // If the queue was full and we had waiters pending to get to put data to queue, wake them up. emscripten_futex_wake((void*)&q->call_queue_head, 0x7FFFFFFF); - - thread_is_processing_queued_calls = false; } // At times when we disallow the main thread to process queued calls, this will diff --git a/tests/jsrun.py b/tests/jsrun.py index 684ddd39747eb..72b759f87f781 100644 --- a/tests/jsrun.py +++ b/tests/jsrun.py @@ -13,7 +13,7 @@ from tools import shared, utils WORKING_ENGINES = {} # Holds all configured engines and whether they work: maps path -> True/False -DEFAULT_TIMEOUT = 5 * 60 +DEFAULT_TIMEOUT = 5 def make_command(filename, engine, args=[]): diff --git a/tests/pthread/test_pthread_c11_threads.c b/tests/pthread/test_pthread_c11_threads.c index ac7a4631ea31a..37a824febcc99 100644 --- a/tests/pthread/test_pthread_c11_threads.c +++ b/tests/pthread/test_pthread_c11_threads.c @@ -17,21 +17,6 @@ void do_once(void) { printf("in do_once\n"); } -// Because this thread is detached it can still be running -// when the main thread exits. And becasue of an emscripten -// bug (https://github.com/emscripten-core/emscripten/issues/15186). -// this means we can't write to stdout after the main thread -// exits. This means we can't use `thread_main` below because -// the destructor to the `tss_t key` writes to stdout. -int thread_main_detached(void* arg) { - printf("in thread_main_detached %p\n", (void*)thrd_current()); - mtx_lock(&mutex); - thread_counter--; - cnd_signal(&cond); - mtx_unlock(&mutex); - return 0; -} - int thread_main(void* arg) { printf("in thread_main %p\n", (void*)thrd_current()); tss_set(key, (void*)thrd_current()); @@ -102,18 +87,9 @@ int main(int argc, char* argv[]) { // Test thrd_detach thrd_t t6; - assert(thrd_create(&t6, thread_main_detached, NULL) == thrd_success); + assert(thrd_create(&t6, thread_main, NULL) == thrd_success); assert(thrd_detach(t6) == thrd_success); - // Wait for the thread to at least be done printing before exiting - // the process. - // We shouldn't need to do this but there is a bug in emscripten - // where a deadlock can occur between main thread calling fflush() - // during exitRuntime and the detached thread calling print (and - // therefore holding the stdout lock). - // See https://github.com/emscripten-core/emscripten/issues/15186. - assert(cnd_wait(&cond, &mutex) == thrd_success); - printf("done!\n"); return 0; }