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

Top-level await integration for ServiceWorkers running modules #1407

Open
littledan opened this issue May 19, 2019 · 51 comments · May be fixed by #1444

Comments

@littledan
Copy link

commented May 19, 2019

The top-level await proposal would make module evaluation result in a Promise, rather than synchronous success or failure. This would affect the Update algorithm, which takes the steps in 16.1 if there is such a failure.

Does anyone have such concerns about this evaluation being asynchronous?

If this seems alright, then I'll make sure we have a PR for ServiceWorker after top-level await reaches Stage 3. It looks like there would be a little bit of refactoring on the SW side, so I'll wait for things to settle down editorially on the TC39 side before making more churn with dependent specs.

cc @domenic @guybedford @MylesBorins

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

I don't see any technical problems adding top-level await. Just a couple of adjustments like you mentioned. However, service workers that use top-level await would be considered bad practice.

When we boot up an active service worker, it's for an event. Sometimes the event is performance sensitive, such as "fetch". Awaiting on anything that isn't needed for TTFB is pretty bad.

So we either:

  1. Allow it since it's part of JavaScript, but provide guidance against it, and maybe warn on the console.
  2. Disable it. Although it kinda feels odd to disable JavaScript features in a particular context like this.

I'm leaning towards 2. What do you think @slightlyoff?

@wanderview

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Top level await on a module that is offlined probably is no worse than importScripts(). But top-level await on arbitrary async code could be detrimental.

Also, top-level await would not be protected by a waitUntil(). In general we probably couldn't automatically keep it alive because then it would become an abuse vector.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Also, top-level await would not be protected by a waitUntil(). In general we probably couldn't automatically keep it alive because then it would become an abuse vector.

I think we could work around that. We know what event we're starting up for, and we could take any start up time away from that event when/if we get that far. Similar to if you've got a sync infinite loop or some other expensive code at the top level.

@littledan

This comment has been minimized.

Copy link
Author

commented May 20, 2019

Top-level await could be useful for certain kinds of module loading, where subresources like dynamically determined JavaScript modules may be important for the code to run. Some of this can be expressed declaratively, e.g., with import-maps, but other cases are more complex.

@wanderview

This comment has been minimized.

Copy link
Member

commented May 20, 2019

where subresources like dynamically determined JavaScript modules may be important for the code to run

This sort of thing can be a bit of a footgun in service workers, unfortunately. It may work the majority of the time, but it increases the chance they will try to dynamically load a resource that is not offlined when the device does not have network. This is why importScripts() that hit network are only permitted during the install phase and otherwise throw immediately in service workers.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Dynamic module loading can be done with:

const modulePromise = import(useFoo ? './foo' : './bar');

addEventListener('fetch', event => {
  event.respondWith(async function() {
    const { whatever } = await modulePromise;
    //
  }());
});

This feels better all round, as you won't be blocked on the import if your code doesn't use it.

@littledan

This comment has been minimized.

Copy link
Author

commented May 20, 2019

Well, thanks for explaining. I think we were hoping that top-level await would be available everywhere (e.g., a deep dependency may use TLA without its users having to worry about it), but it sounds like we may have to rethink that.

@wanderview

This comment has been minimized.

Copy link
Member

commented May 20, 2019

const modulePromise = import(useFoo ? './foo' : './bar');

Didn't we agree to block dynamic import() for now in #1356 (comment)?

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

"for now", but we should enable it once we figure out the right way to allow it. I still think the behaviour we have with importScripts is good, so we'd try to make import() match that.

@domenic

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

I would like to advocate for "let the user do the bad thing if they want". Remember, top-level await is largely sugar over what would be accomplishable today. (Powerful sugar, because of how it allows dependencies to abstract things away, but still.)

We don't prevent people from doing the following:

(async () => {
  await delay(100);
  self.onfetch = e => ...;
})();

and we should similarly not prevent them from doing the following:

await delay(100);
self.onfetch = e => ...;
@annevk

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Isn't there a difference between those snippets? In that the first won't handle the incoming fetch event (needs to be handled synchronously as the event is dispatched after parsing) and the second will, but with an unintended delay? The second would be much harder to debug.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

And we do prevent people from doing:

(async () => {
  await delay(100);
  addEventListener('fetch', event => …);
})();
@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

// 1
import './this-has-top-level-await';

// 2
const modulePromise = import('./this-has-top-level-await');

// 3
await modulePromise;

In a service worker, 1 and 3 are 'bad' because they delay important events. However, 2 is fine.

