Skip to content
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

In defence of waitUntil() #9

Closed
jakearchibald opened this issue Sep 18, 2017 · 13 comments
Closed

In defence of waitUntil() #9

jakearchibald opened this issue Sep 18, 2017 · 13 comments

Comments

@jakearchibald
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link

domenic commented Sep 22, 2017

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

@jakearchibald
Copy link
Contributor Author

I'm probably misremembering then, sorry!

@inexorabletash
Copy link
Member

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
Copy link

domenic commented Sep 26, 2017

I'm a little confused still.

Either:

  • 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); or
  • It doesn't fulfill until async_do_stuff() is done (seems to be how (3) works), thus allowing any aborts during the course of async_do_stuff() to be propagated.

@jakearchibald
Copy link
Contributor Author

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
Copy link
Member

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.

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

No branches or pull requests

4 participants