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

When should "imported scripts updated flag" be unset? #1021

Closed
jakearchibald opened this issue Dec 7, 2016 · 19 comments
Closed

When should "imported scripts updated flag" be unset? #1021

jakearchibald opened this issue Dec 7, 2016 · 19 comments

Comments

@jakearchibald
Copy link
Contributor

importScripts has two behaviours. One where it fetches from the network and adds to its "script resource map", and another where it fetches only from the "script resource map".

This allows importScripts during initial execution to implicitly populate the cache while the service worker is not-active, then later work without the network by using this cache.

The spec flips the switch once install has completed, but @mattto and @wanderview say implementations flip the switch after the first synchronous run of the service worker script.

Should the spec match implementations?

@jakearchibald
Copy link
Contributor Author

I quite like the currently specced behaviour, as it means I can do something like this:

addEventListener('install', event => {
  importScripts('heavy-script.js');
});

addEventListener('fetch', event => {
  const url = new URL(event.request.url);

  if (url.pathname == '/whatever/') {
    importScripts('heavy-script.js');
    event.respondWith(heavyThing());
  }
});

I can cache some heavy scripts, but avoid the cost of executing them on each service worker startup. I can take that hit only when I need it.

Does it make sense to do this? We should think about this in a world with import().

@wanderview
Copy link
Member

Note, we've had a gecko bug to implement this for over a year, but I was never sure if it was really desired:

https://bugzilla.mozilla.org/show_bug.cgi?id=1201887

@jungkees
Copy link
Collaborator

jungkees commented Dec 8, 2016

Our thought flow over the discussion has been:

  1. No sync API should be allowed in SW, and importScripts() is one. So, let's allow it only during install events: Make importScripts() fail outside install #730
  2. (1) prevents even the cached imported scripts from being executed in the later events. So, let's remove this restriction: importScripts() should throw? #642 (comment)

(2) sort of assumed that a specific service worker version and its (cached) imported script resources should map 1:1. So, it currently sets the flag upon a successful install.

@mattto and @wanderview say implementations flip the switch after the first synchronous run of the service worker script.

This is a question about until when we should allow fetching the scripts from the network: either until the first SW run during an Update or until the successful install of a SW version.

In addition to that, #839 (comment) should be discussed in this thread as well - whether we should allow the imported scripts to run in async tasks or not. @jakearchibald's example above suggests a usage where cached imported scripts can be used in async tasks while offline.

@jakearchibald
Copy link
Contributor Author

Yeah, I think dynamic importing is useful, and probably more likely once import() lands.

@wanderview
Copy link
Member

Personally I'm inclined to keep importScripts() as a sync script eval thing only right now. Reasons:

  1. Async importScripts() is a complication for an already complicated piece of code.
  2. Its unclear how much devs will feel they need to risk janking startup of the SW which is off the main thread. I personally have not seen people clamoring for this yet.
  3. Browsers can minimize the impact by storing pre-compiled js for offlined scripts. So the real impact is mainly felt at update/install time.
  4. Nested dedicated workers would allow developers to explicitly move work (including eval of heavy scripts) out of the service worker into a separate thread. I think this is something we want for other use cases and it could potentially solve this use case as well.
  5. Its easier to allow install event importScripts() later if we get a lot of feedback from devs that its needed.

@jakearchibald
Copy link
Contributor Author

Its easier to allow install event importScripts() later if we get a lot of feedback from devs that its needed.

Hmm, although it's really difficult to feature-detect this.

@wanderview how do you feel about import()? Would you want that to enable just-in-time importing?

@wanderview
Copy link
Member

Well, if we want to support import(), yes we would need to add async loading. But import() will require additional work regardless.

I guess it comes down to what people expect to actually implement. If we keep the spec text that allows importScripts() until the end of install, is chrome going to actually implement it? Neither chrome nor firefox has implemented it in the last year. AFAIK no one has complained.

@jungkees
Copy link
Collaborator

jungkees commented Dec 12, 2016

On reflection, I'm leaning toward allowing it only until the initial eval. AFAICT, the purpose of the byte-for-byte check is to examine whether it needs to continue to Install or not. If the spec changes to allow keeps allowing it until the end of install, what do we do with the byte-for-byte check for? When it comes to import(), I think the dynamically imported scripts should not be considered as the service worker script resources determining a version. WDYT?

EDIT: This reasoning is under the decision of #839 (comment).

@jungkees
Copy link
Collaborator

@mattto pointed in #839 (comment) that my previous comment is not true. I agree that the byte-by-byte check for Install is orthogonal to the discussion of the timing of setting the flag.

@jakearchibald
Copy link
Contributor Author

I think dynamic imports are useful, but given that:

  • browsers don't implement it
  • it's sync
  • no one's asking for it

…I'm happy for us to align the spec with browsers for importScripts. However, I think dynamic importing is going to become popular with import(), and we should support it there.

We could achieve this by allowing import() to go via the fetch event. Maybe that's the less magic solution?

@wanderview
Copy link
Member

We could achieve this by allowing import() to go via the fetch event. Maybe that's the less magic solution?

I don't think that would work if import() is called before the install event. Or if the script needs code from the import() in order to handle fetch events.

A general problem will be how to handle the race between import() async modules loading and life cycle events. I guess the most straightforward way to handle that would be for script to use import() in an install event so it can use waitUntil() to guarantee the load completes.

Still, I feel like that can be handled at a later time. I'd like to see import() ship on window and regular workers first.

