Incorporate TAG feedback

[inexorabletash]

Re: w3ctag/design-reviews#217

Suggestion is to rework the acquisition API along the lines of:

const lock = new WebLock('lock-name');
lock.on('acquire', e => {
  e.waitUntil(doSomethingAsync());
});

(Where on is a proposed newfangled way of attaching event listeners, I think.)

Also:

• Some concern about the name 'lock' due to conflict with keyboard locks, wake locks, etc.
• Unclear where query() would end up here.

cc: @jakearchibald @domenic @pwnall @ralphch0

[domenic]

I didn't find this feedback compelling. Making a web lock into an event target, and somehow making adding event listeners cause the side effect of lock acquisition, seems like really bad design. And all that just so you could awkwardly pun on the waitUntil name? I don't see the benefit.

If using a different word helps deter such attempts at "aligning" across other things that have the word "lock" in them, maybe that's worthwhile. But I think it's ok to use multiple meanings for the same word in the web platform.

[ralphch0]

+1 to domenic@'s comment - This API shape feels forced to me.

Is the lock requested when it's listened on? That seems weird. If not, then just creating an object acquires a common resource? - that seems dangerous.

This also opens the possibility of forgetting to call on(), calling it multiple times, or calling it in an async fashion in a way that misses the event.

[inexorabletash]

My interpretation is that the lock would be requested when the WebLock instance is created. You'd need to call on() immediately or you might miss it (ralph's point), calling it later would be a no-op. Which does mean we'd be introducing an event that would fire exactly once that you must listen to, and really only to get the waitUntil() hookup (domenic's point).

[domenic]

I also think the existing web locks pattern is way better than waitUntil, which often awkwardly forces immediately-invoked async function expressions (as @jakearchibald has demonstrated a few times).

[inexorabletash]

Yeah, without async do, waitUntil() is ugly.

[jakearchibald]

Agree with @domenic's assessment. Specifically:

Forgetting to use an async callback is a serious footgun.

I don't get it. The JS engine will throw pretty hard if you use await inside something that isn't an async function. Am I missing something?

The waitUntil proposal seems like a misuse of the existing definition of that method in the serviceworker context, since the return value from requestLock is not an event.

Multiple interfaces on the web have a .type and the world still turns (eg, events and input elements). I think it's fine for waitUntil to be used on other objects, as long as it takes a promise and is used to express the lifetime of an operation. But, I still think the callback pattern is better.

We conceived of a fourth option, using extendable events in a manner more consistent with their use in service workers:

In addition to what @domenic said, using an event to express a once-only 'ready' seems like an anti-pattern. That's what we have promises for right? Also, what happens if you don't call waitUntil? Is it auto-released? If so, when? After dispatching the callback?

It's kinda surprising that they'd suggest our API was a footgun, then recommend an API where it'd be easy to do:

const lock = new WebLock('lock-name');
lock.addEventListener('acquire', async () => {
  await somethingAsync();
});

…which really doesn't do what you might expect at first glance. Whereas this would work fine:

navigator.locks.acquire('lock-name', async () => {
  await somethingAsync();
});

I kinda get their point that await … acquire might seem weird, since it settles once the lock is released, not acquired. Maybe a name that suggests it resolves once the whole 'job' is done would be better.

[inexorabletash]

Per offline discussions with @pwnall, considering changing acquire() to request(), which is more in line with other locking APIs.

[inexorabletash]

re: waitUntil - are we comfortable using the async-callback-based API pattern for other APIs in the future? e.g. imagine a future Cache API:

const cache = await self.caches.open('stuff');
cache.transact(async batch => {
  const [a, b] = await Promise.all([fetch('a'), fetch('b')]);
  const c = await frob(a, b);
  await cache.put('a+b', c);
});

Or Indexed DB "3.0":

db.transaction('stuff', 'readwrite', async tx => {
  const stuff = tx.objectStore('stuff');
  const obj = await stuff.get('key');
  obj.body = await (await fetch(obj.url)).text();
  await stuff.put(obj, 'key');
});

[domenic]

That makes a lot of sense to me.

[jakearchibald]

@inexorabletash that's actually a pretty neat way to get promises into IDB.

