Lock-if-available vs. AbortController

[inexorabletash]

With an explicit timeout option, {timeout:0} can be implemented as "try to get the lock but don't wait if it's held", since the timeout is interpreted on the service side. With the AbortController model for timeouts this can't be done, since any timeout would be a race.

Instead we'd need to introduce a specific option, e.g. {tryLock:true}

[inexorabletash]

leaning towards ifAvailable as a name. bikeshed away!

[inexorabletash]

FWIW, Java calls this tryLock, C# calls this tryEnter.

I suppose we can just use try with modern JS; it's only a reserved word in context.

[inexorabletash]

Hrm... should this reject, or resolve with null?

Rejection matches the abort case, but this violates the spirit of exceptions as being the exceptional case whereas "ifAvailable" failing is not exceptional.

[inexorabletash]

With rejection:

try {
  await requestLock(scope, async lock => {
   // have the lock
  }, {ifAvailable: true});
} except (ex) {
  if (ex.name === 'AbortError') {
    // lock was not available
  }
}

With resolve:

await requestLock(scope, async lock => {
  if (lock) {
    // have the lock
  } else {
    // lock was not available
  }
}, {ifAvailable: true});

I suppose the latter is more idiomatic, it's just odd because you generally don't need to look at the lock in this callback approach; if your callback is called then you have it.

@jakearchibald @domenic - thoughts ?

[inexorabletash]

Never mind - I've convinced myself. Name suggestions appreciated, though.

[inexorabletash]

Closing for now, people can complain about the name elsewhere

[domenic]

What was the conclusion? (I think it should be resolve with null if the name is ifAvailable.)

[inexorabletash]

Resolve with null.

[inexorabletash]

Once I started writing tests it seemed clear. It's also completely opt-in so unless you're using the option the lock will never be null so simple use remains simple.

[ralphch0]

Isn't it dangerous to resolve to null? I'm using the standard API, then realize that I don't want to block, so I add the ifAvailable option, and now critical code is running when it shouldn't. I just caused data corruption in my app in subtle edge conditions that might not show up in testing. My callback might not even have had a "lock" parameter, so I wouldn't even know that the parameter is there, much less that it can be null. Also it's weird that abort using the signal and abort from this option have to be handled differently.

I would suggest adding a 'abortIfUnavailable' option which would abort in the regular fashion (by rejecting). I don't think it's abusing Errors here since it still should be a rare situation in the simple use cases (locks held for a short period of time).

[inexorabletash]

Reopening re: @ralphch0's comment. Feedback from others?

[ralphch0]

Any further thoughts here? Personally, I would prioritize safe defaults over design purism.

[inexorabletash]

@domenic - care to weigh in?

[domenic]

I feel a bit out of my depth, as I don't really understand @ralphch0's argument. I thought this was opt-in, not a matter of safe vs. unsafe defaults.

My mental model is not that you abort if it's not available, but that you're asking "give me a lock if it's available", and it's not available, so you get no lock (i.e. null).

[ralphch0]

I'll try to boil down my position to two points:

1. This feature is a special case of aborting

Basically if you wanted to acquire the lock and wait for X seconds, you can use the AbortController to abort the acquisition request after X seconds. But when X = 0 (you don't want to wait at all), then it's trickier to implement because you would need to somehow account for the inherit browser delay in fulfilling the request. So this feature was suggested as an early exit, to try to acquire the lock, but incur only the strict minimum cost in waiting.

2. Turning on this feature (opting in) is risky

This is because it introduces a new state (lock == null) that the dev may not expect.
Here's an example:
navigator.locks.acquire('mylock', async () => {
// Do something sensitive here.
});

Note that the lock parameter does not need to be declared, so it's hard to know that it even exists. Now the dev notices that the acquisition can sometimes cause delays if other pages don't release the lock quickly. So the dev adds the ifAvailable feature, to make sure no delay is incurred.

navigator.locks.acquire('mylock', {ifAvailable: true}, async () => {
// Do something sensitive here.
});

Now this will work most of the time (hard to catch bug via testing), until it happens that the acquisition is happening while the lock is held by another page. When that happens the sensitive code will run when it's not supposed to, likely causing data loss. If this rejected instead, it will cause a page error which is safer.

[inexorabletash]

We shipped w/ callback-with-null rather than a rejection and the world has thus far not ended, so closing this out. Can revisit pending further data.