Handle Builtin Atomics Async Critical Section in the Main Thread#976
Handle Builtin Atomics Async Critical Section in the Main Thread#976komyg wants to merge 2 commits intotrynova:mainfrom
Conversation
|
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? |
aapoalas
left a comment
There was a problem hiding this comment.
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:
- Acquire WaiterList lock.
- Read value from buffer.
- Compare value to expected and return NotEqual if it is not equal.
- Produce a new waiter and add it into the WaiterList.
- Return a Promise tied to the new waiter.
Or something like that. Instead we do:
- Read value from buffer, compare and return NotEqual if not equal.
- Start a new thread and wait until the new thread has started up.
- On the new thread, acquire the WaiterList lock and tell the main thread we've started up.
- Still on the new thread, produce a new waiter and add it into the WaiterList.
- 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. |
There was a problem hiding this comment.
issue: You're not within the critical section here, and we've already performed the same read above anyway.
Fixes this comment: #956 (review)