w3c / ServiceWorker

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

should FetchEvent.request.signal reflect abort status of outer request? #1544

Open wanderview opened 4 years ago

wanderview commented 4 years ago

Consider a service worker script that looks like:

  self.addEventListener('fetch', evt => {
    evt.respondWith(fetch(evt.request));
  });

And that the controlled page does the following:

  const controller = new AbortController();
  fetch(url, { signal: controller.signal });
  if (some_condition) {
    controller.abort();
  }

Should the fetch() initiated by the service worker script be aborted in this case? I think it would be good to do so.

I'm unsure this is what the spec says, though. It seems that in Handle Fetch we create a Request from an inner request in step 21.3.2:

https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm

The fetch spec, however, does not have an abort signal on the inner request:

https://fetch.spec.whatwg.org/#concept-request

Its only present on the exposed Request object:

https://fetch.spec.whatwg.org/#request-signal

This implies that Handle Fetch effectively strips the AbortSignal from the request when generating FetchEvent.request. Is that intentional?

wanderview commented 4 years ago

@jakearchibald @annevk do you have opinions here?

annevk commented 4 years ago

I would not mind exposing and forwarding the signal, but at that point you have a (potentially) cross-process signal bridge. It seems that would require some new language to define properly.

I definitely think we should expose a fetch being terminated in some fashion to the service worker and there are existing open issues on that (probably also filed by you). E.g., I don't think the server worker knows if fetches are getting terminated when a document is unloaded.

wanderview commented 4 years ago

I don't see that a cross-process signal bridge is a problem as long as we make updating the service worker exposed signal async.

The other problem I have with the signal infrastructure is that it only exposes when the signal itself is aborted, but not when the fetch is stopped/aborted for internal reasons.

wanderview commented 4 years ago

E.g., I don't think the server worker knows if fetches are getting terminated when a document is unloaded.

If we surfaced internal abort reasons on the exposed signal it would satisfy this case.

FWIW, the context for why I was looking at this was that I wanted to write a test showing cache.addAll() aborted outstanding requests when one failed. But it seems impossible to do so in WPT right now.

jakearchibald commented 4 years ago

Should the fetch() initiated by the service worker script be aborted in this case? I think it would be good to do so.

My intention was 'yes', but maybe I spec'd it wrong. Fwiw, I have tests for this, but they weren't merged. Who should I nudge about this? https://github.com/web-platform-tests/wpt/pull/7674/files

annevk commented 4 years ago

See also https://github.com/whatwg/fetch/issues/153.

wanderview commented 4 years ago

We tried to make this work in firefox a few years ago, but looks like it never landed: https://bugzilla.mozilla.org/show_bug.cgi?id=1394102

annevk commented 2 years ago

It seems from #1620 that the specification does have some integration already, albeit it a little vague. E.g., is the cancelation only forwarded if it has happened by the time we reach the step that forwards it? Why not signal it sooner if it happened sooner? It also seems that we should probably not forward anymore once respondWith() is fulfilled?

JakobJingleheimer commented 2 years ago

Piping in with my 2¢: it's very unexpected and undesirable for a ServiceWorker to not only be unable to abort a request but also that the AbortSignal is silently ignored / fails silently.

Is there any chance of this moving forward?

hedgehog125 commented 1 year ago

Is there any update on this? I've been thinking of experimenting with offline video/audio streaming and it's looking like I'll need some weird workarounds at the moment. Do you think the handling for browser triggered aborts will be implemented at the same time as AbortSignals? At least in the spec?

paralin commented 5 months ago

It seems like this is still relevant: when I open a new tab and browse to a ServiceWorker-handled URL with a long-lived request, and then close that tab, the request.signal does not seem to be aborted in the ServiceWorker.

Lcfvs commented 1 month ago

It seems like this is still relevant: when I open a new tab and browse to a ServiceWorker-handled URL with a long-lived request, and then close that tab, the request.signal does not seem to be aborted in the ServiceWorker.

Another basic problem, with long-lived requests but without the need to close the tab: the closed EventSource requests

It seems from #1620 that the specification does have some integration already, albeit it a little vague. E.g., is the cancelation only forwarded if it has happened by the time we reach the step that forwards it? Why not signal it sooner if it happened sooner? It also seems that we should probably not forward anymore once respondWith() is fulfilled?

I'm also self-asking about another idea than to reflect the signal abortion... what about to resolve/reject (optionally with a value) on the respondWith()'s promise