Question about design of API...

[marcoscaceres]

Just wondering, how come you went with taking a function as the second argument instead of having the lock returned in the promise? That is, why?

async function get_lock_then_write() {
  await navigator.locks.request('resource', async lock => {
    await async_write_func();
  });
}

Instead of just?:

async function get_lock_then_write() {
  const lock = await navigator.locks.request('resource');
  await async_write_func();
  // do whatever to release the lock later... 
}

If you stick with the current design, the request() overload methods of LockManager seem a bit odd (having an optional argument in the middle of a method is not great)... why not just do:

  Promise<any> request(DOMString name,
                       LockGrantedCallback callback,
                       optional LockOptions options);

[marcoscaceres]

(fixed couple of typos above)

[inexorabletash]

how come you went with taking a function as the second argument instead of having the lock returned in the promise ... ?

We started with such a design where the lock object was returned and required an explicit release, and evolved from there as footguns were noted. Best captured in the discussion at: #9

having an optional argument in the middle of a method is not great

This is explcitly covered in the explainer in the FAQ, search for "Why is the options argument not the last argument?"

Similarly, that's how the proposal started. Discussed at #19

[marcoscaceres]

Ok, but the optional argument in the middle of the overload seems to be in the proto-spec. Domenic pointed that out too. It seems it didn’t get fixed.

[marcoscaceres]

In #9, I don’t really understand how it leads to the current design with respect to having a promise returned that resolves to “any”, plus taking the callback as the second argument that eventually gets a lock. That still seems super weird to me (it might be fine, it’s just odd).

I guess my core question is: why does the returned promise resolve with “any”? What is that used for:

// what do I do with aThing?
const aThing = await navigator.locks.request('resource', f);

[inexorabletash]

Dominic advocated for options in the middle. That was how the discussion concluded.

The outer promise resolves with the result of the callback. Could also resolve with void

[inexorabletash]

Oh, "optional" on the overload in the proto-spec is wrong. Position is correct, though.

[inexorabletash]

I kept reading "optional argument" as "options argument". My bad. Will fix when I have a bigger screen and a real keyboard.

[inexorabletash]

Apologies for the delay...

I guess my core question is: why does the returned promise resolve with “any”?

const result = await navigator.locks.request('resource', async lock => {
   let ok = await do_something();
   if (ok) {
     ok = await do_something_else();
   }
   return ok;
});

When the result promise resolves (1) the callback function has completed, (2) the lock has been released and (3) it has the value of ok

We could make request() resolve to Promise<void> instead, but it seemed like plumbing the return value of the callback out made sense, rather than dropping it on the floor. If it resolved to Promise<void>, the return value could be smuggled out with a local variable.

In addition, if the callback throws then the result Promise is a rejection, e.g. you can code like this:

try {
  await navigator.locks.request('resource', async lock => {
    if (!await do_something()) throw Error();
    if (!await do_something_else()) throw Error();
  });
} catch (ex) { ... }

or if do_something() and friend throw on error, just:

try {
  await navigator.locks.request('resource', async lock => {
    do_something();
    do_something_else();
  });
} catch (ex) { ... }

[marcoscaceres]

I don't know... that still seems weird to me... like the API is overstepping. Consider, if I want the ok value, then I can just add a promise:

function getTheLock() {
  return new Promise(resolve => {
    navigator.locks.request("resource", async lock => {
      let ok = await do_something();
      if (ok) {
        ok = await do_something_else();
      }
      resolve(ok);
    });
  });
}

But even then I hold the problem is the current design doesn't feel idiomatically. Specially when this is cleaner (and includes the error handling):

async function getTheLock(type) {
  const lock = await navigator.locks.request(type);
  let ok = await do_something();
  if (ok) {
    ok = await do_something_else();
  } else {
    throw new Error();
  }
  return ok;
}

You get all the same benefits, and way less nesting, no?

[ralphch0]

In the first snippet, you still need to handle the rejection case. Also, by calling the resolve method within the callback, you would be resolving the outer promise before the lock is released.

Isn't propagating the resolve/reject values very similar to the then() method of promises? Seems like a natural pattern when we want a method to control an async operation: start it, catch errors, and return the result of the operation.

[domenic]

Your second snippet also never releases the lock, even on the success path.

[marcoscaceres]

Your second snippet also never releases the lock, even on the success path.

Sure, but wouldn’t that just be:

return {ok, lock};

Then the caller can release the lock? I’m obviously missing something here which is non-obvious... if I’m not getting it, it’s pretty likely other developers will misunderstand it too, which worries me.

[domenic]

You shouldn't push the responsibility of releasing the lock on to the caller. It's an implementation detail of your function that uses the lock, in most cases. The caller generally doesn't care that the resource your function is manipulating is locked at all.

Also your return version will permanently fail to release the lock if any of the function's steps throw an exception.

[marcoscaceres]

Ah, ok. Think I get it now (and also why it’s a callback). Thanks @domenic!

[inexorabletash]

@marcoscaceres - any suggestions for how we do a better job in the explainer (readme) of explaining this?

[marcoscaceres]

IMHO, in the Abstract, presenting the purpose of the API with a concrete example would be really helpful (specially before the reader gets to the Background section... more on this below!). What @domenic said in #38 (comment) gave me the "Aha!" moment: basically, this lets developers write a kinda micro task/little tasklet, where, the developer:

1. requests the lock.
2. writes a custom implementation of work to do while holding the lock.
3. Optionally, returns a value/data that made the developer hold the lock in the first place.
4. The lock is auto-released by the user agent when the task completes.

And maybe briefly mention: if you get in trouble, the API provides an escape hatch to protect against deadlocks (you don't need to show this in the abstract... maybe just mention it).

The main thing is to have that clear before the user gets into the Background section... and here is why: I'm probably not alone here, but the "Background" section's mention of IDB is immediately off-putting; it puts the reader in the defensive.

I know no one here is responsible for the design of IDB - but it's burnt, and is so hated, by so many of us, that the mere mention of it there already had me thinking "argh, I'm gonna have to hack around this API - because anything remotely related to IDB is going to make me flip tables".

I'm not suggesting you remove the Background section, just that the Abstract put the reader at ease that this is not IDB all over again. If the user is already in a frame of mind that, "oh wow, this is really useful, easy, and pretty cool!" - then mentioning IDB should be fine, because the statements there are factual and indeed important background information.

Hopefully that helps! Sorry I was a bit thick in "getting it" - but once it's explained, the design makes total sense. I'm now kinda excited that the web will have this primitive, and I'm sure there will be lots of useful places to use it!

[marcoscaceres]

Any plans to move this to WICG or WHATWG? Also, please let me know if you want more Mozilla folks to look at this and maybe take a position on it... personally speaking, I'd say it's definitely "worth prototyping".

[inexorabletash]

I proposed this as a WICG topic. Waiting for a +1 from another interested member. Hint hint....

https://discourse.wicg.io/t/application-defined-locks/2581

[inexorabletash]

And "yes please" to more eyeballs from Mozilla.

[inexorabletash]

Also, thanks for all the suggestions on the abstract. I'll work those in. I have to put a blog post together as well, and it's a very useful structure.

[marcoscaceres]

Ok, request sent mozilla/standards-positions#64

You might want to subscribe to that. If no-one responds, I can reach out to a few people.