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

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

Open
wanderview opened this issue Sep 29, 2020 · 12 comments
Open

Comments

@wanderview
Copy link
Member

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
Copy link
Member Author

@jakearchibald @annevk do you have opinions here?

@annevk
Copy link
Member

annevk commented Sep 30, 2020

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member

annevk commented Sep 30, 2020

See also whatwg/fetch#153.

@wanderview
Copy link
Member Author

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

ItalyPaleAle added a commit to ItalyPaleAle/prvt that referenced this issue Oct 10, 2020
There's currently no way for a service worker to know if a request has been canceled. This includes understanding if the user skipped forward in a video, for example.
To avoid returning the entire file every time, we're limiting Range requests from audio/video tags to 2MB chunks only.

See: w3c/ServiceWorker#1544
@annevk
Copy link
Member

annevk commented Nov 19, 2021

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
Copy link

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
Copy link

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
Copy link

paralin commented May 7, 2024

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
Copy link

Lcfvs commented Sep 24, 2024

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

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

No branches or pull requests

7 participants