Don't execute the proxy queue while adding to it from same thread. #24565

brendandahl · 2025-06-13T21:32:01Z

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.

brendandahl · 2025-06-13T21:32:23Z

Still working on a more reliable test, but I wanted to try this out...

sbc100 · 2025-06-13T21:47:25Z

system/lib/pthread/proxying.c

+  // 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()) {


How about if (q->active_thread && q->active_thread == pthread_self()) { to avoid calling pthread_self at all in the non-contended case?

I wonder if we could combine with the above executing_system_queue check?

At least we could use the same technique of using a TLS variable instead of calling pthread_self? I guess that only works if this issue only applies to the system queue (which I think it does).

At least maybe wrap this whole thing in the is_system_queue check?

I think there's a few different ways to do this and simplify it. I just wanted to try this out with minimal changes first to make sure it's going down the right path.

+1 to reusing the existing thread-local variable once this is confirmed to be the right direction. It can be hoisted to the top level for use from multiple functions if necessary.

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.

sbc100

Nice work!

Hopefully we can also figure out some way to improve the testing ?

sbc100 · 2025-06-16T21:13:24Z

system/lib/pthread/proxying.c

@@ -35,6 +35,8 @@ static em_proxying_queue system_proxying_queue = {
  .capacity = 0,
 };

+static _Thread_local int system_queue_in_use = 0;


We could use bool here along with true/false but maybe thats a bigger/unnecessary change.

For whatever reason I decided to use int instead of bool when I wrote all this, so this is at least consistent with the rest of the file.

sbc100 · 2025-06-16T21:14:44Z

test/code_size/random_printf_wasm2js.json

+  "a.html": 17199,
+  "a.html.gz": 7525,
+  "total": 17199,
+  "total_gz": 7525


How is this is a codesize win!?

Is it perhaps move the TLS init code into the global scope? If so that is kind of crazy.

Strange it looks like the code size test is still failing. Maybe this isn't actually a code size win. Will re-try again.

Makes sense to me. Function-scope TLS has to do all sorts of implicitly thread-safe lazy initialization that global-scope TLS does not have to do.

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: 19396 => 19446 [+50 bytes / +0.26%] test/other/codesize/test_codesize_minimal_pthreads_memgrowth.funcs updated other/codesize/test_codesize_minimal_pthreads_memgrowth.size: 19397 => 19447 [+50 bytes / +0.26%] Average change: +0.26% (+0.26% - +0.26%) ```

tlively · 2025-06-16T21:34:37Z

system/lib/pthread/proxying.c

@@ -35,6 +35,8 @@ static em_proxying_queue system_proxying_queue = {
  .capacity = 0,
 };

+static _Thread_local int system_queue_in_use = 0;


For whatever reason I decided to use int instead of bool when I wrote all this, so this is at least consistent with the rest of the file.

tlively · 2025-06-16T21:35:48Z

system/lib/pthread/proxying.c

+  // potentially try to execute the queue during the add (from
+  // emscripten_yield). This will deadlock the thread, so only try to take the


Suggested change

// potentially try to execute the queue during the add (from

// emscripten_yield). This will deadlock the thread, so only try to take the

// 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

tlively · 2025-06-16T21:38:06Z

test/code_size/random_printf_wasm2js.json

+  "a.html": 17199,
+  "a.html.gz": 7525,
+  "total": 17199,
+  "total_gz": 7525


Makes sense to me. Function-scope TLS has to do all sorts of implicitly thread-safe lazy initialization that global-scope TLS does not have to do.

tlively · 2025-06-16T21:39:21Z

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


I wonder what happened here.

tlively · 2025-06-16T21:40:01Z

Solution LGTM, but I forgot about the testing.

brendandahl · 2025-06-17T16:36:11Z

Changing to using a thread local causes failures in test_pthread_run_on_main_thread_minimal_runtime. I didn't get very far debugging this. I won't be able to work on this until June ~25th, so if anyone wants to pick it up feel free to.

Also use `bool` rather than `int` to better convey intent. Split out from emscripten-core#24565

brendandahl marked this pull request as draft June 13, 2025 21:32

sbc100 reviewed Jun 13, 2025

View reviewed changes

kleisauke mentioned this pull request Jun 14, 2025

Pthread task queue deadlock race condition problem (with pthread_cancel()?) #24570

Open

brendandahl added 2 commits June 16, 2025 21:03

use previous gaurd

aebe57a

brendandahl force-pushed the proxy-deadlock branch from 9bf0082 to fcffdb1 Compare June 16, 2025 21:08

sbc100 reviewed Jun 16, 2025

View reviewed changes

brendandahl force-pushed the proxy-deadlock branch from fcffdb1 to ff117cf Compare June 16, 2025 21:36

tlively approved these changes Jun 16, 2025

View reviewed changes

tlively reviewed Jun 16, 2025

View reviewed changes

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jun 17, 2025

Use global TLS variable in proxying.c. NFC

9ee6223

Also use `bool` rather than `int` to better convey intent. Split out from emscripten-core#24565

sbc100 mentioned this pull request Jun 17, 2025

Use global TLS variable in proxying.c. NFC #24596

Open

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jun 17, 2025

Use global TLS variable in proxying.c. NFC

7b3f8fc

Also use `bool` rather than `int` to better convey intent. Split out from emscripten-core#24565

		// potentially try to execute the queue during the add (from
		// emscripten_yield). This will deadlock the thread, so only try to take the

Don't execute the proxy queue while adding to it from same thread. #24565

Are you sure you want to change the base?

Don't execute the proxy queue while adding to it from same thread. #24565

Uh oh!

Conversation

brendandahl commented Jun 13, 2025

Uh oh!

brendandahl commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Jun 16, 2025

Uh oh!

brendandahl commented Jun 17, 2025

Uh oh!

Uh oh!