Never fail for navigation requests

[NekR]

I suggest to never fail navigation requests. If fetch events' respondWith fails (passed promise rejected) perform hidden/background request to original URL without informing SW about (SW cannot intercept it).
And for better debugging this situation, write a warning to console about what happened (browsers performed request on its own).

This will prevent web developers (like me) from making mistakes in SW which leads to web site being blocked from loading forever.

I know that browsers has to update SW after 24 hours anyway, bug Firefox had bug when it doesn't and when caches.match() fails (rejected) because somehow cache was deleted. Latest fix (46.0.1) didn't help, so I suggest to make fundamental change here.

[jakearchibald]

I know that browsers has to update SW after 24 hours anyway,

That's a maximum, if you've set your SW to cache beyond 24 hours. Otherwise it'll update per navigation.

What does failure mean in this situation? A rejected promise? A never-resolving promise? Resolving with a blank response? Resolving with a 404 response? etc etc

My first reaction is that we shouldn't do this, just as we don't reload a page without JavaScript if a JavaScript error occurs.

[NekR]

That's a maximum, if you've set your SW to cache beyond 24 hours. Otherwise it'll update per navigation.

We do not set any cache headers to SW file. And it doesn't update in Firefox.
Fail means rejected promise as I wrote in parentheses.

Thing is, website can stuck forever if I do this:

e.responseWith(caches.match(...));

or just have any JS error in next then().

Of course I do not mean that browser should perform another request if e.respondWith() was cased with Response/Promise<Response>.

[wanderview]

It should update. I just added a test case for this case. The test sets no-cache, though. I'd have look to see what HTTP cache behavior is without any cache control headers.

[jakearchibald]

We do not set any cache headers to SW file

Best not to leave it to heuristics, and set Cache-Control: no-cache.

Fail means rejected promise as I wrote in parentheses.

So that wouldn't "fix" problems with event.respondWith(cache.match(…)) if it resolved with undefined. Or event.respondWith(new Promise(() => {})).

Thing is, website can stuck forever if I do this e.responseWith(caches.match(...));

It shouldn't be. Trying to call e.responseWith will throw since it isn't a function, and the handling of the event will be aborted, and the request will be handled as if SW wasn't involved.

My attitude to this so far has been "Well, if developers screw up their server code, they users get a broken (or no) experience, why should service worker be different?", but I guess different browsers and evolving support makes it different.

[wanderview]

On my mobile, but pretty sure a throwing fetch event handler is treated like a network error. I don't think it's supposed to restart the network request.

[NekR]

Trying to call e.responseWith will throw since it isn't a function

Hmm.. it's just typo if you do not get.

why should service worker be different?

Because I always update my server unless it physically down. With SW browsers behaves on its own as in everything in browser. But in some areas browser is allowed "to be smart" and browser devs say "we shouldn't expose this API to web devs, browser should optimize/handle", and here you say the opposite. Funny thing browser, it doesn't magic when browser devs think it would be cool if browser will be doing such magic.

Okay guys, nevermind.

[jakearchibald]

@NekR cool down. If you read at what I wrote:

but I guess different browsers and evolving support makes it different.

…I'm making it clear that I'm open to the differences here. We need to be very careful with "smartness" here, since it often ends up doing something the developer didn't want. I hear that you want it, but we need to consider if it's ok as a general rule.

[wanderview]

One thing I've considered is bypassing cache on update if the registration has has any failed interceptions since last update check.

[wanderview]

Also heuristics that disable service worker interception for a registration if there have been N interception failures. That is trickier, though, and could mask problems.

[jakearchibald]

@wanderview I think we need to look at this more closely. Looking at the handle fetch spec, it seems like a synchronous error, before calling respondWith will result in it returning "null" back to fetch spec.

It looks like the fetch spec doesn't handle null being returned, but it needs to, as this is what happens if no fetch events are called, or respondWith isn't called.

We need to figure out what we want to happen if:

• An error is thrown in the fetch listener and respondWith is not called (at the moment, I think the request will proceed to network)
• An error is thrown in the fetch listener and respondWith is called (at the moment, this depends on the value passed to respondWith)
• The resolved value passed to respondWith is not a response (at the moment, network error)
• The promise passed to respondWith rejects (at the moment, network error)