If we choose to disallow 1 & 3, can we still allow 2? It provides a path to including third party libraries that use top-level-await.

@domenic

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Isn't there a difference between those snippets?

No, they are identical, as far as I can tell.

And we do prevent people from doing:

How is this prevented? I think the same mechanism for prevention should apply to top-level await (instead of a novel mechanism like failing to parse the script).

@wanderview

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

I believe its prevented in step 1 here:

https://dom.spec.whatwg.org/#add-an-event-listener

@littledan

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

Thanks for the context, @wanderview. Maybe all we need to do, then, is ensure the "has ever been evaluated" flag is set synchronously, not asynchronously. This way, we would throw a TypeError if the event listener is added after a top-level await is reached. We can permit top-level await to queue further things to run, but at the same time, not wait for the module graph to complete the async parts of its execution before continuing the installation. In a way, this would match script tags with top-level await (where there is no way to wait on that evaluation either).

This would mean, though, that if you add a WebAssembly module into your dependency graph in some ways, your ServiceWorker would encounter an error after it was initially installed. Maybe this would be very bad, broken behavior that we want to avoid earlier.

@Jamesernator

This comment has been minimized.

Copy link

commented Jun 17, 2019

I think there's a false association that async implies slow. A large number of use cases of top-level await (or WASM modules) are likely to only be expensive on first register as things will likely be in various caches on successive loads.

I think it would be fine to just allow whatever and instead add a couple new performance entry types like 'service-worker-first-load'/'service-worker-successive-load' that measures the speed that services workers loaded in response to first register/successive loads.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

@Jamesernator

I think there's a false association that async implies slow.

Nah, the assumption is that in-series is slower than in-parallel.

const asyncResult = await doAsyncThing();

addEventListener('fetch', (event) => {
  if (asyncResultNeeded) {
    //
  }
});

Vs:

const asyncResultP = doAsyncThing();

addEventListener('fetch', async (event) => {
  if (asyncResultNeeded) {
    const asyncResult = await asyncResultP;
    //
  }
});
@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@littledan

Maybe all we need to do, then, is ensure the "has ever been evaluated" flag is set synchronously, not asynchronously.

But if we were booting up the service worker to fire a fetch event, wouldn't our dispatch of the event be blocked on all top-level awaits?

@littledan

This comment has been minimized.

Copy link
Author

commented Jun 18, 2019

@jakearchibald I'm suggesting that we don't set up such blocking mechanics, and instead, just launch execution of the module and go right on our way in setting up the ServiceWorker. Concretely, in the Run Service Worker algorithm, in step 12, we'd just run the module, take the Promise that it returns, and more or less discard it, continuing on our way with the setup.

There's a small amount of complexity with errors during module execution: Although errors are communicated via Promises, the JS specification knows about synchronous errors synchronously, in a way that doesn't break Promise invariants, and we could thread through an API to query this state, or you could just cheat and check if the Promise was rejected. If there's an error after a top-level await, that'd come via an onerror event "later", after the Service Worker is already running.

Does this setup make sense to you? Or do you see some reason why we'd still have to block?

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Yeah, this feels like it could work. I don't think it makes top-level await useful to a service worker, but it seems like an ok way to work around it.

It doesn't prevent:

const modulePromise = import('./this-has-top-level-await');

…which is great.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@littledan thank you for working through this!

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

For TPAC:

Thoughts:

No one allows modules in service workers yet.

Developers may end up doing things like awaiting state from IDB on each service worker load. The worst gotchas would be async things that are sometimes very slow, but may not show up in dev due to different conditions.

However, developers can currently spin a while loop for seconds as part of service worker start up, and we haven't lost too much sleep over that. Similar things might happen when importing large libraries that do a lot of setup work.

We haven't blocked a JS feature in service worker before. Although, we did block XHR, partially because of sync XHR.

Option 1: Allow top level await as is. Service worker initialisation would be blocked on the main script fully executing, including awaited content. We'd advocate against it using it, and perhaps fire warnings in devtools.

Option 2: Ban it. The service worker would throw on await, so it wouldn't install.

Option 3: Allow top level await, but the service worker is considered ready once execution + microtasks are done. That means the awaits will continue to run, but events may be called before the script has 'completed'. Adding events after execution + microtasks is not allowed, as currently defined.

The worry about 3 is some of the changes of behaviour that could be unwittingly introduced by a third party library #1444 (comment).

@asutherland

This comment has been minimized.

Copy link

commented Sep 15, 2019

