Fix a deadlock issue with emscripten_lock_async_acquire() #25670

juj · 2025-10-28T14:12:55Z

Fix a deadlock issue with emscripten_lock_async_acquire() if user attempted to synchronously acquire the lock right after asynchronously acquiring it. b25abd5#r168975511

…empted to synchronously acquire the lock right after asynchronously acquiring it. emscripten-core@b25abd5#r168975511

* Add Wasm Workers Add Wasm Workers * Add TLS test, ES6ify. * Update test * Add TLS support to Wasm Workers * Add C++11 thread_local keyword test. * Add test for C11 _Thread_local. * Add emscripten_malloc_wasm_worker and rename creation API a little. * Add documentation for Wasm Workers. * Flake and lint and fix build error * Remove deps_info dependency that does not work in current setup * __builtin_wasm_tls_align() can be zero * Add more notes about __builtin_wasm_tls_align() being zero * Add test for GCC __thread keyword. * Fix test_wasm_worker_malloc * Fix emscripten_lock_async_acquire() * Fix thread stack creation. * Fix wasm64 build * Add slack to lock_busyspin_wait_acquire * Fix typo in setting * Remove removal of TextDecoder in threads. * Fix non-Wasm Workers build * Fix file system case sensitivity * Fix Wasm Workers proxying mode generation. * Skip TLS tests on Linux, they produce an internal compiler error. * Fix typo * Fix wasm_worker.h include from C code. * Add library_wasm_worker_stub.c. * Wasm Workers working on default runtime. * flake * Disable most wasm workers tests to debug CI * Fix non-minimal runtime wasm workers startup. Add test for WASM_WORKERS=2 build mode. * Simplify in MINIMAL_RUNTIME preamble assignment for wasm maximum memory. * Fix USE_PTHREADS+WASM_WORKERS line. * Add support for simultaneous pthreads + Wasm workers. * Do not pass redundant TLS size to Wasm Worker creation side. * Update emcc.py wasm worker deps * Remove special handling of .S files in system_libs build * Update documentation * Add code size test. * flake * Update tests and wasm worker MT build * Fix mt build * Adjust mt build * Update code size test * Update hello worker wasm * flake * Address review: Allow building with -sSHARED_MEMORY and add a test. Move code from emcc.py to library_wasm_worker.js. * Remove unnecessary dynCall statements * Update mention of C11 and C++11 Atomics APIs * Remove old code. * Utilize runOnMainThread() in MINIMAL_RUNTIME ready handler. * Simplify code * #error quotes * Clean typo * Cleanup tests * Update ChangeLog * Fixes * Add test files. * Fix pthreads * Remove moved test * Address review * Small code size optimization * Small code size opt * Flake * Update Wasm Workers code size test

lindell

Thanks for the fast PR!

lindell · 2025-10-28T14:35:32Z

src/lib/libwasm_worker.js

-    tryAcquireLock();
+    // Asynchronously dispatch acquiring the lock so that we have uniform control flow in both
+    // cases when the lock is acquired, and when it needs to wait.
+    setTimeout(tryAcquireLock);


As discussed in b25abd5#r168975511

I prefer this to run as fast as possible if possible. I don't think it is unreasonable to expect the emscripten_lock_async_acquire to run directly with the things that it entails if the lock is free.

But if we are to do this, would queueMicrotask work here instead? That would avoid things like renders to run before we try to acquire the lock.

It might be possible, although the consensus here is that a microtask should be a short-lived task. We have no possibility to state the semantics on behalf of the user. If this was a new API, we could freely say that should be the model - but since this is an already shipped API, we cannot change/impose semantics on existing users.

I think it would be best to compose using the existing functions. For example,

if (emscripten_lock_busyspin_wait_acquire(lock, 0.5/*msecs*/)) weHaveLock(userData); else emscripten_lock_async_acquire(lock, weHaveLock, userData, INFINITY);

or

if (emscripten_lock_try_acquire(lock)) weHaveLock(userData); else emscripten_lock_async_acquire(lock, weHaveLock, userData, INFINITY);

Would give a convenient way to get the fast access path synchronously.