I think there are legitimate response to return errors/null to subresource requests. Eg, I might have a route that is only served by caches.match, and if there's no match I expect the image load to fail, or my fetch() call to reject.

"Rescuing" navigations is a bit magical, and I wouldn't expect it to happen with devtools open, but it might rescue non-developer users. Right now I can't think of cases where you'd want a navigation to fail, but there might be. This could be done per navigation, or maybe even disable fetch events for a particular service worker if it continually fails (this would be per service worker, so the next installed SW would not have fetches disabled).

[wanderview]

Step 18.6.1 of Handle Fetch aborts the the task steps if a runtime script error occurs:

 5) Dispatch e at activeWorker’s environment settings object’s global object.
 6) For each event listener invoked:
    1) If any uncaught runtime script error occurs, then:
        1) Report the error for the script per the runtime script errors handling.
        2) Abort these steps. 

It then says after step 18.6:

If task is discarded or the script has been aborted by the termination of activeWorker, set handleFetchFailed to true.

I believe previously the spec used the term "aborted" there instead of "task is discarded", but I think it applies.

if handleFetchFailed is true then we return a network error in step 21.

[wanderview]

I guess step 20 checks if respondWithEntered is false before step 21 runs. That's very confusing to me. Because step 20.1 returns network error if you call event.preventDefault(), but not if you throw?

Hmm, we added test cases for this but I don't see them in the blink tree yet:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/fetch-cors-xhr-iframe.html#69

Edit: I don't see any blink tests that exercise fetch event throwing.

[wanderview]

I'll split the throwing question into a separate issue later today. Sorry for the tangent.

[wanderview]

Re-reading Jake's comment, I guess we want to discuss this here.

So firefox and chome act differently for these two cases. Both get something wrong compared to the current spec:

addEventListener('fetch', function(evt)) {
  throw('boom');
});

Firefox treats this as a network error. Chrome continues the network request. Chrome conforms to the current spec text here:

addEventListener('fetch', function(evt)) {
  evt.respondWith(new Response('hmm'));
  throw('boom');
});

Firefox treats this as a network error. Chrome continue the network request. Firefox conforms to the current spec text here.

Chrome screenshot here:

So I guess we do need to figure out what we want to do and write some tests.

[delapuente]

1. An error is thrown in the fetch listener and respondWith is not called (at the moment, I think the request will proceed to network)
2. An error is thrown in the fetch listener and respondWith is called (at the moment, this depends on the value passed to respondWith)
3. The resolved value passed to respondWith is not a response (at the moment, network error)
4. The promise passed to respondWith rejects (at the moment, network error)

I would fail on 3 and 4 and perhaps retry bypassing the worker on 1 and 2. From the programmable proxy perspective of the service worker, if that piece does not work, let the request continue to the network although if I'm explicitly passing a wrong type to the respondWith, let it fail. In any case, on failure, call onerror() for the ServiceWorkerGlobalScope.

[jakearchibald]

V1 because this is a breaking change, if we do indeed change.

Developers I've spoken to consider this "safe fallback to network" behaviour magical, and aren't keen. However, I'm in two minds about it.

[delapuente]

What about something like:

self.onfetch = evt => {
  evt.safeNavigation = true; // make the navigation request to proceed to the network in case of failure.
};

Or:

self.addEventListener('fetch', evt => {
}, { safeNavigation: true });

[wanderview]

Is this really that different from just using .catch()? They are both explicit and opt-in.

self.addEventListener('fetch', evt => {
  evt.respondWith(myActualWork().catch(e => {
    // fallback to network always
    fetch(evt.request);
  });
});

[delapuente]

The declarative syntax could mark the listener somehow to tell the VM about this desired behavior but apart from this, I suppose there is no functional difference.

[jakearchibald]

Pre F2F notes: I'm keen to hear from MS (and possibly Apple) about this, as they haven't shipped. I don't think opt-in is worth it, as .catch can do it.

[jakearchibald]

F2F:

• Lots of developers aren't keen on this
• Sounds like something browsers can do, eg provide a button on the failure page "Retry without SW" (worded in a way that users understand). Or if it keeps failing, the browser could auto unregister the SW