[js-choi]

@inexorabletash: Should an issue be opened in inexorabletrash/indexeddb-promises regarding the async-callback idea?

[inexorabletash]

done: inexorabletash/indexeddb-promises#14

[ralphch0]

It should be noted that this API pattern is already used in Promises themselves. Namely the then() method takes a callback that returns a promise. So we're not necessarily venturing into new territories here with the "async-callback" style.

[dbaron]

So I think it's probably worth clarifying adding to some of the feedback in @triblondon's w3ctag/design-reviews#217 (comment) . It's very hard to debug locking-related bugs where the bug is that something that should have happened while the lock was held instead happens outside the lock. This is because actually hitting the race condition that the lock is preventing is often quite difficult to reproduce. Thus locking APIs should make it hard to do this accidentally or to miss that it happens.

Perhaps the exact footgun case that Andrew was thinking about isn't a problem, but a similar footgun case would, think, be that when you meant to write:

navigator.locks.request('my_resource', async lock => {
   // The lock has been acquired.
   await do_something(); // do a thing while the lock is held
   // Now the lock will be released.
});

you would instead forget the await keyword:

navigator.locks.request('my_resource', async lock => {
   // The lock has been acquired.
   do_something(); // This returns a promise, and the work
                   // will actually happen after the lock is released.
   // Now the lock will be released right after the promise
   // is created, well before the next microtask.
});

This has the failure mode that the code looks pretty close to correct, but it's open to a race condition.

Maybe the missing await is completely obvious to proficient javascript developers... but I tend to suspect it might not be, since unlike other cases where await is missing, it won't lead to obvious and reproducable bugs.

---

Edit (a few minutes later): On the other hand, maybe there isn't a reasonable way to avoid this problem given the presence of promise-based APIs.

[ralphch0]

Yes (to your follow up edit). This seems like a general Async code pitfall that I would expect developers working with async/await or promises to be familiar with.

for example:
async function doWork() {
doAsync(); // Forgot to call await
}

await doWork();
location.reload(); // Oops async work interrupted!

[triblondon]

In addition to @dbaron's comment above, and noting @jakearchibald's comment that the engine will throw on an await in a non-async function, my concern was non-async callbacks that also don't await, but invoke async operations in other ways, for example by invoking fetch and not returning the promise. The result, however, is the same: your code works, and isn't being locked, and that bug is not as obvious as forgetting to await would normally be. I forget awaits or returns all the time, and the resulting bugs are normally immediately obvious.

Is use of this API with a synchronous callback considered a viable use case? If not, could it throw if the callback doesn't return a promise?

Thanks for considering a rename! To me, 'request' still suffers to some extent from the same problem of implying that you get a result on acquiring the lock rather than on completion of the task. I was wary of suggesting a name though as everything I thought of was pants.

Thanks for considering our imagined alternative API. It seems totally fair to discount it.

[travisleithead]

Just a small note on the original idea discussed near the top of this issue... it definitely needs another method to trigger the locking bit. This then makes for a re-usable lock object. The following is just another take on the current design, but designed around events and waitUntil. Lock is released at the conclusion of all handlers' execution. (Similar to what @dbaron said above, if you forget waitUntil you can have problems using asynchronous methods in the event handler.)

const lock = new WebLock('lock-name');
lock.addEventListener('beforeunlock', e => {
  e.waitUntil(doSomethingAsync());
});
lock.request(); // Handle multiple calls? 

...but I'd like to know when this request() is done, so lock.request() should return a Promise, right?

But then the API starts to look much more like what you started with, just more complicated.

I think I like the original proposed API better after going through this exercise 😊

[triblondon]

We see no blocking issues here. In general, the feature is useful and the API is straightforward, so we wish you luck with the implementation! Thanks for choosing TAG for this design review. You earned 2 TAGpoints which are redeemable against future design reviews.

[inexorabletash]

I think I like the original proposed API better after going through this exercise

Thanks for taking the journey! I think we all started off independently with somewhat more idiomatic ideas for what the API should look like, and then unexpectedly converged on what we have now. It's reassuring to hear that others arrived at the same place.

You earned 2 TAGpoints which are redeemable against future design reviews.

Huzzah!