Improve handling of extend lifetime promises #1049

jungkees · 2017-01-05T11:30:42Z

(1) This changes the approach to unsetting the extendable events'
extensions allowed flag. This change introduced a reference count based
approach instead of the promise-copying-and-checking approach.

(2) This also extends the opportunities of the lifetime extension by
allowing calling waitUntil() within microtasks queued by the given
promise's Promise.prototype.then callbacks.

Related issues:

Does accessing ExtendableEvent's extend lifetime promises race? #931 (1)
should async waitUntil() work in .then() handler of promise passed to previous waitUntil()? #935 (2)
async waitUntil() does not allow extension on final promise resolution? #1039 (2)

jungkees · 2017-01-05T11:38:42Z

@jakearchibald, @annevk, this is about (1) introducing reference count for checking extend lifetime promises and (2) allowing e.waitUntil() in the given promise's promise.then callbacks. Your review would be appreciated.

@domenic, it'd be great if you had time to review these particular steps where I referenced ECMAScript internal slots:

/cc @wanderview

jungkees · 2017-01-05T11:49:03Z

Note that I didn't remove the extend lifetime promises definition. I presume we might want to check if we can rely solely on the pending promises count definition replacing the extend lifetime promises. But I'm not entirely sure yet. The extend lifetime promises is being referenced by other specs to run their own custom callbacks (e.g. https://wicg.github.io/BackgroundSync/spec/#fire-a-sync-event). Examining the extend lifetime promises also allows checking whether the extendable event succeeded or failed depending on the contexts.

(1) This changes the approach to unsetting the extendable events' extensions allowed flag. This change introduced a reference count based approach instead of the promise-copying-and-checking approach. (2) This also extends the opportunities of the lifetime extension by allowing calling waitUntil() within microtasks queued by the given promise's Promise.prototype.then callback. Related issues: - #931 (1) - #935 (2) - #1039 (2)

annevk · 2017-01-05T12:38:43Z

docs/index.bs

@@ -1289,7 +1289,9 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

    An {{ExtendableEvent}} object has an associated <dfn for="ExtendableEvent">extend lifetime promises</dfn> (an array of <a>promises</a>). It is initially an empty array.

-    An {{ExtendableEvent}} object has an associated <dfn id="extensions-allowed-flag">extensions allowed flag</dfn>. It is initially set.
+    An {{ExtendableEvent}} object has an associated <dfn for="ExtendableEvent" id="extensions-allowed-flag">extensions allowed flag</dfn>. It is initially set.


It would be good to invert this somehow. Flags and booleans that default to false are much more intuitive.

Agreed. Will try to change it to something like extensions opportunity closed flag instead.

The flag is replaced by the use of the pending promises count and the dispatch flag.

annevk · 2017-01-05T12:40:27Z

docs/index.bs


+          Note: If no lifetime extension promise has been added up to this point (i.e., at the end of the task that called the event handlers), the [=ExtendableEvent/extensions allowed flag=] is immediately unset. Calling {{ExtendableEvent/waitUntil()}} in subsequent asynchronous tasks will throw.


Why do you need both the flag and the count then?

I think the flag is still needed. Otherwise, we can't determine whether the pending count is in the initial state zero or zero as the extension possibility is over.

Can't you just check the event dispatch flag or some such for that?

I was between leaving the flag as a variable helping the understanding of the control flows and removing it as the cleanup of unnecessary variables. I think the latter wins, so removed it.

annevk · 2017-01-05T12:41:05Z

docs/index.bs

+      :: None
+
+      1. Wait until |promise| is settled [=in parallel=].
+      2. [=Queue a microtask=] to run the following substeps:


You cannot queue microtasks from something that runs in parallel. Needs to be a regular task.

Wait, why is that? Microtasks are just a type of task, in general, so I thought this would work.

Microtasks belong to a task. But there's no task here they could belong too.

This should be queued from the task where the promise is settled. I explicitly used promise’s relevant settings object's responsible event loop to queue this microtask.

annevk · 2017-01-05T12:42:57Z

docs/index.bs

+      1. Wait until |promise| is settled [=in parallel=].
+      2. [=Queue a microtask=] to run the following substeps:
+          1. Decrease |event|'s [=ExtendableEvent/pending promises count=] by one.
+          1. For each |reaction| in |promise|.\[[PromiseFulfillReactions]]:


This seems extremely hacky and probably wrong. I don't think we should be poking at the guts of promises.

I haven't reviewed in detail yet, but I agree poking at [[PromiseFulfillReactions]] is a bad idea. Poking at [[PromiseState]] and [[PromiseResult]] is OK though IMO.

Addressed the comments in #1039 (comment).

domenic · 2017-01-05T18:16:18Z

This doesn't appear to be a good idea. I'm not sure what is trying to be accomplished, but as specced this means that given

const p = getSomePromise();
const q = p.then(doSomething);
e.waitUntil(p);
const r = p.then(doSomethingElse);

will only wait on p and q but not r, which seems surprising. I would suggest trying to find some approach that does not look at [[PromiseFulfillReactions]] at all. Maybe just looking at p is sufficient, in this example, since p is what you actually are passing to waitUntil.

slightlyoff · 2017-01-06T01:08:11Z

@domenic: that's exactly the designed behavior. You can extend the lifetime again by calling e.waitUntil(r) if needed.

domenic · 2017-01-06T01:09:22Z

My point is that it's a bizarre design that the lifetime waits on q when you didn't pass q to waitUntil.

jungkees · 2017-01-06T01:22:54Z

@domenic, let me try to clarify the logic first (and then let's continue about the design):

will only wait on p and q but not r, which seems surprising.

I think it also will wait on r too. My intention was that when Handle Extend Lifetime Promise called in waitUntil goes async, const r = p.then(doSomethingElse); is run and queue the microtask for the doSomethingElse before the Handle Extend Lifetime Promise queues a microtask to chase p.

Referencing Promise.[[PromiseFulfillReactions]] from the internal spec algorithm seems hacky but I think the implementation also has to attach some kind of native handlers to catch and handle the then callbacks. Still looking for a better way to spec it but haven't found one yet.

jungkees · 2017-01-06T01:26:51Z

FYI. The second code example in #935 (comment) is the original problem statement for this design.

domenic · 2017-01-06T01:27:02Z

No implementations provide such hooks today, and they're not meant to: doing so is invasive and constrains the promise implementation from performing optimizations that we're actively working on. Internal state is meant to stay internal. If you can't accomplish this with the public .then API, I'd strongly suggest rethinking the design.

Besides, it's just strange that it's waiting on promises you didn't pass to waitUntil.

jungkees · 2017-01-06T01:36:26Z

I'll take this design discussion back to #1039. I thought @wanderview commented there that Firefox already implemented this behavior. Would like to look into that and also discuss with Chromium implementers around this too.

This makes waitUntil() wait only the given promises instead of all the promises in the given promises' chains. So, it removes the direct references to the promises internal slots. This replaces the extensions allowed flag with the combinations of the pending promises count and the state of the event's dispatch flag.

annevk · 2017-01-09T10:05:45Z

docs/index.bs

-          1. Unset |event|'s <a>extensions allowed flag</a>.
-
-      The user agent *should not* <a lt="terminate service worker">terminate</a> the [=/service worker=] associated with |event|'s <a>relevant settings object</a>'s [=environment settings object/global object=] until |event|'s <a>extensions allowed flag</a> is unset. However, the user agent *may* impose a time limit to this lifetime extension.
+      1. Wait, [=in parallel=], until |promise| is settled and |promise|'s <code>then</code> methods, if any, in the task where |promise| has been settled have executed.


How would you test the "and" requirement here? That should be removed I think.

Without the condition after "and", I'm concerned about the race between this microtask (decrease the count) and the promise.then methods that can be called in the main thread (event handler tasks.) Is it an unnecessary concern?

As I said, this cannot be a microtask. It can only be a task. And therefore it should be fine.

Yeah, if I queue a normal task here. It'd be all fine. But I thought it'd be ideal if we could use a microtask in a way. Firefox actually queues a microtask to check this: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#379.

That code doesn't seem to communicate across threads though.

annevk · 2017-01-09T10:06:01Z

docs/index.bs

-
-      The user agent *should not* <a lt="terminate service worker">terminate</a> the [=/service worker=] associated with |event|'s <a>relevant settings object</a>'s [=environment settings object/global object=] until |event|'s <a>extensions allowed flag</a> is unset. However, the user agent *may* impose a time limit to this lifetime extension.
+      1. Wait, [=in parallel=], until |promise| is settled and |promise|'s <code>then</code> methods, if any, in the task where |promise| has been settled have executed.
+      2. [=Queue a microtask=], on |promise|'s [=relevant settings object=]'s [=responsible event loop=], to decrease |event|'s [=ExtendableEvent/pending promises count=] by one.


As I said earlier, you cannot queue microtasks from parallel algorithms.

Well.. I couldn't find any constraints a microtask should belong to a particular task. Doesn't it work even if the step designate the target event loop? If this doesn't make sense, should I queue a task to queue this microtask?

Now ECMAScript's "PromiseJobs" are queued through https://html.spec.whatwg.org/#enqueuejob(queuename,-job,-arguments). I think those steps are not run in the main thread but in a thread in the JS engine internals.

you cannot queue microtasks from parallel algorithms.

The reason it doesn't make sense is because it is run in a parallel thread? Or it's running off the main thread?

Just queue a task to decrease the thing. It doesn't have to be a microtask.

Queue a microtask from a parallel thread is racy as you say, since you don't know what task it ends up in. User agents only have the ability to queue tasks to get things into the main thread.

Addressed it to using a normal task.

jungkees · 2017-01-10T11:56:55Z

@annevk, do you have any further comments? I'd like to merge it if it's ok.

annevk · 2017-01-10T12:32:49Z

I guess a remaining problem is that you haven't defined the task source.

domenic · 2017-01-10T14:19:18Z

Hmm, do you even need to go in parallel/queue a task? It seems like for promises you should be able to just do "upon fulfillment"/"upon rejection".

annevk · 2017-01-10T14:42:39Z

That's a good point. If you don't queue a task though, it does start to matter when the fulfillment observer is attached, right? I guess as long as that's well-defined it's not really an issue, but would be worth testing.

Do we have tests for this feature?

wanderview · 2017-01-10T17:05:14Z

Do we have tests for this feature?

Not sure if its exactly what you mean, but we wrote this wpt test:

https://github.com/w3c/web-platform-tests/blob/master/service-workers/service-worker/extendable-event-async-waituntil.https.html#L70

jungkees · 2017-01-11T01:46:21Z

Hmm, do you even need to go in parallel/queue a task? It seems like for promises you should be able to just do "upon fulfillment"/"upon rejection".

I don't think we can guarantee the expected sequence by using "upon fulfillment"/"upon rejection" here. This promise.then for decreasing the count should be called after all the promise.then() functions called during the event dispatch. Either queuing a normal task or explicitly specifying the order of the microtask seems to be needed.

domenic · 2017-01-11T02:17:15Z

Why does it need to happen after the then callbacks? That doesn't show up in the problem description. If you want it to happen after p.then(f) executes f, pass p.then(f) to waitUntil, instead of passing p to waitUntil.

jungkees · 2017-01-11T02:28:37Z

In the following snippet for example:

addEventListener('push', e => {
  let outResolve;
  let p = new Promise(resolve => { outResolve = resolve; });
  e.waitUntil(p); // Upon fulfillment/rejection of p inside
  p.then(() => {
    e.waitUntil(q);
  });
  setTimeout(() => outResolve(), 1);
});

I think the "Upon fulfillment/rejection of p" triggered inside of waitUntil() queues (precisely, it appends the reaction to p.[[PromiseFulfillReactions]] as p is pending) the dcrease job before the promise job for e.waitUntil(q); is queued, which makes e.waitUntil(q); throw.

domenic · 2017-01-11T02:29:25Z

Hmm I guess that does appear in #1039 (comment). I think that's a bad design, but it's not a huge deal. I guess if you want that you can just do "upon fulfillment/rejection, queue a task".

domenic · 2017-01-11T02:34:17Z

@jungkees yeah, I see. I think that is a bad design and if you want that code to work it should be coded as

addEventListener('push', e => {
  let outResolve;
  let p = new Promise(resolve => { outResolve = resolve; });
  e.waitUntil(p.then(() => {
    e.waitUntil(q);
  });
  setTimeout(() => outResolve(), 1);
});

since then you actually "wait until" the argument (instead of "wait until-but-also-wait-an-extra-turn" the argument). But it's not a huge deal.

jungkees · 2017-01-11T02:54:27Z

Yeah, "wait until-but-also-wait-an-extra-turn" the argument is the original problem statement and the behavior people agreed on. With your comment, I sort of come to think of again whether this was a right decision but would like to settle on what we decided.

For the spec wording, which of the following would be better?

Upon fulfillment of promise, queue a task to decrease event’s pending promises count by one.
Upon rejection of promise, queue a task to decrease event’s pending promises count by one.

or

Upon fulfillment or rejection of promise, queue a task to decrease event’s pending promises count by one.

or

Upon settlement of promise, queue a task to decrease event’s pending promises count by one.

domenic · 2017-01-11T02:56:15Z

Yeah, "wait until-but-also-wait-an-extra-turn" the argument is the original problem statement and the behavior people agreed on. With your comment, I sort of come to think of again whether this was a right decision but would like to settle on what we decided.

Yep, that's fair; I totally understand.

For the spec wording, which of the following would be better?

IMO Upon fulfillment or rejection of promise, queue a task to decrease event’s pending promises count by one. is the best combination of brevity and using well-defined terms. We should maybe define "upon fulfillment" but currently we don't.

jungkees · 2017-01-11T03:00:55Z

OK. That sounds good. Thanks!

…allel

jungkees · 2017-01-11T12:47:05Z

@annevk, I merged it having addressed your task source comment and @domenic's promise shorthand comment. Thanks for reviewing it!

slightlyoff · 2017-01-13T01:00:26Z

@domenic: thanks for the clarification. I agree it's a bit bizarre to have a derived promise extend the lifetime and that only the argument to waitUntil() should be gauranteed to extend things.

jungkees assigned jakearchibald and annevk and unassigned jakearchibald and annevk Jan 5, 2017

jungkees requested review from jakearchibald and annevk January 5, 2017 11:31

annevk requested changes Jan 5, 2017

View reviewed changes

jungkees mentioned this pull request Jan 6, 2017

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

Closed

jungkees force-pushed the sw-lifetime-extension branch from 3210c4d to 93b6499 Compare January 9, 2017 09:55

annevk requested changes Jan 9, 2017

View reviewed changes

Use a normal task for dcrease count

480305a

jungkees mentioned this pull request Jan 10, 2017

Should 'Extend Service Worker Lifetime' be called after dispatching 'message' #1050

Closed

Designate task source; use reacting to promise instead of wait in par…

f88ea60

…allel

jungkees merged commit 6c1f3fe into master Jan 11, 2017

jungkees deleted the sw-lifetime-extension branch January 11, 2017 12:37


		Note: If no lifetime extension promise has been added up to this point (i.e., at the end of the task that called the event handlers), the [=ExtendableEvent/extensions allowed flag=] is immediately unset. Calling {{ExtendableEvent/waitUntil()}} in subsequent asynchronous tasks will throw.

Improve handling of extend lifetime promises #1049

Improve handling of extend lifetime promises #1049

Conversation

jungkees commented Jan 5, 2017

jungkees commented Jan 5, 2017

jungkees commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Jan 5, 2017

slightlyoff commented Jan 6, 2017

domenic commented Jan 6, 2017

jungkees commented Jan 6, 2017 • edited Loading

jungkees commented Jan 6, 2017

domenic commented Jan 6, 2017

jungkees commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Jan 10, 2017

annevk commented Jan 10, 2017

domenic commented Jan 10, 2017

annevk commented Jan 10, 2017

wanderview commented Jan 10, 2017

jungkees commented Jan 11, 2017

domenic commented Jan 11, 2017 • edited Loading

jungkees commented Jan 11, 2017 • edited Loading

domenic commented Jan 11, 2017

domenic commented Jan 11, 2017

jungkees commented Jan 11, 2017

domenic commented Jan 11, 2017

jungkees commented Jan 11, 2017

jungkees commented Jan 11, 2017

slightlyoff commented Jan 13, 2017

jungkees commented Jan 6, 2017 •

edited

Loading

domenic commented Jan 11, 2017 •

edited

Loading

jungkees commented Jan 11, 2017 •

edited

Loading