New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Waiter removed twice when waking suspended agent? #861

Open
binji opened this Issue Apr 3, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@binji

binji commented Apr 3, 2017

@AndrewScheidecker pointed out what seems to be a bug in the spec here:
WebAssembly/design#1019 (comment)

Incidentally, I believe I found an erroneous assertion in the SAB spec. In Atomics.wait, step 18 performs RemoveWaiter after Suspend completes. RemoveWaiter asserts that the agent being removed is in the WaiterList. However, if Suspend returned because a call to Atomics.wake called WakeWaiter, the agent will have been removed from the WaiterList already by step 11 of Atomics.wake.

I took a look and this seems right to me. Are we missing something?

cc: @lars-t-hansen @syg

@syg

This comment has been minimized.

Show comment
Hide comment
@syg

syg Apr 4, 2017

Member

@lars-t-hansen wrote that part but I concur with @AndrewScheidecker's reading here that there's double removal in the case of non-spurious wakeup in Atomics.wake. I'd imagine the fix here is to perform RemoveWaiter from Atomics.wake only in the case of spurious wakeup, and in the intentional wakeup case, assert that W is not in WL.

Member

syg commented Apr 4, 2017

@lars-t-hansen wrote that part but I concur with @AndrewScheidecker's reading here that there's double removal in the case of non-spurious wakeup in Atomics.wake. I'd imagine the fix here is to perform RemoveWaiter from Atomics.wake only in the case of spurious wakeup, and in the intentional wakeup case, assert that W is not in WL.

@lars-t-hansen

This comment has been minimized.

Show comment
Hide comment
@lars-t-hansen

lars-t-hansen Apr 4, 2017

Contributor

@syg, I assume by "spurious wakeup" you mean "the agent was explicitly woken, it did not time out". If so, I agree with your suggestions. (If you actually mean "spurious wakeup" then we have bigger problems, because there shouldn't be any spurious wakeups.)

Contributor

lars-t-hansen commented Apr 4, 2017

@syg, I assume by "spurious wakeup" you mean "the agent was explicitly woken, it did not time out". If so, I agree with your suggestions. (If you actually mean "spurious wakeup" then we have bigger problems, because there shouldn't be any spurious wakeups.)

@lars-t-hansen

This comment has been minimized.

Show comment
Hide comment
@lars-t-hansen

lars-t-hansen Apr 4, 2017

Contributor

Opened a PR here: #865

Contributor

lars-t-hansen commented Apr 4, 2017

Opened a PR here: #865

@syg

This comment has been minimized.

Show comment
Hide comment
@syg

syg Apr 4, 2017

Member

@lars-t-hansen Err, I'm not sure what I meant as I typed that while leaving work yesterday -- but timeout is the right thing here.

Member

syg commented Apr 4, 2017

@lars-t-hansen Err, I'm not sure what I meant as I typed that while leaving work yesterday -- but timeout is the right thing here.

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