[AUDIO_WORKLET] Rewrite lock test to be more realistic #25904

cwoffenden · 2025-12-04T16:24:18Z

This is a complete rethink of the lock test to do something locks would actually be used for: maintain atomicity of a complex data structure. We have this dummy struct with three members:

typedef struct {
  uint32_t val0;
  uint32_t val1;
  uint32_t val2;
} Dummy;

A series of calculations are performed thousands of times from both the main and audio thread, involving all the members. Without locks the steps are interleaved, resulting in the wrong result at the end.

It simulates a queue, for example, or any other container. Define DISABLE_LOCKS to run lock-free, and it will fail (with a shorter number of iterations sometimes it would get lucky, but running intensively for a few seconds ensures failure every time).

~~The unlock is kept to a short 10ms and will assert if failed to acquire:~~ The unlock was changed to 1s, with 10ms looking too short for CI:

emscripten/test/webaudio/audioworklet_emscripten_locks.c

Line 84 in d2e6b57

int have = emscripten_lock_busyspin_wait_acquire(&testLock, 1000);

Currently that's 4M calls.

sbc100 · 2025-12-08T20:41:43Z

test/webaudio/audioworklet_emscripten_locks.c

+    dummy->val2 += dummy->val0 * dummy->val1;
+    dummy->val0 /= 4;
+    dummy->val1 /= 3;
+    dummy->val2 /= 2;


Can we avoid these dummy calculation completely, avoiding the use of magic number by just doing something like this:

// This assertion will fail if the the assignment operators on the main thread vs the worker are interleaved assert(dummy->val0 == dummy->val1 == dummy->val3); int newval = dummy->val0 + 1; dummy->val0 = newval; dummy->val1 = newval; dummy->val2 = newval;

I’ll run a test with assigning the same value multiple times (with enough variance each time, a simple increment isn’t enough). It needed to be intensive and for long enough before the clashes started.

How about and shared 1024 block:

char g_shared[1024];

With each thread doing:

memset(g_shared, THREAD_ID, 1024): for (int i = 0; i < 1024; i++) assert(g_shared[i] == THREAD_ID);

Surely that would be enough to detect any kind of interleaving?

Then you could just increate 1024 to larger number to make the interleaving more likely?

~~Rewrote the test without magic numbers.~~

It doesn't clash enough, reverting. Interestingly, doing this doesn't clash:

int dummy_temp0 = dummy->val0; dummy->val0 += dummy->val1 * 7; dummy->val1 += dummy->val2 * 7; dummy->val2 += dummy_temp0 * 7;

I'll need to come back to it later to remove the magic numbers. Testing with --repeat=1000 I can get 100% passes, and then 100% fails when defining DISABLE_LOCKS, and on multiple machines.

Magic numbers removed by calculating them in a single thread, then comparing the multi-thread result.

test/webaudio/audioworklet_emscripten_locks.c

sbc100

The test still seems somewhat convoluted to me, but perhaps that is just how has to be.

@juj WDYT?

cwoffenden · 2025-12-09T16:45:34Z

The test still seems somewhat convoluted to me, but perhaps that is just how has to be.

Too few iterations can accidentally work, and same for running for too short a time. Same with the set-up making sure both threads are running, without synchronisation it can end up running the two threads serially.

juj · 2025-12-09T17:16:28Z

The test looks definitely simpler than before, nice work on the PR. The previous test looks quite a bit like a kitchen sink test.

Maybe the most complexity in the test doesn't come from what the dummy calculation is, but rather the synchronization to ensure that main thread and audio worklet thread are computing at the same time. If there was a way to simplify to remove some of the switch-case states, that might help make the test read cleaner.

Though LGTM to me. Was the conclusion with the Chrome Audio Worklet deadlock hang bug that it was a browser issue, or a non-issue of some kind?

cwoffenden · 2025-12-09T23:17:59Z

Maybe the most complexity in the test doesn't come from what the dummy calculation is, but rather the synchronization to ensure that main thread and audio worklet thread are computing at the same time.

Exactly that, and it's all too easy to run them sequentially. The timeout can end up delaying for a long period before starting (on Chrome), and the worklet won't start until audio playback begins, so this ensures both are ready.

Was the conclusion with the Chrome Audio Worklet deadlock hang bug that it was a browser issue, or a non-issue of some kind?

