Handle asynchronous modules (top-level await). #1444

Ms2ger · 2019-06-25T13:30:29Z

jakearchibald · 2019-06-25T13:38:48Z

Is top-level await only allowed in module scripts?

Ms2ger · 2019-06-25T13:43:54Z

Yes, that's correct.

jakearchibald

LGTM. @annevk are you happy with this as the final approach? Top level await is allowed, but any listeners added after an await will throw.

littledan

This patch has the semantics I was imagining. It's probably worth including a note about the unfortunate compatibility situation leading to the synchronous check of the promise (which is generally a design anti-pattern).

Or, to the folks here more familiar with ServiceWorker usage: would it be OK to skip the synchronous error check, and just rely on the error being reported a bit later, and letting the rest of the initialization algorithm complete as if the module loaded successfully?

docs/index.bs

jakearchibald · 2019-06-27T07:58:21Z

would it be OK to skip the synchronous error check, and just rely on the error being reported a bit later,

I don't think so. When we first install a service worker, if it throws an error during initial execution, we discard it.

annevk · 2019-06-27T13:51:30Z

I'm probably missing something, but how could the timing be observed? (In particular for the error case. I don't quite understand why it matters that the check there is synchronous or how that is observable.)

Can you add listeners before an await? At what point are they invoked?

jakearchibald · 2019-06-28T08:43:20Z

Here's how I think this works: (@littledan: correct me if I'm wrong)

When we run a service worker, we consider it as 'evaluated' and ready to receive events after synchronous execution + microtasks.

Errors & evaluation

This error will prevent a service worker from installing:

throw Error('…');

As will this:

await undefined;

throw Error('…');

As will this:

import './foo.js';

// If `./foo.js` is:
throw Error('…');

However, this error will not prevent a service worker from installing:

await new Promise(r => setTimeout(r));

throw Error('…');

…as the error has happened after 'evaluation'.

Adding listeners

// This is fine
addEventListener('fetch', func);

// This is fine
await undefined;
addEventListener('fetch', func);

// This shows a warning, and the fetch listener isn't added.
await new Promise(r => setTimeout(r));
addEventListener('fetch', func);

However, the throw is asynchronous, so it wouldn't prevent install.

Dispatching events

We consider the service worker ready to receive events once it's evaluated:

addEventListener('fetch', () => {
  console.log('A');
});

await new Promise(r => setTimeout(r, 5000));
console.log('B');

The console may log A before B.

Concerns

import './third-party-script-v1.js';

addEventListener('install', event => …);
addEventListener('fetch', event => …);

The above works as the developer expects. However, later, they update to './third-party-script-v2.js'. This include a top-level await that waits beyond a microtask.

Now, their service worker updates successfully, but their install and fetch listeners are absent. They'll show as an error in the console, but the developer may miss them.

annevk · 2019-06-28T09:10:24Z

Thank you! "Concerns" seems pretty bad from a debugging perspective, i.e., action at a distance.

To address @littledan's concern, we could perhaps race the evaluationPromise with another promise, but maybe that introduces issues for the remainder of the algorithm. (I was also thinking of this as a way to allow top-level await, but only for a limited number of ticks.)

This whole setup still seems rather suboptimal for the design of service workers.

jakearchibald · 2019-06-28T09:28:50Z

I was also thinking of this as a way to allow top-level await, but only for a limited number of ticks

What would we do if it exceeded that number of ticks?

This whole setup still seems rather suboptimal for the design of service workers.

Yeah, it still feels like we'd have to tell developers "you can do this but oh god please don't do this".

Looking at the PR, we could fail the install of a service worker if its |evaluationPromise|.\[[PromiseState]] is "pending". This would effectively prevent top-level await in service workers at install time. A top-level await could be introduced later, using something like if (Date.now() > …), in which case we'd behave as above.

annevk · 2019-06-28T09:37:14Z

I was thinking that if you exceed the ticks it's equivalent to an evaluation error. So you're allowed to do some minor queueMicrotask()-type stuff, but nothing else.

