tc39 / proposal-atomics-wait-async

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

Consistency question, re: handling of exceptions thrown during input validation #28

Closed rwaldron closed 4 years ago

rwaldron commented 4 years ago

Currently, any errors that occur as a result of failed input validation are thrown synchronously. This means that safe usage of Atomics.waitAsync() will look like this:

try {
  Atomics.waitAsync(...).then(...);
} catch (error) {
  ...
}

The sync flag in DoWait() could be used to instead handle these errors via IfAbruptRejectPromise(abrupt completion value, promiseCapability).

WDYT?

rwaldron commented 4 years ago

@syg @lars-t-hansen I'm in the process of writing tests for this feature and I'm finding that this issue keeps coming up. Can I get some thoughts here?

Also, I want to hear from @ljharb as well.

ljharb commented 4 years ago

This is a good catch; async function was explicitly designed to never throw a sync exception, including from default arguments - it seems ideal to follow the same precedent here.

syg commented 4 years ago

Edit: Oh wait, I think I misunderstood the initial question. Your validation questions are about passing non-TypedArrays and so forth? What I said below doesn't apply to that, but it does point out a pretty bad design flaw in that the case below I describe is the "not equal" case, which is rejected. This is a good catch and I'd need to think a bit more about it...

This is intentional because it would be hard to use Atomics.waitAsync for what it was designed for otherwise.

Atomics.wait, and thus Atomics.waitAsync, are basically Linux kernel futexes. Futexes are very low-level building blocks for other synchronization mechanisms, like mutexes. The design philosophy of futexes is to be very fast when the synchronization does not need to wait, and only do the slow thing if it's necessary. What does "does not need to wait" mean? The canonical example is a mutex. A user sees two states, unlocked and locked, but non-naive implementation needs at least three states: unlocked, locked, and locked and contended.

So, it's important that futexes fail early and fast when the initial value comparison fails so the user code knows that there is contention right now. If users of waitAsync must wait for the current JS task to execute to completion and for the microtask queue to start draining to find out that there is contention, that knowledge is probably so stale at that point you can't really write a good lock with it.

I'd also like to hear any thoughts from @binji here.

syg commented 4 years ago

My current thinking is that validation errors should reject the promise. I'll move the discussion of eagerly returning "not-equal" off of this issue. Found SYNC-RESOLVE.md from 3 years ago where we came down to not having any sync "not-equal" result, though now I have renewed worries about the performance.

syg commented 4 years ago

Though to be clear this is a needs-consensus change. I'll make a PR to the agenda for a deprioritized item.

syg commented 4 years ago

@ljharb I've chatted with some more folks since https://github.com/tc39/proposal-atomics-wait-async/issues/28#issuecomment-604692995 and @jridgewell pointed me to https://2ality.com/2016/03/promise-rejections-vs-exceptions.html, which seems well reasoned. That argues that invalidation errors should fail fast and remain synchronous exceptions. Are there API examples where invalidation errors are turned into Promise rejections?

ljharb commented 4 years ago

Yes, Promise.all().

syg commented 4 years ago

As another counterpoint to the status quo, I was pointed to https://www.w3.org/2001/tag/doc/promises-guide#always-return-promises.

rwaldron commented 4 years ago

And Promise.any()

rwaldron commented 4 years ago

Looks like we have solution