It seems to be on purpose from Chrome's side, and my conclusion was the test was creating the perfect conditions to trigger some timeout abuse mitigation (CPU hammering whilst spinning for long periods, plus no sound output whilst spinning in the worklet too, resulting in callback delays being stretched out, then the test timing out).

This test scraps the long spinning and ping-poinging between threads for this simpler, more realistic atomic update*. --repeat=1000 with DISABLE_LOCKS defined should fail every time (and pass every time otherwise).

*based on what we do with shipping code and a message queue, which is what kept me coming back to solve this.

cwoffenden · 2025-12-09T23:21:20Z

Hmm, looks like the 10ms timeout is too short on a CI VM for a lock acquire (lengthened and left running for 5000 repeats without problems).

cwoffenden · 2025-12-10T16:36:29Z

Latest failure isn't related:

https://app.circleci.com/pipelines/github/emscripten-core/emscripten/47861/workflows/e0e089a7-1d63-4721-b79e-001378940465/jobs/1089938?invite=true#step-108-3082329_81

It's in audioworklet.c.

The test function is called approx. 200'000x from each thread.

This reverts commit 5677c95.

cwoffenden mentioned this pull request Dec 4, 2025

Can audio worklets pre-emptively interact with the main thread? #24213

Open

cwoffenden force-pushed the cw-aw-rethink-locks branch from cedebcc to 7ab2ad6 Compare December 4, 2025 22:32

cwoffenden requested review from juj and sbc100 December 5, 2025 11:47

cwoffenden mentioned this pull request Dec 5, 2025

[AUDIO_WORKLETS] Move code off the main thread in locks test #25276

Closed

cwoffenden added flaky test audio-worklets labels Dec 5, 2025

cwoffenden mentioned this pull request Dec 5, 2025

test_audio_worklet_emscripten_locks is flaky #25245

Open

sbc100 reviewed Dec 8, 2025

View reviewed changes

cwoffenden force-pushed the cw-aw-rethink-locks branch from e00ba55 to 51b17f4 Compare December 9, 2025 09:43

cwoffenden requested a review from sbc100 December 9, 2025 16:33

sbc100 reviewed Dec 9, 2025

View reviewed changes

cwoffenden force-pushed the cw-aw-rethink-locks branch 2 times, most recently from 1b71c91 to d2e6b57 Compare December 10, 2025 05:56

cwoffenden added 11 commits December 10, 2025 21:07

Complete rethink of the test

b298f2d

Minor tidy

21fcb01

Minor tidy

fceee8f

Should be flake-free

8f19d2b

Lowered lock wait time

41391bd

Something to re-run blocked CI

d3df687

Improved symmetry

1de67c7

The test function is called approx. 200'000x from each thread.

(Failure due to Firefox not downloading on CI)

855e3bc

Moved to EMSCRIPTEN_KEEPALIVE

54815fa

Removed magic numbers

798d1e2

Re-enabled locks

ce9d866

cwoffenden added 6 commits December 10, 2025 21:07

Revert "Removed magic numbers"

0c54c71

This reverts commit 5677c95.

Minor tidy

868f5b3

Pre-calculate magic numbers

dd7c0bd

Ahem, re-enable locks

1e9ab3a

Very, very long lock acquire wait (for slower CI)

29097a5

Clarified text

5c2641b

cwoffenden force-pushed the cw-aw-rethink-locks branch from 332d68e to 5c2641b Compare December 10, 2025 20:07

[AUDIO_WORKLET] Rewrite lock test to be more realistic #25904

Are you sure you want to change the base?

[AUDIO_WORKLET] Rewrite lock test to be more realistic #25904

Conversation

cwoffenden commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 Dec 8, 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!

cwoffenden Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

sbc100 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

sbc100 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

cwoffenden Dec 9, 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!

cwoffenden Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

cwoffenden commented Dec 9, 2025

Uh oh!

juj commented Dec 9, 2025

Uh oh!

cwoffenden commented Dec 9, 2025

Uh oh!

cwoffenden commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwoffenden commented Dec 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cwoffenden commented Dec 4, 2025 •

edited

Loading

sbc100 Dec 8, 2025 •

edited

Loading

cwoffenden Dec 9, 2025 •

edited

Loading

cwoffenden commented Dec 9, 2025 •

edited

Loading