(At some point we need to have a conversation with TC39 about language extensions and the different types of host environments. Not everything that is great for documents/dedicated workers/servers necessarily makes sense for workets/service workers.)

jakearchibald · 2019-06-28T09:42:12Z

I was thinking that if you exceed the ticks it's equivalent to an evaluation error. So you're allowed to do some minor queueMicrotask()-type stuff, but nothing else.

I thought we already got microtasks for free since they're handled just after "executing as script". The bit I'd be worried about is:

addEventListener('fetch', …);
await usuallyFast();

…which means you have a service worker that usually works, but sometimes doesn't.

littledan · 2019-06-28T17:52:02Z

Yeah, these are the semantics I was suggesting. Another possibility (based on a suggestion from @annevk) would be to have a per-agent variable which sets whether TLA will currently be allowed. We could make the module link algorithm check this flag, and make the flag disable TLA just while initializing the module, and reenable it later. How does this sound?

jakearchibald

Going to unapprove this until we figure out the behaviour we want at TPAC next week.

Ms2ger · 2019-09-20T09:33:29Z

With the changes in tc39/proposal-top-level-await#128 and whatwg/html#4352, I believe this implements the new semantics.

annevk · 2019-09-23T13:41:15Z

Is there no way to avoid looking at the internals of a promise? If we really need to do that perhaps provide an abstraction in IDL? At the moment TC39 will only alert IDL and HTML of changes to such things (if it happens at all).

littledan · 2019-09-23T14:34:20Z

In previous drafts, the result of evaluating a module was either a Promise or a synchronous reaction. Following feedback from @domenic, this was made to always return a single Promise. We could go back to the old way, to avoid looking at Promise internals here, though it would add a couple lines of spec algorithm to HTML.

jakearchibald

This looks good. Just one question (see review).

jakearchibald · 2020-01-21T13:37:57Z

docs/index.bs

+      : Output
+      :: a boolean
+
+      1. If |record| is not a [=Cyclic Module Record=], then:


I'm struggling to figure out what a "Cyclic Module Record" is. What's the difference between a Cyclic Module Record and a non-Cyclic Module Record? How does each relate to a module?

JavaScript modules (Source Text Module Records) had a bunch of their functionality for traversing the module graph refactored up into a spec-internal superclass called Cyclic Module Records, so that WebAssembly modules could share the logic. Not all modules use this infrastructure, for example, JSON modules are leaves so they don't bother.

jakearchibald

LGTM.

Are there any tests for this? If not I can write some.

littledan · 2020-01-23T11:31:03Z

@jakearchibald I think we're missing tests for this as well as whatwg/html#4352 , making it a rather big project. I'd welcome help here!

hiroshige-g · 2020-09-17T05:31:50Z

docs/index.bs

+              1. Let |evaluationPromise| be the result of <a lt="run a module script">running the module script</a> |script|, with report errors set to false.
+              1. Assert: |evaluationPromise|.\[[PromiseState]] is not "pending".
+              1. If |evaluationPromise|.\[[PromiseState]] is "rejected":
+                  1. Set |startFailed| to true and abort these steps.


I agree with @hiroshige-g. This is subtle, but throwing an error during service worker script evaluation doesn't count as a startup failure in the current spec. The service worker thread is running and will get events dispatched to it. Throwing an error only affects the Install algorithm; the service worker will subsequently be terminated and will not be installed. If we want to change this behavior, we should do so for classic scripts as well as module scripts.

hiroshige-g · 2020-09-23T03:45:00Z

docs/index.bs

-              1. Invoke [=Reject Job Promise=] with |job| and `TypeError`.
-
-                  Note: This will do nothing if [=Reject Job Promise=] was previously invoked with "{{SecurityError}}" {{DOMException}}.
+      1. If |script| is null or [=Is Async Module=] with |script|'s [=script/record=], |script|'s [=script/base URL=], and « » is true, then:


