Add abort reason to abort fetch

[nidhijaju]

Following on from https://github.com/whatwg/dom/pull/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):
  • …
  • …
• [x] Tests are written and can be reviewed and commented upon at:
  • https://github.com/web-platform-tests/wpt/pull/35374
• [ ] Implementation bugs are filed:
  • Chrome: …
  • Firefox: …
  • Safari: …

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

---

Preview | Diff

---

Preview | Diff

[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 https://github.com/web-platform-tests/wpt/pull/6484#issuecomment-315775251 for a great explanation by @jakearchibald. There's additional context in #523 and https://github.com/w3c/ServiceWorker/pull/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.

[annevk]

@nidhijaju I think most dependent PRs have landed now. Are you willing to push this over the finish line? Perhaps @noamr can help as he worked on the underlying infrastructure quite a bit.

[noamr]

Now that #1329 is merged, I guess we can put the abort reason on the fetch controller, which is available in handle fetch when it aborts its descendant fetches. It would then have to be serialized to the new realm as discussed in comments on this issue. I don't think the whole request needs to be serialized for this reason, though maybe it does for other reasons.

[noamr]

Regarding serialization, what makes sense to me is that when the reason has to be sent across realms (when handle fetch aborts its internal fetches), perform the equivalent of a structuredClone on reason since it can be any JS object.

Perhaps the controller should keep both the cloned and regular version of the reason, pass the real one if it's the same client and a cloned one to service workers (this needs WPTs!)

[nidhijaju]

@annevk I've made some changes to this PR and started a service worker PR as well (https://github.com/w3c/ServiceWorker/pull/1655). Would you be able to take a look?

[noamr]

So this PR seems to be halfway between the simple version and the complex version.

The simple version here ignores service workers. For that, I think you don't need any serialization/deserialization; in fact, I don't think you need the "cloned error" concept at all. You just need the changes you made to fetch(), where if the signal is already aborted, or if the abort steps for the signal are triggered, you reject the returned promise with the signal's reason, instead of with an "AbortError" DOMException.

The complex version involves updating the request.signal.reason seen in a service worker, when the main page does controller.abort(reason) from outside the service worker. This requires more plumbing than what you have here; in particular, you need to specify how the message gets sent across realms.

Why is that necessary? The aborted error is a result of serialization, so by definition it's transferrable between realms, and the main question is when is it serialized. We don't specify how fetch messages the service worker realm to abort in more detail than that today.

I don't understand @noamr's recommendations about how to use the controller for this. In particular I don't understand how we go from "abort a fetch controller with a reason JavaScript object" to "post a message to the service worker realm containing the Request representing this request, to signal abort on that Request's signal using a deserialization of that request". Maybe he can suggest how this would best be done...

I think I might have been or still am confused about how this is observable. @nidhijaju, as I said WPTs would go a long way here to explain. Maybe I was proposing a solution for only part of the problem - which is aborting the navigation preload and I was missing a totally separate aspect.

[domenic]

Why is that necessary? The aborted error is a result of serialization, so by definition it's transferrable between realms, and the main question is when is it serialized. We don't specify how fetch messages the service worker realm to abort in more detail than that today.

I'm unsure what you're asking about. Are you questioning the goal of updating request.signal.reason for the service worker realm's Request object? That seems to be what people were indicating a desire for in, e.g., https://github.com/whatwg/fetch/pull/1343#issuecomment-960474944 and preceding messages.

Or are you asking why you need plumbing to do this? Well, there's nothing in the current PR which updates that value, so that's why I suggested adding spec text to do so...

[noamr]

Why is that necessary? The aborted error is a result of serialization, so by definition it's transferrable between realms, and the main question is when is it serialized. We don't specify how fetch messages the service worker realm to abort in more detail than that today.

I'm unsure what you're asking about. Are you questioning the goal of updating request.signal.reason for the service worker realm's Request object? That seems to be what people were indicating a desire for in, e.g., #1343 (comment) and preceding messages.

Or are you asking why you need plumbing to do this? Well, there's nothing in the current PR which updates that value, so that's why I suggested adding spec text to do so...

You answered my question, I indeed guided @nidhijaju to solve the wrong problem, though the fetch part of the solution holds.

I believe what we need to do something like the following:

• In fetch, serialize the reason immediately when abort(reason) is called, and save realm-agnostic result in the controller as this PR does.
• In handle fetch, create an AbortController, and when we create the Request object, associate it with that AbortController's signal
• If the fetch is aborted, also abort the created controller with the fetch controller's serialized abort reason ("cloned error" in this patch). WDYT?

[domenic]

I think that's about right! I didn't realize the fetch controller was shared with the handle fetch algorithm; indeed, with that fact, it becomes clearly the right coordination point.

Minor comment:

In handle fetch, create an AbortController, and when we create the Request object, associate it with that AbortController's signal

Right now in handle fetch, when we create a Request object in step 24.3.2, we never set its signal. But later in 24.3.19, we treat the signal as if it exists. So that's bad. Probably we should update 24.3.2 to use https://fetch.spec.whatwg.org/#request-create so that an AbortSignal is properly created.

Also, in spec land, we don't need to create an AbortController. We can just mess with the signals directly. So just that update should suffice.

So in summary, I think the changes look like this:

• Change "abort fetch" to accept an error (like this patch)
• Change "abort" for fetch controllers to accept an error (similar to this patch):
  • When given a JavaScript object, it should serialize it before storing it in the controller. undefined inputs, or inputs that cause serialization to throw, should get turned into serializations of a new "AbortError" DOMException.
• Update the call sites for "abort" for fetch controllers to pass an error appropriately:
  • cancelAlgorithm should be updated to take the reason and pass that
  • "Add the following abort steps" should grab signal's reason and pass that.
• Rename this patch's "cloned error" to "serialized abort reason" for clarity, and change its type to "null or a serialization from StructuredSerialize".
• Update Service Worker spec:
  • Change 24.3.2 to use "create a Request"
  • Change 24.3.19 to deserialize and use controller's serialized abort reason if it is non-null and deserializing doesn't throw or return undefined; otherwise a "AbortError" DOMException.

(Note: the above summary was edited after first being posted, please make sure you're looking at the latest!)

[nidhijaju]

Thank you, Noam and Domenic! I wasn't quite sure how to get the abort reason for cancelAlgorithm, but I've tried to incorporate all other feedback into the latest changes. The ServiceWorker spec PR (https://github.com/w3c/ServiceWorker/pull/1655) has also been updated, and the WPTs (https://github.com/web-platform-tests/wpt/pull/35374) should also be complete now.

Could you take a look to see if this looks better?

[noamr]

Thank you, Noam and Domenic! I wasn't quite sure how to get the abort reason for cancelAlgorithm, but I've tried to incorporate all other feedback into the latest changes. The ServiceWorker spec PR (w3c/ServiceWorker#1655) has also been updated, and the WPTs (web-platform-tests/wpt#35374) should also be complete now.

Could you take a look to see if this looks better?

To me it does look better, thanks! Added comments to both PRs.

[nidhijaju]

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

[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!