@jakearchibald
Copy link
Contributor Author

I don't think that would work if import() is called before the install event.

Yeah, we'd have to come up with some rule there, like it goes to the network before activate.

A general problem will be how to handle the race between import() async modules loading and life cycle events. I guess the most straightforward way to handle that would be for script to use import() in an install event so it can use waitUntil() to guarantee the load completes.

Basically what the spec says for importScripts now?

@wanderview
Copy link
Member

wanderview commented Dec 12, 2016

Basically what the spec says for importScripts now?

Kind of. But in this future world someone could call import() at script eval time and it could finish loading after the install event completes. The "complete install" steps basically will need to have a check for any outstanding async import loads and fail install in those cases. Since we can't just throw from import().

Really I don't like this, though, because it could work fine in a fast local dev environment and then fail later in a slower production environment. Seems like a footgun we should try to avoid if we can.

Edit: Maybe we could make install completion wait until all outstanding import() loads are complete.

@jungkees
Copy link
Collaborator

With #1021 (comment) and #1021 (comment), I'm also happy for us to align the spec with the implementations.

One thing I'd like to check with the current implementations is whether they don't even allow to get a script from the cache when importScripts() is called from within an async task.

@jungkees
Copy link
Collaborator

With a simple test code:

self.oninstall = e => {
  importScripts('importedscript1.js');
};

self.onfetch = e => {
  importScripts('importedscript2.js');
};

Both importedscript1.js and importedscript2.js execute (without a network error) in both Firefox and Chrome without importScripts() to those urls in the top-level script. I'm confused. This is not expected to be working according to what @wanderview and @mattto mentioned? Am I misunderstanding?

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Dec 13, 2016

Very interesting @jungkees!

Here's an expanded test, and live demo.

Results:

Test Spec Firefox Chrome
Root import (first run) Download, execute & cache As spec'd As spec'd
Root import (later runs) Fetch from cache & execute As spec'd As spec'd
Install event import Download, execute & cache Downloads, executes, but does not cache As spec'd
Uncached fetch event import Throw error Fetch from network Fetch from network

So I withdraw "I'm happy for us to align the spec with browsers for importScripts", since the browsers aren't particularly aligned. Also, allowing synchronous network fetches in functional events (which both browsers do) seems like a really bad idea.

Aligning to the spec seems like a better idea.

@jakearchibald
Copy link
Contributor Author

@wanderview

Maybe we could make install completion wait until all outstanding import() loads are complete

We don't do this for things like cache.addAll so why should we do it here? Devs should do:

import whatever from "./thing-i-always-need.js";

addEventListener('install', event => {
  event.waitUntil(
    import('./thing-i-sometimes-need.js')
  );
});

@wanderview
Copy link
Member

We don't do this for things like cache.addAll so why should we do it here?

Mainly because I wasn't aware the syntax made it easy like this. Works for me.

dougt pushed a commit to dougt/chromium that referenced this issue Jan 10, 2017
ServiceWorkerContextRequestHandler should emit a network error instead
of falling back to network when it cannot handle a request. Otherwise, a
service worker can be spawned that did not load via our custom
ServiceWorkerWriteToCacheJob/ServiceWorkerReadFromCacheJob jobs,
resulting in a running worker whose ServiceWorkerVersion has not
been properly initialized. I suspect this can cause the bug 485900.

This patch:
- Changes network fallback for failure cases to an ERR_FAILED network
  error.
- As an exception, an installed worker loading an unstored script still
  results in network fallback. This should be deprecated and removed
  eventually, see w3c/ServiceWorker#1021.
- Changes the behavior for a new worker loading an already stored script
  (i.e., calling importScripts() for the same script multiple times).
  Before this patch, we would fallback to network for this script. Now,
  we read the stored script. This is not yet codified in the spec but is
  expected to have almost no real-world impact and has support on
  w3c/ServiceWorker#1041

BUG=485900,678899

Review-Url: https://codereview.chromium.org/2602853002
Cr-Commit-Position: refs/heads/master@{#442590}
jelmansouri pushed a commit to jelmansouri/chromium-net that referenced this issue Mar 7, 2017
ServiceWorkerContextRequestHandler should emit a network error instead
of falling back to network when it cannot handle a request. Otherwise, a
service worker can be spawned that did not load via our custom
ServiceWorkerWriteToCacheJob/ServiceWorkerReadFromCacheJob jobs,
resulting in a running worker whose ServiceWorkerVersion has not
been properly initialized. I suspect this can cause the bug 485900.

This patch:
- Changes network fallback for failure cases to an ERR_FAILED network
  error.
- As an exception, an installed worker loading an unstored script still
  results in network fallback. This should be deprecated and removed
  eventually, see w3c/ServiceWorker#1021.
- Changes the behavior for a new worker loading an already stored script
  (i.e., calling importScripts() for the same script multiple times).
  Before this patch, we would fallback to network for this script. Now,
  we read the stored script. This is not yet codified in the spec but is
  expected to have almost no real-world impact and has support on
  w3c/ServiceWorker#1041

BUG=485900,678899

Review-Url: https://codereview.chromium.org/2602853002
Cr-Original-Commit-Position: refs/heads/master@{#442590}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 815dc48c9d00b0142a2421de8f10b7cb9c5b34ab
@jungkees
Copy link
Collaborator

We didn't get to a conclusion in this thread, but I close this in favor of #1319.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants