w3c / ServiceWorker

Service Workers
https://w3c.github.io/ServiceWorker/
Other
3.63k stars 316 forks source link

Top-level await integration for ServiceWorkers running modules #1407

Closed littledan closed 3 years ago

littledan commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

Didn't we agree to block dynamic import() for now in https://github.com/w3c/ServiceWorker/issues/1356#issuecomment-433411852?

jakearchibald commented 5 years ago

"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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

And we do prevent people from doing:

(async () => {
  await delay(100);
  addEventListener('fetch', event => …);
})();
jakearchibald commented 5 years ago
// 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 commented 5 years ago

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 commented 5 years ago

I believe its prevented in step 1 here:

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

littledan commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@littledan thank you for working through this!

jakearchibald commented 4 years ago

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 https://github.com/w3c/ServiceWorker/pull/1444#issuecomment-506655298.

asutherland commented 4 years ago

I like option 1. In https://github.com/w3c/ServiceWorker/issues/1331 and related issues like https://github.com/w3c/ServiceWorker/issues/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:

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

jakearchibald commented 4 years ago

Resolution:

littledan commented 4 years ago

@jakearchibald Thanks for this! We'll get a spec PR out for this resolution shortly. (I imagine it'd be with a grammar parameter for modules to ban TLA, that's used in these cases.) Just for context, who was present in the discussion to determine this resolution?

jakearchibald commented 4 years ago

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.

ljharb commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

[[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 commented 4 years ago

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 commented 4 years ago

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

ljharb commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 https://github.com/whatwg/html/pull/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 https://github.com/w3c/ServiceWorker/pull/1444#issuecomment-534103889), 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

devsnek commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

littledan commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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.