Add abort reason to abort fetch

[nidhijaju]

Following on from whatwg/dom#1027, this PR uses the AbortSignal's abort reason in the abort fetch algorithm.

@annevk Does this look okay to you?
/cc @yutakahirano, @domenic

• At least two implementers are interested (and none opposed):
  • …
  • …
• Tests are written and can be reviewed and commented upon at:
  • Fetch: Add tests for AbortSignal's abort reason web-platform-tests/wpt#35374
• Implementation bugs are filed:
  • Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1371359
  • Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1793736
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=246069

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

---

Preview | Diff

[annevk]

Unfortunately I think this integration is more complex and it might be best to build on top of @noamr's #1329.

In particular, the problem is this line:

Terminate the ongoing fetch with the aborted flag set.

That in turn is used to cancel the stream with an "AbortError" deep in the overall fetch algorithm. What we want to do instead is use the reason there.

[annevk]

error is always given I think, but it can be undefined. I think the text here and in Streams suggest we should have this primitive in DOM.

Something like

To get an error for an abort reason, given a JavaScript value reason, run these steps:

1. If reason is undefined, then return a new "AbortError" DOMException.
2. Return reason.

What do you think?

[annevk]

An alternative might be to pass the signal on and then we define

The error of an AbortSignal object signal is ...

That might make it a bit easier to use. I.e., callers can say "throw signal's error".

[saschanaz]

But can it ever be undefined? It seems both AbortController#abort and static AbortSignal.abort sets the reason if undefined.

[yutakahirano]

Unfortunately I think this integration is more complex and it might be best to build on top of @noamr's #1329.

In particular, the problem is this line:

Terminate the ongoing fetch with the aborted flag set.

That in turn is used to cancel the stream with an "AbortError" deep in the overall fetch algorithm. What we want to do instead is use the reason there.

I think that's not needed because the response body is errored in https://whatpr.org/fetch/1343.html#abort-fetch, and this PR already gives the abort reason to the procedure. The ongoing fetch is terminated but the error objects in "if aborted" steps don't matter as long as #abort-fetch is already called.

[annevk]

Ah yes, see also #1187. It doesn't make a whole lot of sense to me that we have two places to cover the same requirement. And the place this PR is addressing it in is the place I propose we remove to avoid redundancy.

[annevk]

@smaug---- raised an interesting question. What happens if the signal enters a service worker? We'd have to serialize/deserialize the reason, right? That's also why the "Terminate the ongoing fetch with the aborted flag set." aspect is important here.

cc @domenic @jakearchibald

[domenic]

What happens if the signal enters a service worker? We'd have to serialize/deserialize the reason, right?

That seems ideal, and kind of nice as a developer communication channel. But, the plumbing might get pretty complicated, and it might not be worth the extra spec/implementation/test work. The alternative is just saying that cross-realm aborting is special and we censor any abort reasons to "AbortError" DOMExceptions.

At a first glance that would require storing the serialized form of the fetch error on the request (?) object, and then in places like https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm step 24.3.19 deserializing and using that to signal abort.

As an example of the kind of fun we'd have to deal with, recall that deserialization can fail if there's not enough memory to allocate the appropriate ArrayBuffer. So we'd have to handle that case.

[ricea]

What happens if the signal enters a service worker? We'd have to serialize/deserialize the reason, right?

That seems ideal, and kind of nice as a developer communication channel. But, the plumbing might get pretty complicated, and it might not be worth the extra spec/implementation/test work. The alternative is just saying that cross-realm aborting is special and we censor any abort reasons to "AbortError" DOMExceptions.

What we do for transferring abort reasons in streams is that we try to serialise them, but if the platform doesn't know how to serialise it we end up with an empty object. Since ECMAScript knows how to serialise Error types this works reasonably well for streams. But somehow it seems worse to end up with an empty object as an abort reason in fetch. So I have a preference for censoring it to an AbortError on transfer in fetch. I don't know what that would look like in standards language.

[annevk]

I guess when either serialization or deserialization ends up throwing we can pass on undefined and then do the undefined -> new "AbortError" DOMException trick. It would be nice if this was aligned with Streams though.

I'm not entirely sure how the plumbing for this would work and that's probably due to the message passing that needs to happen being rather implicit. Thoughts:

1. I guess we need to store it on request. This would also allow the fetch algorithm itself to act on it better. (Ideally we can refactor XMLHttpRequest's timeout stuff to use this underneath.)
2. Then when the request is cloned for the service worker, we need some version of https://dom.spec.whatwg.org/#abortsignal-follow that does cross-thread following. Cross-thread following would be the same as same-thread following, except that it does the serialize/deserialize dance described above. And we'll not worry about the message passing aspects and how they get a handle to each other across threads...

cc @MattiasBuelens

[MattiasBuelens]

I think the cleanest way to handle this is making Request as a whole [Transferable]:

• Request.signal would be transferred using AbortSignal's transfer steps.
• Request.body would be transferred using ReadableStream's transfer steps.

To transfer an AbortSignal, we could use some sort of cross-thread message channel. For example, we could construct a MessageChannel, attach an abort algorithm to send a message containing the abort reason through one end, and then call "signal abort" with that reason when we receive it on the other end. This is similar to how we currently transfer streams. (And, also similar to streams, we'll have to be careful around "double-transferring" an AbortSignal. 😛)

Making Request transferable also allows us to clear up some vagueness around how we pass streams through service workers:

• Right now, Service Worker's handle fetch says:

  24.3.2 Let requestObject be a new Request object associated with request and a new associated Headers object whose guard is "immutable".

  However, request's body may be a ReadableStream created in a different realm. It's never specified how this stream should be accessed from within the workerRealm. Really, we should transfer the entire Request to the worker's realm, including its signal and body.

• When Fetch's HTTP fetch receives a non-null response from the service worker (in step 5.5), it should transfer it from the worker's realm to the current realm.

---

But somehow it seems worse to end up with an empty object as an abort reason in fetch. So I have a preference for censoring it to an AbortError on transfer in fetch. I don't know what that would look like in standards language.

I would find it weird if transferring Request and then accessing .signal would behave different from transferring Request.signal directly. 😕

I guess we could not make Request and AbortSignal transferable from user-land code, and only provide dedicated algorithms to "transfer a request" and to "transfer-receive a request". Then we could do some Fetch-specific censoring in there.

I guess when either serialization or deserialization ends up throwing we can pass on undefined and then do the undefined -> new "AbortError" DOMException trick. It would be nice if this was aligned with Streams though.

It won't necessarily throw. You may still get back an empty object, because it's merely serializing all the own properties.

class MyError {
  get message() {
    return "Oops!";
  }
}
const original = new MyError();
original.message; // -> "Oops!"
const clone = structuredClone(original);
clone.message; // -> undefined

[annevk]

I think in the case where serialization and deserialization succeed we shouldn't temper with signal.reason (i.e., it should be possible for it to be an "empty" object). But when they don't I think we have to temper with it in some form, so we might as well use "AbortError" DOMException. That seems better than "DataCloneError" DOMException. (I guess we should test an explicit "DataCloneError" DOMException, that ought to work. 😊)

I'm not sure about adding public APIs for serializing/transferring Request, but anything we do should be compatible with that happening at some point.

[yutakahirano]

What happens if the signal enters a service worker? We'd have to serialize/deserialize the reason, right?

Is this (i.e., aborting without reason) currently specified? https://fetch.spec.whatwg.org/#http-fetch handles a request, but AbortSignal is not on it so I don't know how to tell the fetch is aborted to the service worker.

[annevk]

It's not well-defined at the moment, but it's intended to "work". See web-platform-tests/wpt#6484 (comment) for a great explanation by @jakearchibald. There's additional context in #523 and w3c/ServiceWorker#1178. The gist of it is that it is based upon the hand-wavy termination concept and the "aborted flag".

Now, #1329 should improve that setup, but looking at this again it seems that service workers need more attention there. That is, how do we pass the state on to service workers? Perhaps "handle fetch" should get a controller as well?

[yutakahirano]

In that case can we keep being hand-wavy about service workers for now, and fix it later altogether?

[annevk]

Note that currently you can infer from the text what is supposed to happen, albeit not in a great way. E.g., it is somewhat clear the AbortSignal that is exposed in the service worker ends up aborted due to a document invoking abort(). To what extent would it be clear what that AbortSignal's abort reason ends up being if we don't change the language at all? Only tests?

[annevk]

@yutakahirano also note that @noamr is making great progress on #1329 and per our discussion on Matrix we have a general idea of how to forward the termination to service workers. After which this should be relatively straightforward.

[yutakahirano]

Note that currently you can infer from the text what is supposed to happen, albeit not in a great way. E.g., it is somewhat clear the AbortSignal that is exposed in the service worker ends up aborted due to a document invoking abort(). To what extent would it be clear what that AbortSignal's abort reason ends up being if we don't change the language at all? Only tests?

My mental model for the service worker interception is that you are transferring the request from the initiator context to the service worker. Hence it feels natural that request.signal is aborted when abortController.abort() is called, and the abort reason is also cloned or transferred, with some edge cases. I'm not sure if this matches with your definition of inference though.

@yutakahirano also note that @noamr is making great progress on #1329 and per our discussion on Matrix we have a general idea of how to forward the termination to service workers. After which this should be relatively straightforward.

If #1329 will be landed soon I'm fine with waiting for the change.

[domenic]

LGTM with nits, very nice!

[nidhijaju]

Hi @annevk, would you be able to take a look at this when you have time?

[annevk]

Thanks so much for pushing this forward! I think there are still some things to improve with how fetch() and the fetch algorithm interact, but overall this is looking rather good.

[annevk]

Note to self: might want to add a source comment here about this being a Record.

[annevk]

Thank you! I'd appreciate another look from @domenic and @noamr (if available) and I think we should address the null-vs-undefined issue and we should probably introduce a shared algorithm given that we do the same thing three times, but otherwise this seems good to go.

[domenic]

LGTM with nit!

[annevk]

Apologies, my last feedback round was misguided. There was no reason to switch from null to undefined as the null is not exposed to JS. And as @domenic noted serialization and deserialization are different operations so at most two algorithms could share more code.

So I've reverted the null to undefined change. I have implemented some code sharing for deserialization and as part of that I found a bug. The networking layer doesn't have a "this" so for now I've gone with an implementation-defined realm and a comment that we really need to clean all of that up in the future.

I think it's good to go now though. Anyone any final feedback on this? It seems the tests are ready to land and I think we can assume implementation support as this was always part of the plan.

[nidhijaju]

I think it's good to go now though. Anyone any final feedback on this? It seems the tests are ready to land and I think we can assume implementation support as this was always part of the plan.

Looks good to me -- thank you, @annevk!
Yes, the service worker and test PRs are ready to land, and I've also filed and linked the implementation bugs to the PR description.

[noamr]

I found some time to look at it again, another LGTM From me.

[annevk]

Thanks all!