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.

Sign up for GitHub

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

Improve handling of extend lifetime promises #1049

Merged
merged 4 commits into from
Jan 11, 2017
+293 −383
Merged

Improve handling of extend lifetime promises #1049

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39d3132
Improve handling of extend lifetime promises
jungkees Jan 5, 2017
93b6499
Fix promise handling; remove extensions allowed flag
jungkees Jan 9, 2017
480305a
Use a normal task for dcrease count
jungkees Jan 9, 2017
f88ea60
Designate task source; use reacting to promise instead of wait in par…
jungkees Jan 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 29 additions & 21 deletions docs/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1289,25 +1289,34 @@ 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">pending promises count</dfn> (the number of pending promises in the [=ExtendableEvent/extend lifetime promises=]). It is initially set to zero.

[=/Service workers=] have two <a>lifecycle events</a>, {{install!!event}} and {{activate!!event}}. [=/Service workers=] use the {{ExtendableEvent}} interface for {{activate!!event}} event and {{install!!event}} event.

<a href="#extensibility">Service worker extensions</a> that <a href="#extension-to-service-worker-global-scope">define event handlers</a> *may* also use or extend the {{ExtendableEvent}} interface.

Note: To extend the lifetime of a [=/service worker=], algorithms that <a>dispatch</a> events using the {{ExtendableEvent}} interface run [=Extend Service Worker Lifetime=] algorithm after <a>dispatching</a> the event. See <a>Handle Fetch</a>, <a>Handle Foreign Fetch</a>, and <a>Handle Functional Event</a>.

<section algorithm="wait-until-method">
<h4 id="wait-until-method">{{ExtendableEvent/waitUntil()|event.waitUntil(f)}}</h4>

{{ExtendableEvent/waitUntil()}} method extends the lifetime of the event.

<dfn method for="ExtendableEvent"><code>waitUntil(|f|)</code></dfn> method *must* run these steps:

1. If the <a>extensions allowed flag</a> is unset, then:
1. If the [=ExtendableEvent/pending promises count=] is zero and the [=dispatch flag=] is unset, then:
1. <a>Throw</a> an "{{InvalidStateError}}" exception.
1. Abort these steps.

Note: If no lifetime extension promise has been added in the task that called the event handlers), calling {{ExtendableEvent/waitUntil()}} in subsequent asynchronous tasks will throw.

1. Add |f| to the [=ExtendableEvent/extend lifetime promises=].
1. Increase the [=ExtendableEvent/pending promises count=] by one.

Note: The [=ExtendableEvent/pending promises count=] is increased even if the given promise has already been settled. The corresponding count decrease is done within [=Handle Extend Lifetime Promise=] algorithm.

1. Run [=Handle Extend Lifetime Promise=] with the [=context object=] and |f|.
1. Return.

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=] when |event|'s [=dispatch flag=] is set or |event|'s [=ExtendableEvent/pending promises count=] is not zero. However, the user agent *may* impose a time limit to this lifetime extension.
</section>

[=/Service workers=] and <a href="#extensibility">extensions</a> that <a href="#extension-to-service-worker-global-scope">define event handlers</a> *may* define their own behaviors, allowing the [=ExtendableEvent/extend lifetime promises=] to suggest operation length, and the rejected state of any of the <a>promise</a> in [=ExtendableEvent/extend lifetime promises=] to suggest operation failure.
Expand Down Expand Up @@ -1450,6 +1459,11 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. <a>Throw</a> an "{{InvalidStateError}}" exception.
1. Abort these steps.
1. Add |r| to the <a>extend lifetime promises</a>.
1. Increase the [=ExtendableEvent/pending promises count=] by one.

Note: The [=ExtendableEvent/pending promises count=] is increased even if the given promise has already been settled. The corresponding count decrease is done within [=Handle Extend Lifetime Promise=] algorithm.

1. Run [=Handle Extend Lifetime Promise=] with the [=context object=] and |r|.

Note: {{FetchEvent/respondWith(r)|event.respondWith(r)}} extends the lifetime of the event by default as if {{ExtendableEvent/waitUntil()|event.waitUntil(r)}} is called.

Expand Down Expand Up @@ -1561,6 +1575,11 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. <a>Throw</a> an "{{InvalidStateError}}" exception.
1. Abort these steps.
1. Add |r| to the <a>extend lifetime promises</a>.
1. Increase the [=ExtendableEvent/pending promises count=] by one.

Note: The [=ExtendableEvent/pending promises count=] is increased even if the given promise has already been settled. The corresponding count decrease is done within [=Handle Extend Lifetime Promise=] algorithm.

1. Run [=Handle Extend Lifetime Promise=] with the [=context object=] and |r|.
1. Set the <a>stop propagation flag</a> and <a>stop immediate propagation flag</a>.
1. Set the [=ForeignFetchEvent/respond-with entered flag=].
1. Set the [=ForeignFetchEvent/wait to respond flag=].
Expand Down Expand Up @@ -2678,9 +2697,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Let |e| be the result of <a>creating an event</a> with {{InstallEvent}}.
1. Initialize |e|’s {{Event/type}} attribute to {{install!!event}}.
1. <a>Dispatch</a> |e| at |installingWorker|'s [=service worker/global object=].
1. Invoke [=Extend Service Worker Lifetime=] with |e|.
1. *WaitForAsynchronousExtensions*: Run the following substeps <a>in parallel</a>:
1. Wait until |e|'s <a>extensions allowed flag</a> is unset.
1. Wait until |e|'s [=ExtendableEvent/pending promises count=] is zero.
1. If the result of <a>waiting for all</a> of |e|'s <a>extend lifetime promises</a> rejected, set |installFailed| to true.

