Allow the addRoutes() to be called during installing

[yoshisatoyanagisawa]

This is a fix for #1742 and #1743.

Currently, it is technically possible to call addRoutes() outside of the install event. This change follows how FetchEvent.respondWith() deal with the case, and prevents the addRoutes() method to be called outside of the install event.
At the same time, the event wait until a promise returned by addRoutes() resolves.

---

Preview | Diff

[yoshisatoyanagisawa]

@domenic @sisidovski Will you review this? Note that Chromium code has not followed the change yet.

[youennf]

I have some questions:

1. Should addRoutes be callable only synchronously within the event handler or while the event is active (aka while the service worker is installing)?
2. Should addRoute execution delay installation of the service worker?
3. Should addRoutes promise rejection trigger automatic failure of the service worker or should it be let to the website?

My preference so far:

1. Allow calling addRoutes while the event is active.
2. Have addRoute execution delay installation of the service worker.
3. Leave failure of the installation up to the website.

Based on this, I would tend to keep the current API signature as is, addRoutes(RouterRule or sequence).

[yoshisatoyanagisawa]

I have some questions:

1. Should addRoutes be callable only synchronously within the event handler or while the event is active (aka while the service worker is installing)?

Do you mean that the while the service worker is installing is that the state is "installing"?
Considering the install algorithm, the state sounds somewhat large.

Will you help me to understand corner cases that bring difference between two?

2. Should addRoute execution delay installation of the service worker?

It should be. Or, there might be ambiguity on how the subsequent navigation will be handled.
With the rules or without the rules.

3. Should addRoutes promise rejection trigger automatic failure of the service worker or should it be let to the website?

Current Chromium implementation allows addRoutes() to fail, and it does not bring ServiceWorker failure. Note that we allow addRoutes() to be called multiple times.

The thought behinds are:

1. unsupported condition or source may return promise rejection, and it can be a way for a web developer to understand a supported SW static routing features for each browsers. (The way to add a new condition or a new source support. WICG/service-worker-static-routing-api#28)
2. avoid allowing a broken third party library to break the site using ServiceWorker completely.

My preference so far:

1. Allow calling addRoutes while the event is active.

As written above, let me ask details.

2. Have addRoute execution delay installation of the service worker.
3. Leave failure of the installation up to the website.

I think we agree on both 2. and 3.

Based on this, I would tend to keep the current API signature as is, addRoutes(RouterRule or sequence).

Sure. I will work on that tomorrow.

[youennf]

In general, I think we want to allow adding routes before the service worker information is stored persistently.
This storage step happens in Safari when moving from installing to installed, so it is ok to call addRoutes until then.

I also think this might be handy for web developers.
With the current PR, the first addRoute call would success but not the second:

oninstall = async e => {
   const promise = cache.open('test');
   e.waitUntil(promise);
   e.addRoute(...); // would resolve.
   await promise;
   e.addRoute(...); // would reject.
   // At this point, service worker is still installing, but https://w3c.github.io/ServiceWorker/#installation-algorithm steps 13 and after will be soon executed to proceed with storing scripts and so on. So service worker will move from installing to installed.
}

Also, importScripts can be called successfully while service worker is installing.
And it might be handy to allow addRoutes from these imported scripts.

[yoshisatoyanagisawa]

Thanks for the comment. I tried to make the returning promise lives with the event, I hope that is what you meant.

[youennf]

Changes look good.
A few thoughts:

• Before resolving the promise, we need to queue a task to actually resolve the promise in the service worker thread.
• In theory, the in parallel steps could fail (say quota error), in which case the installation of the service worker would fail if we reject the addRoutes promise. One possibility would be to call add lifetime promise with another promise that gets resolved as soon as the addRoutes promise gets settled. This other promise would never be rejected.
• With this PR, addRoutes can only succeed if called synchronously in the install event handler (point 1 of above discussion). Wdyt about this point?

[domenic]

This feels like it might be a little racy. Perhaps you should use the parallel queue infrastructure whenever you modify the list of router rules? Or maybe even when you access it as well?

I am curious how this maps to the implementation.

[yoshisatoyanagisawa]

Sure.
I tried to fix that by using ServiceWorkerEventLoop.

In Chromium, mojo should be handled sequentially and it actually executed one by one as far as I understand.

[youennf]

This is not exactly what we want here.
|serviceWorker|'s list of router rules are used when matching a fetch, which happens out of process.
The usual way of handling this is to use [=in parallel=] to do manipulation out of process.
And we queue a task to actually go back to the service worker execution context.

This step could be replaced with something like:

1. [=In parallel], run the following substeps:

And then, at the end of these substeps:

1. [=Queue a task=] to run the following steps on |serviceWorkerEventLoop| using the [=DOM manipulation task source=]
    1. Resolve |promise| with undefined.

[yoshisatoyanagisawa]

Let me ask on this more. I got confused for this.

To allow third-party libraries to set routes, addRoutes() can be called multiple times. Also, to avoid ambiguity on the evaluation order, we want to force the rules to be evaluated in the added order.
Then, what happens if addRoutes() is called multiple times with the suggested process? I am afraid its execution order is not guaranteed.

Since it says [=in parallel=], I suppose one procedure can be executed while the other is executed. If multiple addRoutes() are called at once, there can be race condition and rules added at the same time may not be lost? That is my intention to make a task to do followings:

1. get the existing rule.
2. add the new rules.
3. write back to |serviceWorker| [=list of router rules=]

Or, can we expect the deterministic execution order for [=in parallel=]?

[youennf]

Right, we definitely want to preserve determinism.
The solution is probably to use a parallel queue, like https://w3c.github.io/webcodecs/#codec-work-queue, and use it.

That said, this is an existing issue in the service worker spec, see cache algorithms for instance.
I would go with a comment stating that it is expected that the order of the rules is preserved.

[yoshisatoyanagisawa]

Thanks. I tried to revised it to use the parallel queue.

[youennf]

If using the parallel queue, you need to queue a task on the service worker event loop to resolve the promise. We are not expected to resolve promises from a parallel queue.

[yoshisatoyanagisawa]

sure.

[yoshisatoyanagisawa]

• Before resolving the promise, we need to queue a task to actually resolve the promise in the service worker thread.

I guess I got the same comment from @domenic and I hope the recent one resolves this.

• In theory, the in parallel steps could fail (say quota error), in which case the installation of the service worker would fail if we reject the addRoutes promise. One possibility would be to call add lifetime promise with another promise that gets resolved as soon as the addRoutes promise gets settled. This other promise would never be rejected.

Thanks for the suggestion. I tried to add the yet another promise for it.

• With this PR, addRoutes can only succeed if called synchronously in the install event handler (point 1 of above discussion). Wdyt about this point?

Will you suggest the way to extend that during the installing?

[youennf]

Will you suggest the way to extend that during the installing?

All that might be needed is to remove the check on the event's dispatch flag on this PR.

The add to lifetime call does two things:

• Extend the installing phase until addRoutes completes.
• Reject/throw if service worker is no longer installing.

[yoshisatoyanagisawa]

Will you suggest the way to extend that during the installing?

All that might be needed is to remove the check on the event's dispatch flag on this PR.

sure.

[youennf]

LGTM.
I think we still need to work on the in parallel/queue a task, but this can be done as part of #1742.

It would be useful to add some WPT tests:

• addRoutes after installing.
• The order of route validation might be slightly different after the PR (in case one route is invalid and the other has a fetch source but there is no registered fetch event handler).
• Call addRoutes several times and verify ordering is preserved when matching a fetch load.

[yoshisatoyanagisawa]

Yes. WPTs will come later.

@domenic Will you take another look at this?

[yoshisatoyanagisawa]

@monica-ch Can I ask you to take a look?

[yoshisatoyanagisawa]

@youennf can I ask you to clarify the followings? Or, will you show some snippets that make things different?

The order of route validation might be slightly different after the PR (in case one route is invalid and the other has a fetch source but there is no registered fetch event handler).

The PR clearly separated the verification and the update. However, a process before the PR still returns promise reject on the verify failure without updating the |serviceWorker|'s [=service worker/list of router rules=]. Therefore, I still think addRoutes() is atomic. i.e. if there is a invalid rule, addRoutes() just fails and [=service worker/list of router rules=] won't be updated.
I suppose the rules order still preserved.

Stepping back, do you suggest to add WPT that failing addRoutes() are just ignored?
i.e. addRoutes() invalid and addRoutes() valid are called in this order, the ServiceWorker should respect the latter addRoutes(). You might suggest to add WPTs for this.

[monica-ch]

@yoshisatoyanagisawa Do you have an opened chromium issue to track the code changes in chomium per the spec changes?

[yoshisatoyanagisawa]

@yoshisatoyanagisawa Do you have an opened chromium issue to track the code changes in chomium per the spec changes?

Sure. I opened the issue for this:
https://issues.chromium.org/issues/393165254

[yoshisatoyanagisawa]

Also, I have updated the title because in spite of my first PR, addRoutes() is allowed during installing.

[youennf]

The order of route validation might be slightly different after the PR

This is a minor point really.

What I am meaning is that, before the PR, we would do:

• Iterate through the routes to validate them, reject as needed
• Iterate through the routes to check route source and fetch event, reject as needed.

With the PR, it does:

• Iterate through the routes:
  • If route is not validated, reject
  • If route source is fetch-event and no fetch event is registered, reject

In both cases, TypeError is used, but the error message is likely different.

[yoshisatoyanagisawa]

The order of route validation might be slightly different after the PR

This is a minor point really.

What I am meaning is that, before the PR, we would do:

• Iterate through the routes to validate them, reject as needed
• Iterate through the routes to check route source and fetch event, reject as needed.

With the PR, it does:

• Iterate through the routes:

  • If route is not validated, reject
  • If route source is fetch-event and no fetch event is registered, reject

In both cases, TypeError is used, but the error message is likely different.

Got it. Yeah, TypeError will be thrown in both cases, but which one to verify first has been changed. Since this is a specification, the verification order should be mattered. I had to hone that feeling.