A random idea: How about checking whether |evaluationPromise|.\[[PromiseState]] is "pending" instead of [=Is Async Module=] ... is true?

This makes the logic a little simpler (using only |evaluationPromise|.\[[PromiseState]], removing module graph traversal and module inspection), but this is not equivalent from the current condition (e.g. for modules with await undefined).

A goal of the design here was to not be "racy" in the sense that, both asynchronous behavior and errors are based on the module syntax, not whether it has already run or not, or whether it dynamically reached an await. This would make behavior more stable.

Thanks, it makes sense > based on syntax.

This is a predicate checking if any module in a module graph is [[Async]], i.e. contains a top-level await. It is needed for ServiceWorker integration, as ServiceWorkers disallows top-level await in its modules to prevent stalling during registration. w3c/ServiceWorker#1444 Bug: v8:9344 Change-Id: Id84489bc73717b4c9950059c8ff6def9297499d0 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2451212 Commit-Queue: Shu-yu Guo <syg@chromium.org> Reviewed-by: Camillo Bruni <cbruni@chromium.org> Reviewed-by: Georg Neis <neis@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#70390}

Top-level await is prohibited in service workers and causes the Update algorithm to reject with a TypeError according to the spec PR [1]. This CL also fixes error handling in SWs after top-level await integration. Since this rejection happens before evaluation, all Promises returned by module evaluation are synchronously settled. (Module evaluation always returns Promises after top-level await integration). [1] treats synchronously rejected Promises the same as errors thrown from sync module scripts without top-level await. This CL implements the new behavior. [1] w3c/ServiceWorker#1444 Bug: 1129795 Bug: 1022182 Bug: v8:9344 Change-Id: Ia764ab553b88e52d48125f23632dc77f4c256009

This CL disallows top-level await for service workers by terminating the worker. The error that's thrown when Run Service Worker fails is changed to a TypeError per the spec draft for top-level await integration [0]. Note that this also changes the sudden termination errors that were previously AbortErrors to TypeErrors. [0] w3c/ServiceWorker#1444 Bug: 1129795 Bug: 1022182 Bug: v8:9344 Change-Id: Ia764ab553b88e52d48125f23632dc77f4c256009 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2454306 Commit-Queue: Shu-yu Guo <syg@chromium.org> Reviewed-by: Dominic Farolino <dom@chromium.org> Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/master@{#832849}

…it in service workers, a=testonly Automatic update from web-platform-tests [top-level await] Prohibit top-level await in service workers This CL disallows top-level await for service workers by terminating the worker. The error that's thrown when Run Service Worker fails is changed to a TypeError per the spec draft for top-level await integration [0]. Note that this also changes the sudden termination errors that were previously AbortErrors to TypeErrors. [0] w3c/ServiceWorker#1444 Bug: 1129795 Bug: 1022182 Bug: v8:9344 Change-Id: Ia764ab553b88e52d48125f23632dc77f4c256009 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2454306 Commit-Queue: Shu-yu Guo <syg@chromium.org> Reviewed-by: Dominic Farolino <dom@chromium.org> Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/master@{#832849} -- wpt-commits: aa77795ebe17dc0d7c3259a3fe92023b9da62d3e wpt-pr: 26019

…it in service workers, a=testonly Automatic update from web-platform-tests [top-level await] Prohibit top-level await in service workers This CL disallows top-level await for service workers by terminating the worker. The error that's thrown when Run Service Worker fails is changed to a TypeError per the spec draft for top-level await integration [0]. Note that this also changes the sudden termination errors that were previously AbortErrors to TypeErrors. [0] w3c/ServiceWorker#1444 Bug: 1129795 Bug: 1022182 Bug: v8:9344 Change-Id: Ia764ab553b88e52d48125f23632dc77f4c256009 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2454306 Commit-Queue: Shu-yu Guo <sygchromium.org> Reviewed-by: Dominic Farolino <domchromium.org> Reviewed-by: Makoto Shimazu <shimazuchromium.org> Reviewed-by: Hiroshige Hayashizaki <hiroshigechromium.org> Cr-Commit-Position: refs/heads/master{#832849} -- wpt-commits: aa77795ebe17dc0d7c3259a3fe92023b9da62d3e wpt-pr: 26019 UltraBlame original commit: 259caf0f5dd8d0d96bc5604e0b5f0ad11f16faf1