If |task| is discarded or the script has been aborted by the <a lt="Terminate Service Worker">termination</a> of |installingWorker|, set |installFailed| to true.
Expand Down Expand Up @@ -2744,8 +2762,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Let |e| be the result of <a>creating an event</a> with {{ExtendableEvent}}.
1. Initialize |e|’s {{Event/type}} attribute to {{activate!!event}}.
1. <a>Dispatch</a> |e| at |activeWorker|'s [=service worker/global object=].
1. Invoke [=Extend Service Worker Lifetime=] with |e|.
1. *WaitForAsynchronousExtensions*: Wait, <a>in parallel</a>, until |e|'s <a>extensions allowed flag</a> is unset.
1. *WaitForAsynchronousExtensions*: Wait, <a>in parallel</a>, until |e|'s [=ExtendableEvent/pending promises count=] is zero.
1. Wait for |task| to have executed or been discarded, or the script to have been aborted by the <a lt="terminate service worker">termination</a> of |activeWorker|.
1. Wait for the step labeled *WaitForAsynchronousExtensions* to complete.
1. Run the <a>Update Worker State</a> algorithm passing |registration|'s <a>active worker</a> and *activated* as the arguments.
Expand Down Expand Up @@ -2830,22 +2847,16 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
</section>

<section algorithm>
<h3 id="extend-service-worker-lifetime-algorithm"><dfn>Extend Service Worker Lifetime</dfn></h3>
<h3 id="handle-extend-lifetime-promise-algorithm"><dfn>Handle Extend Lifetime Promise</dfn></h3>

: Input
:: |event|, an {{ExtendableEvent}} object
:: |promise|, a [=promise=]
: Output
:: None

1. If |event|'s <a>extend lifetime promises</a> is empty, unset |event|'s <a>extensions allowed flag</a> and abort these steps.
1. Let |extendLifetimePromises| be an empty array.
1. Run the following substeps <a>in parallel</a>:
1. *SetupPromiseArray*: Set |extendLifetimePromises| to a copy of |event|'s <a>extend lifetime promises</a>.
1. Wait until all the <a>promises</a> in |extendLifetimePromises| settle.
1. If the length of |extendLifetimePromises| does not equal the length of |event|'s <a>extend lifetime promises</a>, jump to the step labeled *SetupPromiseArray*.
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

2. [=Queue a microtask=], on |promise|'s [=relevant settings object=]'s [=responsible event loop=], to decrease |event|'s [=ExtendableEvent/pending promises count=] by one.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed it to using a normal task.

</section>

<section algorithm>
Expand Down Expand Up @@ -2926,7 +2937,6 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. If |request| is a <a>navigation request</a>, initialize |e|'s {{FetchEvent/targetClientId}} attribute to |request|'s [=request/target client id=], and to the empty string otherwise.
1. Let the {{FetchEvent/isReload}} attribute of |e| be initialized to <code>true</code> if |request|'s [=request/client=] is a <a>window client</a> and the event was dispatched with the user's intention for the page reload, and <code>false</code> otherwise.
1. <a>Dispatch</a> |e| at |activeWorker|'s [=service worker/global object=].
1. Invoke [=Extend Service Worker Lifetime=] with |e|.
1. If |e|'s [=FetchEvent/respond-with entered flag=] is set, set |respondWithEntered| to true.
1. If |e|'s [=FetchEvent/wait to respond flag=] is set, then:
1. Wait until |e|'s [=FetchEvent/wait to respond flag=] is unset.
Expand Down Expand Up @@ -2990,7 +3000,6 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Initialize |e|’s {{ForeignFetchEvent/request}} attribute to |r|.
1. Initialize |e|’s {{ForeignFetchEvent/origin}} attribute to the <a lt="Unicode serialization of an origin">Unicode serialization</a> of |request|'s [=request/origin=].
1. <a>Dispatch</a> |e| at |activeWorker|'s [=service worker/global object=].
1. Invoke [=Extend Service Worker Lifetime=] with |e|.
1. If |e|'s [=ForeignFetchEvent/respond-with entered flag=] is set, set |respondWithEntered| to true.
1. If |e|'s <a>wait to respond flag</a> is set, wait until |e|'s <a>wait to respond flag</a> is unset.
1. Let |internalResponse| be |e|'s [=ForeignFetchEvent/potential response=].
Expand Down Expand Up @@ -3052,7 +3061,6 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Invoke <a>Run Service Worker</a> algorithm with |activeWorker| as the argument.
1. <a>Queue a task</a> |task| to run these substeps:
1. Invoke |callbackSteps| with |activeWorker|'s [=service worker/global object=] as its argument.
1. Invoke [=Extend Service Worker Lifetime=] with |event|.

The |task| *must* use |activeWorker|'s <a>event loop</a> and the <a>handle functional event task source</a>.

Expand Down
Loading