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 Cache#addAll reject with AbortSignal's reason if the signal is already aborted? #1684

Open
CYBAI opened this issue Jun 16, 2023 · 3 comments

Comments

@CYBAI
Copy link
Contributor

CYBAI commented Jun 16, 2023

Based on https://dom.spec.whatwg.org/#dom-abortsignal-abort, when a signal is aborted, the abort reason would be set to the given reason or a new AbortError DOMException.

Currently, in Step 5-7-3-1 in https://w3c.github.io/ServiceWorker/#dom-cache-addall, it says

If response’s aborted flag is set, reject responsePromise with an "AbortError" DOMException and abort these steps.

so it will always reject the response promise with an AbortError DOMException.

Should it reject with the signal's abort reason instead?

Note: In fetch method, in Step 4-1, when the signal is already aborted, fetch will reject with the signal's reason.

@annevk
Copy link
Member

annevk commented Jul 26, 2023

Hey @CYBAI, thanks for raising this!

It seems that none of addAll() accounts for the fact that each request can be aborted. I agree that it would make sense to adhere to the same logic as fetch() uses and propagate the first exception thrown.

@shaseley can you review this for Chromium?

@asutherland can you review this for Gecko?

@asutherland
Copy link

It seems that none of addAll() accounts for the fact that each request can be aborted. I agree that it would make sense to adhere to the same logic as fetch() uses and propagate the first exception thrown.

Agreed. This is also consistent with the naming of Promise.all() which also rejects on the first rejection with the first rejection reason. (Versus Promise.allSettled which will provide all rejections.)

@shaseley
Copy link
Contributor

Oh, looks like we're not using the input Request object signals at all? Good catch; +1 propagating the first exception/abort reason, whether from an associated signal or other reason.

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

4 participants