jakearchibald · 2021-01-18T10:59:40Z

@mfalken could you take a look at this too?

jakearchibald

Looks good aside from the undefined. Also, are there any tests?

docs/index.bs

Ms2ger

I don't know off-hand what the testing situation is. Can check later.

docs/index.bs

Ms2ger · 2021-01-20T12:09:54Z

A test was added at web-platform-tests/wpt#26019; thanks @codehag for pointing it out to me. Let me know if you want me to look into adding more tests.

mfalken

Sorry for the delay! I needed some time to understand this.

Can the commit description elaborate more on what the change is (the bug thread is very long). It sounds like this is implementing #1407 (comment), is that right?

mfalken · 2021-01-22T05:52:50Z

docs/index.bs

+              1. Let |evaluationPromise| be the result of <a lt="run a module script">running the module script</a> |script|, with report errors set to false.
+              1. Assert: |evaluationPromise|.\[[PromiseState]] is not "pending".
+              1. If |evaluationPromise|.\[[PromiseState]] is "rejected":
+                  1. Set |startFailed| to true and abort these steps.


I agree with @hiroshige-g. This is subtle, but throwing an error during service worker script evaluation doesn't count as a startup failure in the current spec. The service worker thread is running and will get events dispatched to it. Throwing an error only affects the Install algorithm; the service worker will subsequently be terminated and will not be installed. If we want to change this behavior, we should do so for classic scripts as well as module scripts.

mfalken · 2021-01-22T05:56:11Z

docs/index.bs

+              1. If |evaluationPromise|.\[[PromiseState]] is "rejected":
+                  1. Set |startFailed| to true and abort these steps.
+
+                      Note: This is intended to maintain compatibilty with failing synchronous modules before the introduction of asynchronous modules.


I don't understand this point, and it seems to be opposite to what @hiroshige-g is saying. Does throwing an error in the synchronous module case cause |startFailed| to be true?

