async waitUntil() does not allow extension on final promise resolution?

[wanderview]

I feel like we discussed this case at one point, but can't find where:

addEventListener('fetch', evt => {
  // keep alive the SW based on some promise
  let resolveOuterPromise;
  let p = new Promise(resolve => resolveOuterPromise = resolve);
  evt.waitUntil(p);

  // Code can be defined to run on the final keep alive promise resolution that
  // then keeps the SW alive again.
  p.then(_ => {
    // I think the spec says this should throw, but I thought we agreed this should work.
    evt.waitUntil(fetch(evt.request));
  });

  // promise resolves at some point...
  setTimeout(_ => resolveOuterPromise(), 100);
});

I don't think the spec allows this as currently written. Step 4 here:

https://w3c.github.io/ServiceWorker/#extend-service-worker-lifetime-algorithm

Unsets the flag allowing extension immediately after the current set of extension promises settles in step 2. I think this blocks the addition of new extension promises?

Or is there something which guarantees other .then() handlers will run between step 2 and 3 here?

I think we should either explicitly have a microtask to run step 3/4 or run add a comment about some hidden invariant which results in this outcome.

[wanderview]

FWIW, we have implemented this behavior in FF and written a WPT test verifying it. See:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/extendable-event-async-waituntil.js#27

And:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/extendable-event-async-waituntil.js#33

[jungkees]

I don't think the spec allows this as currently written.

Yes, that's right. I think we should add all the derived promises called by Promise.prototype.then of the promises in the extend lifetime promises list. I'll try to spec it in an implementable way.

[stefhak]

Related(?):
I've been trying to use .waitUntil() with the push event to make sure things get done before the SW stops, basically:

