w3c / web-locks

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

In defence of waitUntil() #9

Closed jakearchibald closed 7 years ago

jakearchibald commented 7 years ago
async function get_lock_then_write() {
  const flag = await requestFlag('resource', 'exclusive');
  flag.waitUntil(async_write_func());
}

// vs

async function get_lock_then_write() {
  const flag = await requestFlag('resource', 'exclusive');
  await async_write_func();
  flag.release();
}

Although these look somewhat equivalent, they're different if async_write_func rejects. The waitUntil example will see the promise settle and release the lock, whereas the .release example will never be released.

To avoid forever-locking, you'd have to do:

async function get_lock_then_write() {
  const flag = await requestFlag('resource', 'exclusive');
  try {
    await async_write_func();
  }
  finally {
    flag.release();
  }
}

Question is, is this enough of a footgun to justify waitUntil?

inexorabletash commented 7 years ago

Yep - it's a good question.

Another possible API shape, based on a suggestion for IDB+Promises by @dominiccooney:

requestFlag('resource', 'exclusive', async flag => {
  // ... do stuff here
});

This eliminates an explicit waitUntil() and encourages writing code that holds the flag for a bounded period of time; a throw within the async callback would (usually) release.

jakearchibald commented 7 years ago

Yeah, I like that design. I think we discussed it for this API and @domenic wasn't a fan. I could be misremembering though.

philipwalton commented 7 years ago
requestFlag('resource', 'exclusive', async flag => {
  // ... do stuff here
});

I like this best as well.

I'm concerned that using the name waitUntil will confuse people into thinking the object is an event (though maybe that's OK if it become a commonly used method name). Either way, I like the idea of forcing the release code along with the request.

inexorabletash commented 7 years ago

Re: async callback

How would timeouts (via AbortController/AbortSignal) work in this case - is the callback simply never invoked, since you can listen on signal.onabort if needed?

Is there any return value from requestFlag()?

inexorabletash commented 7 years ago

Also, if we get 'async do' in ECMAScript you can write:

const flag = await requestFlag(scope, mode);
flag.waitUntil(async do {
  // do stuff here...
});

... which looks remarkably similar to the "async callback" approach.

inexorabletash commented 7 years ago

Writing "explicit release" in terms of "async callback" requires this, I think:

function requestExplicitFlag(scope, mode, ...rest) {
  return new Promise(resolve => {
    requestFlag(scope, mode, flag => {
      // p waits until flag.release() is called
      const p = new Promise(r => { flag.release = r; }); 
      resolve(flag);
      return p;
    }, ...rest);
  });
}
jakearchibald commented 7 years ago

@inexorabletash

How would timeouts (via AbortController/AbortSignal) work in this case … Is there any return value from requestFlag()?

requestFlag could return a promise that resolves once the flag has been granted, and resolves with whatever the callback returns. That's handy for:

await requestFlag('resource', 'exclusive', async flag => {
  // ... do stuff here
});
console.log('Task complete!');

But it also means it can reject with AbortError.

domenic commented 7 years ago

If I had a preference, I can't remember why. It is interesting that the async callback version would maybe allow signaling aborts...

jakearchibald commented 7 years ago

I'm probably misremembering then, sorry!

inexorabletash commented 7 years ago

I think they'd all allow signalling aborts equally?

const controller = new AbortController();
try {
  // 1
  const flag = await requestFlag(scope, mode, {signal:controller.signal});
  flag.waitUntil( async_do_stuff() );

  // 2
  const flag = await requestFlag(scope, mode, {signal:controller.signal});
  try { await async_do_stuff(); } finally { flag.release(); }

  // 3
  await requestFlag(scope, mode, async flag => {
     await async_do_stuff();
  }, {signal:controller.signal});

} catch (ex) {
  if (ex.name === 'AbortError') { ... }
}
domenic commented 7 years ago

I'm a little confused still.

Either:

jakearchibald commented 7 years ago

the promise returned by requestFlag has fulfilled, giving you a flag, and thus can never reject with an "AbortError" DOMException (seems to be how (1) and (2) work);

fwiw, you can abort a fetch() after the initial promise resolves. I'm not sure how that would be expressed in (1), but in (2) waitUntil could resolve once the flag is released. Maybe a bit of a new thing for waitUntil to do though.

But anyway, I much prefer (3).

inexorabletash commented 7 years ago

Hrm, I think I was interpreting "allow signalling aborts" to mean: there's a promise you can watch that will reject with AbortError if the AbortSignal fires.

Another interpretation is: there's a promise you can watch that rejects if the operation done while holding the flag rejects.

In that case: yes, in (1) we could do that by having waitUntil return a promise as Jake suggests (I explored that over in https://github.com/inexorabletash/indexeddb-promises), (2) doesn't really have that, and it falls out naturally from (3).

In the IDL sketch I also have Lock.prototype.released which is a promise resolving after the flag is released. In (1) it would be the equivalent of Promise.all() over the promises passed to waitUntil() (so, rejecting if one rejects). In (2) it would always resolve. In (3) it would be the same as the promise returned from the requestLock() call so could be eliminated.