I like option 1. In #1331 and related issues like #1157 and requests for exposure of localStorage to ServiceWorkers (!) the issue of data available synchronously-ish at SW startup has come up several times. I know I've at least suggested that async APIs should still be used in this case and we will work on making the async APIs more performant.

To me, this means that:

  • Yes, top-level async awaits should be possible so async storage can be used. (And this is preferable to exposing localStorage, etc.)
  • We have a better chance of creating accurate and useful data-preload hints if we are able to know these storage uses are part of top-level evaluation and there's no chance for contamination from "fetch" events interleaving with the top-level evaluation.

Also, option 3 seems hard to reason about and explain to humans.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

Mostly me, @annevk, @domenic, @n8schloss.

Thanks for looking at the spec side of things. Just to clarify, we do want dynamic imports to be able to use top-level await.

Ms2ger added a commit to tc39/proposal-top-level-await that referenced this issue Sep 19, 2019
This is part of the solution to <w3c/ServiceWorker#1407>.
Ms2ger added a commit to whatwg/html that referenced this issue Sep 19, 2019
This is part of the solution to <w3c/ServiceWorker#1407>.
Ms2ger added a commit to littledan/html that referenced this issue Sep 20, 2019
This is part of the solution to <w3c/ServiceWorker#1407>.
@ljharb

This comment has been minimized.

Copy link

commented Sep 21, 2019

The current proposed solution would fork the language's two parse goals - Script and Module - into three: Script, Module, and ModuleWithoutAwait (ie, "Service Worker Module"). This will create an enormous burden on tooling - linting/static analysis and transpilers in particular - an ecosystem which has already endured much burden by the addition of the Module parse goal.

I think that any syntax that parses in a Module should parse in every Module, and I'm not clear on why this is a necessary solution for Service Workers. Can someone tl;dr me the above very long thread and help me understand why creating a third parse goal for the JS language is a necessary step here?

@Jamesernator

This comment has been minimized.

Copy link

commented Sep 23, 2019

The current proposed solution would fork the language's two parse goals

It's not really a new parse goal, all modules parse with TLA, it's just that service workers will reject any modules have a dependency that is marked [[Async]]: true, presumably this includes WebAssembly modules too as they depend on TLA architecture.

Regardless I think TLA should still be allowed in the entry for service workers simply because it can turn what could possibly be a simple change into a large one if it needs to propagate all the way to the entry file.

@syg

This comment has been minimized.

Copy link

commented Oct 9, 2019

Having SWs ban [[Async]]: true modules feels like a fine solution but I think still requires more formal ecma262 allowance. Whatever happened with Anne's original suggestion?

I'd be happy to propose adding a [[AllowAsyncModule]] bit to agents (a la the [[CanBlock]] bit for Atomics.wait). The spirit of both are the same: different agents have different constraints around the sync-ness of computation, whether it be indefinite blocking or async modules.

@devsnek

This comment has been minimized.

Copy link

commented Oct 9, 2019

The difference with [[CanBlock]] is that it creates runtime errors. There's no way to gracefully degrade TLA. You just have to not use it.

What if service workers just never support modules? That would force people to use import() and avoid creating a third parse goal.

@syg

This comment has been minimized.

Copy link

commented Oct 9, 2019

[[AllowAsyncModule]] can raise a runtime error during evaluation of the top-level await in an agent where that bit is false, no? To the extent that erroring out on a call to Atomics.wait is considered graceful degradation, this feels the same to me. I'm really not seeing the third parse goal argument here.

Never supporting modules seems a lot more drastic... that seems like the wrong tradeoff?

@ljharb

This comment has been minimized.

Copy link

commented Oct 9, 2019

If there exists three sets of syntax that are allowed to parse, that's three parse goals. At the moment, there's two. Put another way, if TLA works in any Module, it should work in every Module - or else there's two kinds of Modules (thus, three parse goals)

@syg

This comment has been minimized.

Copy link

commented Oct 9, 2019

@ljharb An agent flag allows TLA to be parsed everywhere. I'm proposing a runtime error allowance.

@ljharb

This comment has been minimized.

Copy link

commented Oct 9, 2019

The distinction for the developer between an early parse error, and an unconditional runtime error in a service worker context, seems pretty close to zero to me.

@syg

This comment has been minimized.

Copy link

commented Oct 9, 2019

To clarify a point of possible confusion when discussing with @devsnek on chat: I'm not proposing the agent flag prevents any code with a TLA in SW module contexts from running, but that the evaluation of a TLA would consult this agent flag, at runtime, and throw.

@jakearchibald

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