self.addEventListener('push', evt => {
    evt.waitUntil(handlePush(evt.data));
    //or alternatively just: handlePush(evt.data);
}

where handlePush returns a promise that is resolved when done. I could not determine any prolonged run time with evt.waitUntil.
Is it specified anywhere what events should change the run time of the SW if used with waitUntil? It is clear that for install and activate things change with waitUntil, and there is something said about fetches, but what is the story beyond that?

[jungkees]

This is not related to the issue posed in the OP. I'm surprised it didn't work. Both Blink and Gecko implemented it. On which environment did you tried it?

Is it specified anywhere what events should change the run time of the SW if used with waitUntil?

The ExtendableEvent interface provides waitUntil() method, and APIs extending SW are supposed to extend ExtendableEvent for their own events: https://w3c.github.io/ServiceWorker/#extensibility. Examples are:

• PushEvent: https://w3c.github.io/push-api/#dom-pushevent
• NotificationEvent: https://notifications.spec.whatwg.org/#notificationevent
• SyncEvent: https://wicg.github.io/BackgroundSync/spec/#syncevent

[stefhak]

@jungkees thanks for the info - and sorry for posting stuff that is not related to this issue. Let me know if I should move this discussion somewhere else.
Anyway, the tests I did was on an Android tablet, and I did test mainly with Chrome (but also with FF Nightly). I must admit my tests were not very thorough - I have a use case where a WebPush is used to have the SW to fetch and cache data (that the origin server predicts the user will soon want). My problem is that the SW stops mid way through this, and adding "waitUntil" in the way I described still did not make it finalize the task but stopped roughly at the same point, and after noting that I did not dig further (so I can't really say if it continued a slightly longer or not).

To me this seems under specified. Since my use case is with the PushEvent I looked into that spec, and "waitUntil" is not mentioned there.

[jungkees]

Did you use a code pattern as described in the OP? Like:

evt.waitUntil(p);

// Code can be defined to run on the final keep alive promise resolution that
// then keeps the SW alive again.
p.then(_ => {
  // I think the spec says this should throw, but I thought we agreed this should work.
  evt.waitUntil(fetch(evt.request));
});

Or the promise returned from handlePush() in your example - evt.waitUntil(handlePush(evt.data)); - just settles after all the things are done? I think you'd better clarify the case by providing more detailed info as you encountered the problem described above: "SW stops mid way through this, and adding "waitUntil" in the way I described still did not make it finalize the task but stopped roughly at the same point".

[jungkees]

I'm working on this in #1049, and had a feedback that if this design is tight: #1049 (comment). Also I sort of touched the ECMAScript internal slots to spec this, and that seems hacky. So, I'd like to understand more of how implementations actually did and will do to find a right specification devices and methods.

/cc @wanderview @mattto @domenic @annevk @jakearchibald @slightlyoff

[domenic]

I think there might be confusion here, looking at some of the examples. Given the following:

e.waitUntil(p);

const q = p.then(() => {
  e.waitUntil(r);
});

My impression is that the OP wants to wait until p and r, i.e. on the two different things passed to waitUntil. This seems very reasonable and easy to accomplish (using only the public API). It also matches https://github.com/jakearchibald/async-waituntil-polyfill/.

However, the implementation in #1049 is doing something completely different: it's waiting on p and q. Waiting on q is not very reasonable, because q is never passed to waitUntil, and can only be discovered by crawling the promise graph.

---

This can be further exemplified by changing the above example a bit:

e.waitUntil(p);

const q = p.then(() => {
  e.waitUntil(r);

  return new Promise(() => {}); // never settles, thus q never settles
});

The implementation in #1049, since it attempts to wait on q (which never settles), will wait forever. Whereas the implementation in https://github.com/jakearchibald/async-waituntil-polyfill/ will only wait on p and r.

[jungkees]

My impression is that the OP wants to wait until q and r

I believe you meant p and r here. I think this design is reasonable.

However, the implementation in #1049 is doing something completely different: it's waiting on p and q. Waiting on q is not very reasonable, because q is never passed to waitUntil, and can only be discovered by crawling the promise graph.

Right. I added q to the pending list, and that's why I ended up poking the internal slots. I thought we should guarantee all the then callbacks in the given promise's promise chain. E.g.

e.waitUntil(p);

p.then(doSomething).then(() => { e.waitUntil(r); });

Now the more I think about it, the more convinced that I have misunderstood the original issue.

[domenic]

I believe you meant p and r here

Thanks, edited!

[smaug----]

#1050 is related, I think

[jungkees]

Closed by 6c1f3fe.

[smaug----]

Whaat, how did this stuff start to use tasks. That makes it very random how long one can still add waitUntil promises to the event object. There can be many other pending tasks already and all those might get run before the queued task.

[wanderview]

To adjust my original example code I think we agreed to this:

addEventListener('fetch', evt => {
  // keep alive the SW based on some promise
  let resolveOuterPromise;
  let p = new Promise(resolve => resolveOuterPromise = resolve);
  evt.waitUntil(p);

  // Code can be defined to run on the final keep alive promise resolution that
  // then keeps the SW alive again.
  p.then(_ => {
    // But if there is an extra microtask turn here...
    Promise.resolve().then(_ => {
      // Then this should throw.
      evt.waitUntil(fetch(evt.request));
    });
  });

  // promise resolves at some point...
  setTimeout(_ => resolveOuterPromise(), 100);
});

So I think queuing a full up task is wrong.

[jungkees]

e.waitUntil(p) // p.then (i.e., upon settlement step in the spec)
p.then(_ => e.waitUntil(q));
p.then(_ => e.waitUntil(r));

Should the waitUntil(r) throw here? This is the same microtask order as in your example. Where should the microtask for the "upon settlement of p step in the waitUntil method" queued? Between q and r? If it's queued before q, even q will throw. If it's queued between q and r, I think the behavior looks odd. (Will devs expect this?)

I think we should either use a normal task as spec'd or consider the design in #1049 (comment).

[wanderview]

I don't think either q or r waitUntil calls in your example shoudl throw. However, s here should throw:

e.waitUntil(p) // p.then (i.e., upon settlement step in the spec)

// do not throw
p.then(_ => e.waitUntil(q));
p.then(_ => e.waitUntil(r));

// throws
p.then(_ => Promise.resolve().then(_ => e.waitUntil(s)));

[jungkees]

That behavior means,

The "count decrease" job should be queued as a microtask exactly in the position marked $ below:
| resolve p [then_1: q] [then_2: r] [then_3: chain] $ [then_4: s] |

(assuming "||" is a task boundary, "[]" is a microtask.)

even though p.[[PromiseFulfillReactions]] contains the reactions in this order: ["count decrease", q, r, the chain for s].

Also it seems we'll need some customization to Promise hooks (https://html.spec.whatwg.org/#enqueuejob(queuename,-job,-arguments) or somewhere) to record and detect the depth of the then callbacks for the queued microtasks?

[annevk]

We should not add new hooks just for this feature. Either you throw when you try to add q, you queue a microtask upon fulfillment and decrease from that microtask, or you go with the task-based option.

[jungkees]

you queue a microtask upon fulfillment and decrease from that microtask

I think this would satisfy Ben's mental model. This would even work for all sequences:

e.waitUntil(p);
p.then(_ => e.waitUntil(q));
p.then(_ => e.waitUntil(r));

p.then(_ => e.waitUntil(q));
e.waitUntil(p);
p.then(_ => e.waitUntil(r));

p.then(_ => e.waitUntil(q));
p.then(_ => e.waitUntil(r));
e.waitUntil(p);

I'm happy with this solution.

[jungkees]

@annevk, queuing a microtask on the same event loop using the same task source that I used for the normal task makes sense?

[jungkees]

@annevk, I mean queuing a microtask requires specifying a task source as well as an event loop like a normal task case?

[annevk]

No, microtasks don't have a task source. If you do it as part of the fulfillment steps it should be fine.

[jakearchibald]

@wanderview does this need to be open, given the things covered in #1213?

[wanderview]

#1213 should take precedence now, but this issue might be useful for understanding how I helped mess up the WPT.

[jakearchibald]

Closed in favour of #1213 - it's still here for reference.