Skip to content

Handle Builtin Atomics Async Critical Section in the Main Thread#976

Open
komyg wants to merge 2 commits intotrynova:mainfrom
komyg:fix/built-atomics-async-implementation
Open

Handle Builtin Atomics Async Critical Section in the Main Thread#976
komyg wants to merge 2 commits intotrynova:mainfrom
komyg:fix/built-atomics-async-implementation

Conversation

@komyg
Copy link
Copy Markdown
Contributor

@komyg komyg commented Mar 26, 2026

Fixes this comment: #956 (review)

@komyg
Copy link
Copy Markdown
Contributor Author

komyg commented Apr 7, 2026

Hey @aapoalas, I fixed a timeout issue that we were having on Test262, and all tests are passing now.

Could you review this when you have the time?

Copy link
Copy Markdown
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this doesn't unfortunately really do what is advertised or requested by the specification.

The point is that we should be doing the following steps for the async case especially:

  1. Acquire WaiterList lock.
  2. Read value from buffer.
  3. Compare value to expected and return NotEqual if it is not equal.
  4. Produce a new waiter and add it into the WaiterList.
  5. Return a Promise tied to the new waiter.

Or something like that. Instead we do:

  1. Read value from buffer, compare and return NotEqual if not equal.
  2. Start a new thread and wait until the new thread has started up.
  3. On the new thread, acquire the WaiterList lock and tell the main thread we've started up.
  4. Still on the new thread, produce a new waiter and add it into the WaiterList.
  5. On the main thread return a Promise tied to the new thread.

The issue is that between steps 1 and 3 it's possible for some foreign thread to change the value to not be our expected value, acquite the WaiterList lock, and wake up all waiters. Our waitAsync call will not be woken up by this because the foreign thread got to the WaiterList first, we are not in it yet, but we also managed to read the old expected value from the buffer. This ends up with our waitAsync Promise never resolving.

The fundamental mistake that I made in the original implementation was using a native thread for the waiter: that makes it really hard (or slow) to have the read happen within the critical section. At the end of the day the correct choice should be to leave the threading and timeouting to the embedder.


let data_block = buffer.get_data_block(agent);

// Re-read value under critical section to avoid TOCTOU race.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: You're not within the critical section here, and we've already performed the same read above anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants