w3c / web-locks

Cross-tab resource coordination API
https://w3c.github.io/web-locks/
Other
124 stars 16 forks source link

Spec does not say what should happen when aborted after lock acquired #79

Closed saschanaz closed 3 years ago

saschanaz commented 3 years ago

There is a WPT test for this: https://github.com/web-platform-tests/wpt/blob/81bfb439212b8001925d46617fe4a37d4bdc6498/web-locks/signal.tentative.https.any.js#L134-L166

But the spec for abort is:

https://wicg.github.io/web-locks/#dom-lockmanager-request

If options’ signal dictionary member is present, then add the following abort steps to options’ signal dictionary member:

  1. Enqueue the steps to abort the request request to the lock task queue.
  2. Reject promise with an "AbortError" DOMException.

That seems like it always reject regardless of lock acquisition. Should the spec be fixed?

inexorabletash commented 3 years ago

If the promise is already settled because the lock was granted, the abort should be a no-op. (Otherwise, it would be some sort of steal...)

it does sound like the spec is iffy there?

saschanaz commented 3 years ago

I wonder what a general dev would expect here:

const controller = new AbortController();
navigator.locks.request("foo", { signal: controller.signal }, () => {
  return new Promise(() => {});
});
setTImeout(() => controller.abort(), 100);

Not aborting also sounds kinda weird, and yeah, aborting here does sound like another type of steal.

But then steal is kinda like an implicit global abort signal in my head, 🤔🤔 (I know it's not, at least AbortSignal does not release held locks)

saschanaz commented 3 years ago

On a second thought, devs can always use signal within the callback if they want immediate abort.

inexorabletash commented 3 years ago

Note that if you trace through Web IDL and ECMAScript and eventually bottom out in https://tc39.es/ecma262/#sec-promise-reject-functions, rejecting an already resolved promise is a no-op. So... I think the spec is technically already defining the desired behavior, but it could use a note explaining this.

saschanaz commented 3 years ago

I think this is about not-yet-settled promise, since granting a lock does not settle any promise before the callback ends.

inexorabletash commented 3 years ago

Ah, right, if request a lock has enqueued but not invoked the callback... that's not it...

Aside: Does "If ifAvailable is true and request is not grantable, then..." look backwards? Shouldn't that be "is grantable" ? (I haven't looked at the spec in a long time. no, that's what the null is for, sorry about the noise!

inexorabletash commented 3 years ago

Okay, reading a bit more (sorry, feeling rusty), I think the abort a request steps need to handle request not being in the queue, return false if it's not in the queue (true otherwise), and the signal steps only reject if the abort returns true. How does that sound?

inexorabletash commented 3 years ago

Also - since I've got some interested/talented people involved in this issue - I'd love to get another editor on the spec! If anyone has cycles and wants to start contributing, I'd appreciate the help!

saschanaz commented 3 years ago

Or we can remove the abort steps when granting a lock, which makes the steps not called at all when not needed.

saschanaz commented 3 years ago

Also - since I've got some interested/talented people involved in this issue - I'd love to get another editor on the spec! If anyone has cycles and wants to start contributing, I'd appreciate the help!

BTW, I think I'm interested to be an editor, but I'll probably wait until the spec moves to W3C before that.