Performance here should be optimal whenever there is no long-lived contention. And in the case there is > 0.5msec contention, latency will be on the slow path in any case since emscripten_lock_async_acquire() will yield to the event loop (the fact that it performs a single extra CAS will not be observable.. e.g. emscripten_lock_busyspin_wait_acquire() will have performed millions of CASes already, one more won't matter)

although the consensus here is that a microtask should be a short-lived task

Do you have any links to these discussions?

The Task vs Microtask has no semantic difference in "size", just when they are expected to run.

The thing we need to ask ourself is if we want the Atomic.awaitSync to be able to happen before or after a page render if it is queued (or other similar tasks). Eg. should we always yield to the eventloop when we want to aquire the lock async. I would argue to only yield if the lock is blocking.

But in the end, I don't think this matters a lot compared to the big difference of removing the yielding in the critical section which this PR fixes 👍

Do you have any links to these discussions?

This was from

https://developer.mozilla.org/en-US/docs/Web/API/Window/queueMicrotask and https://developer.mozilla.org/en-US/docs/Web/API/HTML_DOM_API/Microtask_guide : "The microtask is a short function which will run after ... "

The thing we need to ask ourself is if we want the Atomic.awaitSync to be able to happen before or after a page render if it is queued (or other similar tasks). Eg. should we always yield to the eventloop when we want to aquire the lock async. I would argue to only yield if the lock is blocking.

I understand you're in the mindset of designing what would be the best API. And I agree with that, though given this is an already shipped API, that would change the semantics. For example, if user writes

emscripten_set_timeout(doSomething, 0 /*msecs*/, userData); emscripten_lock_async_acquire(lock, weHaveLock, userData, INFINITY);

Should the async timeout callback trigger first? Or the async lock acquire trigger first?

Currently the computation model is consistent to always trigger the timeout first. Using a microtask would change the async lock callback to trigger either before or after the timeout depending on whether there was contention from other threads or not.

Maybe it would be a better design for one to say "well the above should be unspecified, don't rely on it as a end-user." But given this is an already shipped API, I am very cautious to change that behavior.

It is possible to manually control this behavior with the above two constructs to get that sync functionality, so one can already get the necessary thing with a couple of extra lines without a performance penalty.

I missed the current description.
"The calling thread will asynchronously try to obtain the given lock after the calling thread yields back to the event loop"

With it being explicitly defined, I agree we should not change its behavior, compared to it being undefined.

Personally I think using the microtask queue should be fine here, and we could update the documentation to say "yields back to the microtask queue".

If there is code out there that is dependant on the ordering the the two callback about that seems way to fragile. Also, such code would have been broken by the existence of the current bug (i.e. the current bug this this PR fixes basically ensures that no such code exists in the wild yet, so now would be good time to switch to the microtask queue.. although that should be a followup PR I think).

juj · 2025-10-29T21:05:23Z

@tlively Would you be available to review this PR, since you worked on Emscripten's multithreading also in the past?

sbc100

lgtm with some nits

sbc100 · 2025-10-29T21:21:03Z

test/test_browser.py

+  @also_with_minimal_runtime
+  @flaky('https://github.com/emscripten-core/emscripten/issues/25270')
+  def test_wasm_worker_lock_async_and_sync_acquire(self):
+    self.btest('wasm_worker/lock_async_and_sync_acquire.c', expected='1', cflags=['--closure=1', '-sWASM_WORKERS'])


Can this use btest_exit (and default to zero return meaning success)?

(Also does it need --closure=1? Does this make sense at -O0?)

sbc100 · 2025-10-29T21:28:48Z

test/test_browser.py


+  # Tests emscripten_lock_async_acquire() function when lock is acquired both synchronously and asynchronously.
+  @also_with_minimal_runtime
+  @flaky('https://github.com/emscripten-core/emscripten/issues/25270')


Because this test doesn't actually create any workers this flaky decorator isn't applicable is it?

sbc100 · 2025-10-29T21:30:01Z

test/wasm_worker/lock_async_and_sync_acquire.c

+  emscripten_lock_release(&lock);
+  printf("on_acquire: released lock.\n");
+#ifdef REPORT_RESULT
+  REPORT_RESULT(result);


Can't you replace this with assert(main_done)?

sbc100 · 2025-10-29T21:47:35Z

system/include/emscripten/wasm_worker.h

+// NOTE 2: This function will always acquire the lock asynchronously. That is,
+//         the lock will only be attempted to acquire after current control flow
+//         yields back to the browser, so that the Wasm call stack is empty.
+//         This is to guarantee an uniform control flow.


This extra note seems redundant with the second sentence above? Maybe you can tune the second sentence instead of re-stating here if you think there is a difference?

juj added 2 commits October 28, 2025 16:09

Fix a deadlock issue with emscripten_lock_async_acquire() if user att…

9d120b0

…empted to synchronously acquire the lock right after asynchronously acquiring it. emscripten-core@b25abd5#r168975511

Add comment

5325528

lindell reviewed Oct 28, 2025

View reviewed changes

lindell approved these changes Oct 29, 2025

View reviewed changes

sbc100 reviewed Oct 29, 2025

View reviewed changes

Fix a deadlock issue with emscripten_lock_async_acquire() #25670

Are you sure you want to change the base?

Fix a deadlock issue with emscripten_lock_async_acquire() #25670

Conversation

juj commented Oct 28, 2025

Uh oh!

lindell 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!

juj Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

juj commented Oct 29, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

juj Oct 29, 2025 •

edited

Loading