(If we keep the note, there's a typo "compatibilty".)

mfalken · 2021-01-22T06:02:52Z

docs/index.bs

+          1. Return false.
+      1. If |record|.\[[Async]] is true, then:
+          1. Return true.
+      1. [=list/For each=] string |requested| of |record|.\[[RequestedModules]],


nit: use a colon instead of a comma at the end for consistency

mfalken · 2021-01-22T06:03:03Z

docs/index.bs

+          1. Assert: |url| is never failure, because [=resolve a module specifier|resolving a module specifier=] must have been previously successful with these same two arguments.
+          1. If |seen| does not [=set/contain=] |url|, then:
+              1. [=set/Append=] |url| to |seen|.
+              1. If [=Is Async Module=] for |moduleMap|[|url|]'s [=script/record=], |moduleMap|, |base|, and |seen| is true, then


nit: colon at end

syg · 2021-05-20T01:36:50Z

docs/index.bs

+          1. Return true.
+      1. [=list/For each=] string |requested| of |record|.\[[RequestedModules]],
+          1. Let |url| be the result of [=resolve a module specifier|resolving a module specifier=] given |base| and |requested|.
+          1. Assert: |url| is never failure, because [=resolve a module specifier|resolving a module specifier=] must have been previously successful with these same two arguments.


During implementation in Chrome we realized that linking failures means this assert can trip. If the module graph couldn't be successfully linked, we can't assume the entire module graph is accessible this way.

I'm not really familiar with the module loading stuff. Any idea what spec change is needed here?

I don't grok the module loading stuff enough myself to be confident with any suggested fix. I don't think resolving a module specifier itself can fail, but that a module graph could have failed to Link(). In which case a requested module's url may not map to any module record in the module map.

Is there an observable error timing currently for a "SW has a TLA" error vs a linking error? My in-flight CL in Chrome chooses to only check if a module graph has any TLAs in it if it successfully linked. If it failed to link, it doesn't check if it's async at all and let the linking error propagate.

While reviewing this I was not sure if this comment was resolved. If I understand correctly, per the next comment thread, the assert failure was fixed in a later commit to this PR that added the following:

If |moduleMap|[|url|] does not have a [=script/record=], then return false.

Fixes w3c#1407. Co-authored-by: Jake Archibald <jaffathecake@gmail.com> Co-authored-by: yulia <ystartsev@mozilla.com>

codehag · 2021-05-21T15:56:29Z

I think we addressed the open issues, should we rebase?

syg · 2021-05-21T16:26:52Z

docs/index.bs

+          1. Return true.
+      1. [=list/For each=] string |requested| of |record|.\[[RequestedModules]]:
+          1. Let |url| be the result of [=resolve a module specifier|resolving a module specifier=] given |base| and |requested|.
+          1. If |url| is *failure*:


I don't know if this can actually fail (I'm inclined to believe no, it can't).

syg · 2021-05-21T16:27:21Z

docs/index.bs

+              1. Return false.
+          1. If |seen| does not [=set/contain=] |url|, then:
+              1. [=set/Append=] |url| to |seen|.
+              1. If [=Is Async Module=] for |moduleMap|[|url|]'s [=script/record=], |moduleMap|, |base|, and |seen| is true, then:


The spec bug I was pointing out in #1444 (comment) is here, moduleMap[url] might not have a module record in case of link failure.

Tried to fix this.

I think we also want to bring the assertion back: Ms2ger@6290a2c

mfalken

I'm very sorry for the long delay. I am not an expert here, but this looks good to me. In particular, my main concern from the previous review about startFailure has been addressed. Thank you.

mfalken · 2021-06-11T05:23:37Z

docs/index.bs

+              1. Let |evaluationPromise| be the result of [=run a module script|running the module script=] |script|, with report errors set to false.
+              1. Assert: |evaluationPromise|.\[[PromiseState]] is not "pending".
+              1. If |evaluationPromise|.\[[PromiseState]] is "rejected":
+                  1. Set |evaluationStatus| to [$ThrowCompletion$](|evaluationPromise|.\[[PromiseResult]]).


ThrowCompletion() does not become a link in the Preview. Is it possible to fix it?

mfalken · 2021-06-11T05:29:11Z

docs/index.bs

+          1. Return true.
+      1. [=list/For each=] string |requested| of |record|.\[[RequestedModules]],
+          1. Let |url| be the result of [=resolve a module specifier|resolving a module specifier=] given |base| and |requested|.
+          1. Assert: |url| is never failure, because [=resolve a module specifier|resolving a module specifier=] must have been previously successful with these same two arguments.


While reviewing this I was not sure if this comment was resolved. If I understand correctly, per the next comment thread, the assert failure was fixed in a later commit to this PR that added the following:

If |moduleMap|[|url|] does not have a [=script/record=], then return false.

mfalken · 2021-06-11T08:18:39Z

Actually I'll just merge this and we can fix the link later.

This was referenced Jun 25, 2019

Tracking: Platform integration tc39/proposal-top-level-await#119

Closed

Top-level await integration whatwg/html#4352

Merged

jakearchibald approved these changes Jun 25, 2019

View reviewed changes

littledan reviewed Jun 26, 2019

View reviewed changes

docs/index.bs Outdated Show resolved Hide resolved

jakearchibald requested changes Sep 12, 2019

View reviewed changes

jakearchibald mentioned this pull request Sep 15, 2019

Top-level await integration for ServiceWorkers running modules #1407

Closed

Ms2ger force-pushed the tla branch from 53a6cce to b62c3c8 Compare November 29, 2019 09:31

Ms2ger requested a review from jakearchibald December 9, 2019 14:03

jakearchibald reviewed Jan 21, 2020

View reviewed changes

jakearchibald approved these changes Jan 23, 2020

View reviewed changes

hiroshige-g reviewed Sep 17, 2020

View reviewed changes

hiroshige-g reviewed Sep 23, 2020

View reviewed changes

chromium-wpt-export-bot mentioned this pull request Oct 9, 2020

[top-level await] Prohibit top-level await in service workers web-platform-tests/wpt#26019

Merged

Ms2ger requested a review from jakearchibald January 11, 2021 16:52

jakearchibald requested changes Jan 18, 2021

View reviewed changes

docs/index.bs Outdated Show resolved Hide resolved

docs/index.bs Outdated Show resolved Hide resolved

docs/index.bs Outdated Show resolved Hide resolved

docs/index.bs Outdated Show resolved Hide resolved

docs/index.bs Show resolved Hide resolved

Ms2ger commented Jan 18, 2021

View reviewed changes

docs/index.bs Show resolved Hide resolved

jakearchibald approved these changes Jan 18, 2021

View reviewed changes

mfalken reviewed Jan 22, 2021

View reviewed changes

Base automatically changed from master to main February 4, 2021 19:56

syg reviewed May 20, 2021

View reviewed changes

Handle asynchronous modules (top-level await).

12fbfaf

Fixes w3c#1407. Co-authored-by: Jake Archibald <jaffathecake@gmail.com> Co-authored-by: yulia <ystartsev@mozilla.com>

syg reviewed May 21, 2021

View reviewed changes

Ms2ger force-pushed the tla branch from 46ea106 to 12fbfaf Compare May 21, 2021 17:16

Ms2ger added 2 commits May 25, 2021 15:13

Tentative fix

d4db15b

fixup! Tentative fix

51e01b8

Ms2ger mentioned this pull request May 25, 2021

Tla take 2 Ms2ger/ServiceWorker#2

Closed

mfalken approved these changes Jun 11, 2021

View reviewed changes

jakearchibald merged commit 9c77873 into w3c:main Jun 11, 2021

Ms2ger deleted the tla branch June 11, 2021 11:41

ADKaster mentioned this pull request Oct 18, 2024

Steps for Update have an empty condition for async modules #1732

Closed

Handle asynchronous modules (top-level await). #1444

Handle asynchronous modules (top-level await). #1444

Conversation

Ms2ger commented Jun 25, 2019 • edited by pr-preview bot Loading

jakearchibald commented Jun 25, 2019

Ms2ger commented Jun 25, 2019

jakearchibald left a comment

Choose a reason for hiding this comment

littledan left a comment

Choose a reason for hiding this comment

jakearchibald commented Jun 27, 2019

annevk commented Jun 27, 2019 • edited Loading

jakearchibald commented Jun 28, 2019 • edited Loading

Errors & evaluation

Adding listeners

Dispatching events

Concerns

annevk commented Jun 28, 2019

jakearchibald commented Jun 28, 2019

annevk commented Jun 28, 2019

jakearchibald commented Jun 28, 2019

littledan commented Jun 28, 2019

jakearchibald left a comment • edited Loading

Choose a reason for hiding this comment

Ms2ger commented Sep 20, 2019

annevk commented Sep 23, 2019

littledan commented Sep 23, 2019

jakearchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

jakearchibald left a comment

Choose a reason for hiding this comment

littledan commented Jan 23, 2020

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

jakearchibald commented Jan 18, 2021

jakearchibald left a comment

Choose a reason for hiding this comment

Ms2ger left a comment

Choose a reason for hiding this comment

Ms2ger commented Jan 20, 2021

mfalken left a 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

codehag commented May 21, 2021

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

mfalken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfalken commented Jun 11, 2021

Ms2ger commented Jun 25, 2019 •

edited by pr-preview bot

Loading

annevk commented Jun 27, 2019 •

edited

Loading

jakearchibald commented Jun 28, 2019 •

edited

Loading

jakearchibald left a comment •

edited

Loading

littledan Jan 22, 2020 •

edited

Loading