Btw, I'd be happy with a solution that causes this to fail later than parse time.

  1. Execute script.
  2. If the script has pending promises added via top level await, fail.

This might be closer to how we fail scripts that peg the CPU.

The requirement is that we can reliably fail service worker install for scripts that use top-level await.

@littledan

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

I'm a bit confused by the above discussion. The reason that the PR checks [[Async]] rather than making an agent flag that's passed down to JS is that such an agent flag would have to be checked by each thing that creates a module. But there are different specifications that do that--ECMA-262 and the Wasm/JS API, for one. Also, it's unclear how to make an agent flag context-dependent, in the sense of flagging the first import but allowing later dynamic import.

@annevk

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

On IRC Daniel explained to me that my agent-annotation suggestion works poorly architecturally for disabling top-level await, but allowing top-level await for dynamic imports. That's why whatwg/html#4352 places the decision at module "creation".

And failing at parse time was apparently hard to get agreement on in TC39.

My remaining concern here is editorial, about inspecting [[PromiseState]] (see #1444 (comment)), but that is something we can evolve over time and doesn't really affect the model so shouldn't be a blocking concern.

Hope that helps alleviate some of the confusion. It's at least a bit clearer for me again.

@littledan

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

We'll follow up on this editorial issue. It's still unclear exactly what would be best, to ensure that other users of modules don't need excessively complex logic to handle Promises and completion values, while not inspecting Promise internals here.

@syg

This comment has been minimized.

Copy link

commented Oct 9, 2019

Also, it's unclear how to make an agent flag context-dependent, in the sense of flagging the first import but allowing later dynamic import.

That's a good point. I was imagining the the flag being mutable at runtime, which admittedly isn't like any of the other per-agent flags, which are all constant.

That's why whatwg/html#4352 places the decision at module "creation".

Ah, that's what I was missing. The check for [[Async]] currently is at module "creation" time already? In that case I withdraw my agent flag suggestion.

@devsnek

This comment has been minimized.

Copy link

commented Oct 9, 2019

I'd suggest Perform ? HostCanAwaitAtTopLevel(current realm Record, GetActiveScriptOrModule()). You could also put a flag on the module and do If_ GetActiveScriptOrModule().[[AllowTLA]] is *false*, throw a TypeError exception.

@littledan

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

@devsnek I'd rather not take this suggestion, because of the context-dependence issue with static vs dynamic import described above.

@devsnek

This comment has been minimized.

Copy link

commented Oct 10, 2019

@littledan, that's why I suggested it, the VM can freely choose whether or not to allow it at each point. from there, html can say to allow X module but not Y module.

@littledan

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

@devsnek How would the context information be passed to/from HTML, about whether it comes from a dynamic or static import? This isn't a function of the realm or script, but rather the spec algorithm that invokes the module import.

@devsnek

This comment has been minimized.

Copy link

commented Oct 10, 2019

@littledan the same way html provides an implementation of HostPromiseRejectionTracker i assume. and it's called in the evaluation for AwaitExpression.

@littledan

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

@devsnek I'm still having trouble understanding. I also don't understand what the issue is with the phrasing @Ms2ger and I collaborated on. If you have a concrete suggestion, can I suggest you make spec PRs for it, the way we have PRs out for the [[Async]] strategy?

@devsnek

This comment has been minimized.

Copy link

commented Oct 10, 2019

@littledan perhaps when i find some time...

the idea @syg and i came up with is basically instead of failing the entire graph, just make a top level await expression itself throw. Since an await can resume abruptly already, this doesn't introduce weird new semantics or the appearance of new parse goals.

@littledan

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

Oh, I missed that you were talking about shifting this to when the TLA is hit. I worry that that would be less predictable and regular than rejecting all async modules. If we want to prevent top-level await in modules, I'd prefer we reject the module graph ahead of time, rather than when the await expression runs. In general, the semantics of TLA have been careful to generally treat modules which have a TLA which is not dynamically hit as "async modules", taking e.g., an extra microtask turn.

@devsnek

This comment has been minimized.

Copy link

commented Oct 10, 2019

@littledan i'd rather have to discuss ideas about the timing than ideas about rejecting the entire graph though. if we want to reject the entire graph I fail to understand why we're bringing module goal to service workers at all.

@littledan

This comment has been minimized.

Copy link
Author

commented Oct 12, 2019

Note that we already concluded the discussion about the design of top-level await when we promoted it to Stage 3. Any changes from here could be proposed for consensus, and would generally be expected to be based on implementation experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.