search

tc39 / proposal-atomics-wait-async

"asynchronous atomic wait" for ECMAScript
https://tc39.github.io/proposal-atomics-wait-async/
Other
89 stars 18 forks source link

The per-agent datastructures are broken #13

Closed littledan closed 5 years ago

littledan commented 5 years ago

I don't think the per-agent [[Blocked]] and [[AsyncWaitList]] datastructures work as intended. For example, if an agent first does an Atomics.waitAsync on index i, and then does Atomics.wait on index j, and then someone notifies i memory cell, then the agent will become unblocked, whereas it should really stay blocked.

Instead, I think the WaiterLists should consist of records which contain both an agent signifier and either an indication that this entry blocks the agent, or the Promise Capability that's related to it. (If you want to enforce some priorities, then keep the WaiterList sorted.) Then, this record could be passed to NotifyWaiter so that the appropriate action can be taken. This design would remove the need for additional per-agent data structures.

syg commented 5 years ago

Ah yeah you’re right, good catch. I refactored this incorrectly... there used to be a null indicating “blocked” in the WaiterList pairs. Will fix.

On Wed, Jun 26, 2019 at 09:28 Daniel Ehrenberg notifications@github.com wrote:

I don't think the per-agent [[Blocked]] and [[AsyncWaitList]] datastructures work as intended. For example, if an agent first does an Atomics.waitAsync on index i, and then does Atomics.wait on index j, and then someone notifies i memory cell, then the agent will become unblocked, whereas it should really stay blocked.

Instead, I think the WaiterLists should consist of records which contain both an agent signifier and either an indication that this entry blocks the agent, or the Promise Capability that's related to it. (If you want to enforce some priorities, then keep the WaiterList sorted.) Then, this record could be passed to NotifyWaiter so that the appropriate action can be taken. This design would remove the need for additional per-agent data structures.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tc39/proposal-atomics-wait-async/issues/13?email_source=notifications&email_token=AAAXLUMAW3GYU4ITU5NO4G3P4NVHTA5CNFSM4H3SIASKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G3Z3R2A, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAXLUPMSRLJ5WX33QZTFILP4NVHTANCNFSM4H3SIASA .

littledan commented 5 years ago

@syg I don't think reinstating that will be enough. If you just have a stack of Promise Capabilities, you'll end up resolving the wrong one if waitAsync/notify doesn't happen in exactly the right order. The Promise Capability should be in the WaiterList together with the agent signifier, I think.

syg commented 5 years ago

@littledan That's right, the current per-agent [[AsyncWaiterList]] is buggy and loses the index dimension. The queue-of-queues implementation could be flattened into the current ClusterWaiterList or the per-agent [[AsyncWaiterList]] needs to be per-agent and per-location.