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

Web Locks API #217

Closed
inexorabletash opened this Issue Nov 10, 2017 · 10 comments

Comments

Projects
None yet
6 participants
@inexorabletash

inexorabletash commented Nov 10, 2017

Hello TAG!

I'm requesting a TAG review of:

Further details (optional):

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our Github repo for each point of feedback

@plinss plinss added this to the tag-telcon-2018-01-02 milestone Dec 5, 2017

@torgo torgo modified the milestones: tag-telcon-2018-01-02, tag-telcon-2018-01-23 Jan 16, 2018

@torgo

This comment has been minimized.

Member

torgo commented Jan 16, 2018

@triblondon to write some notes here.

@inexorabletash

This comment has been minimized.

inexorabletash commented Jan 16, 2018

@triblondon

This comment has been minimized.

triblondon commented Jan 19, 2018

This seems like a cool feature. I have some questions:

I'm wondering why the callback based API that also seems to incorporate promises was chosen. Explicit release is documented as an alternate API that was considered but there isn't much on why you chose the callback API instead. I'd instinctively want this (explicit release):

const lock = await navigator.locks.acquire('my-lock');
await doSomethingAsync();
await lock.release();

It seems odd to me that the promise returned by acquire() actually doesn't resolve when the lock is acquired but rather when it is released. I also wonder if people will often forget to await on the async operation inside the callback:

// Appears to work but actually releases lock before async operation starts
await navigator.locks.acquire('my-lock', () => doSomethingAsync());

The impact of this design decision seems to be that on average locks will err on the side of releasing too early rather than too late. Given that they are origin-scoped, and therefore late-releasing locks can only impact the author's own applications, I would think we could provide the more explicit version.

I can imagine scenarios in which I might want to hold multiple locks, but in unconnected non-nested operations, which happen to overlap. That seems a lot easier to reason about with explicit locks.

If we are to have the callback shaped API, which resolves upon release, perhaps acquire is the wrong word - I'd consider wrap, enclose, lock or lockFor.

Have you considered a built in timeout or do we consider AbortController to be the right way to expose the ability to do that now?

@inexorabletash

This comment has been minimized.

inexorabletash commented Jan 19, 2018

I'm wondering why the callback based API that also seems to incorporate promises was chosen.

The best places that captures the debate is at: WICG/web-locks#9 - the design started with explicit release, but evolved away in response to feedback from @domenic, @jakearchibald and others.

The callback+promises approach sounds weird in the abstract and I was skeptical, but the resulting code ends up looking surprisingly clean. Agreed that you need to remember the await and async sprinkles. But as noted in that bug thread, writing correct code with other forms is convoluted and the language doesn't help you get it right. So... a trade off that (we felt) is somewhat closer closer to a "pit of success".

It does make the API awkward when needing explicit lifetime control; you end up recreating the explicit API with explicit promise hi-jinx. (e.g. this shows up in tests quite a bit)

Have you considered a built in timeout or do we consider AbortController to be the right way to expose the ability to do that now?

Definitely considered — it was an explicit option in initial API sketches. Having it built in allows for the lock manager to run the timer, which is an advantage if e.g. a page is janking. But AbortController appears to be a generalization, so exposing just that for now. We look forward to developer feedback once it's implemented.

@plinss plinss modified the milestones: tag-telcon-2018-01-23, tag-f2f-london-2018-01-31 Jan 23, 2018

@triblondon

This comment has been minimized.

triblondon commented Feb 1, 2018

Worth noting also that this callback mechanism makes web locks inconsistent with all the other numerous lock related APIs. We've established four distinct patterns so far for acquiring a lock on something. Do we really need a fifth?

@domenic

This comment has been minimized.

Member

domenic commented Feb 1, 2018

I think web locks are very different from the type of locks listed in those comments; it's unfortunate that sometimes we use the same English word for two separate concepts, but I think that's what's happening here. In particular those are about locking other people away from using a resource (keyboard, pointer) or locking something in a certain position (wake, orientation). Web locks are about the CS concept of a lock/mutex.

@dbaron

This comment has been minimized.

Member

dbaron commented Feb 2, 2018

On the flip side, this actually seems a bit different from a lock/mutex, given that it's promise-based and thus doesn't block the execution stack (except for async functions). (That also, if I'm understanding correctly, greatly reduces the consequences of deadlocks, although the locks certainly can still be deadlocked.) @travisleithead wondered if perhaps this should be named in terms of transactions rather than locks.

I also think it's still similar enough in concept that the API surface shouldn't be unnecessarily different.

@triblondon

This comment has been minimized.

triblondon commented Feb 2, 2018

The TAG just discussed this at our London F2F meeting and concluded that:

  • The callback approach has some problems in its current form:
    • Forgetting to use an async callback is a serious footgun. It will appear to work and you may not discover the bug for some time. Throwing an error if the callback doesn't return a promise might mitigate this.
    • The term acquire is inappropriate to what the method actually does. It looks more like a transaction. A rename could mitigate this.
  • We also looked at the two alternative API proposals
    • 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.
    • It's confusing that the waitUntil method doesn't block, meaning that the scope in which the lock is defined becomes garbage collectable before the lock releases
    • Explicit release may have a higher likelihood of accidental long lived locks and mistakes in error handling code.
  • We conceived of a fourth option, using extendable events in a manner more consistent with their use in service workers:
const lock = new WebLock('lock-name');
lock.on('acquire', e => {
  e.waitUntil(doSomethingAsync());
});
  • We recognise that Web Locks are not the same kind of lock as keyboard locks and wake locks, but they share the name, so the API shape being different is unexpected. We see that this feature was previously called requestFlag and maybe that or another name should be considered.
@inexorabletash

This comment has been minimized.

inexorabletash commented Feb 2, 2018

Thanks folks! I'll chew this over and work through it with others involved in the design.

I really appreciate the attention.

@triblondon

This comment has been minimized.

triblondon commented Apr 7, 2018

We've posted further feedback on their issue WICG/web-locks#35, and that about wraps this up.

@triblondon triblondon closed this Apr 7, 2018

@triblondon triblondon removed their assignment Apr 7, 2018

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