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

cache.addAll() is not consistent in aborting outstanding requests on error #1543

Open
wanderview opened this issue Sep 28, 2020 · 2 comments

Comments

@wanderview
Copy link
Member

In the cache.addAll() algorithm:

https://w3c.github.io/ServiceWorker/#cache-addAll

The spec periodically cancels outstanding requests with text like:

Terminate all the ongoing fetches initiated by requests with the aborted flag set.

However, it does not appear to do this if the fetch() rejects instead of resolving a promise. It also does not do this if a resolved Response has ok() return false, status code is 206, etc.

Also, does this kind of termination cause the associated Request.signal to expose the abort status? I'm having a hard time understanding that from the spec. @jakearchibald do you know?

@wanderview
Copy link
Member Author

I'd like to write some cache.addAll() abort WPT tests and being able to test abort status on the signal would make things a lot easier.

@wanderview
Copy link
Member Author

I guess if cache.addAll() aborts internally it is not exposed to the script's AbortSignal. It seems the Request constructor always creates a new AbortSignal which then follows the original signal. This means the signal flows into the new Request, but if we abort the new Request signal internally the info does not flow back out to the script.

So I'm not sure if its really possible to test if outstanding requests are cancelled when one request in addAll() fails.

blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Sep 30, 2020
The spec requires that requests be aborted on addAll() failure:

https://w3c.github.io/ServiceWorker/#cache-addAll

See the sections that say "Terminate all the ongoing fetches
initiated by requests with the aborted flag set."  Note, there
appear to be a few places where this text is missing as noted in:

w3c/ServiceWorker#1543

Currently there is no way to reliably observe that this abort takes
place in a WPT test.  The AbortSignal is not updated for internal
abort decisions and the information is not propagated to the signal
on a service worker FetchEvent.request.  Therefore this test only adds
a simple unit test to verify the related AbortController was triggered.

Bug: 1130781
Change-Id: I0e0498741a6d387147a2ef0ea9d31feaa805dee8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429308
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812273}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
The spec requires that requests be aborted on addAll() failure:

https://w3c.github.io/ServiceWorker/#cache-addAll

See the sections that say "Terminate all the ongoing fetches
initiated by requests with the aborted flag set."  Note, there
appear to be a few places where this text is missing as noted in:

w3c/ServiceWorker#1543

Currently there is no way to reliably observe that this abort takes
place in a WPT test.  The AbortSignal is not updated for internal
abort decisions and the information is not propagated to the signal
on a service worker FetchEvent.request.  Therefore this test only adds
a simple unit test to verify the related AbortController was triggered.

Bug: 1130781
Change-Id: I0e0498741a6d387147a2ef0ea9d31feaa805dee8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429308
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812273}
GitOrigin-RevId: d6d55147aa1276bb6815e520b71b6c8277ece517
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

1 participant