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

Add abort reason to abort fetch #1343

Merged
merged 31 commits into from
Oct 5, 2022
Merged

Conversation

nidhijaju
Copy link
Contributor

@nidhijaju nidhijaju commented Oct 27, 2021

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

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


Preview | Diff


Preview | Diff

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated
Comment on lines 7506 to 7507
<li><p>If <var>error</var> is not given, let <var>error</var> be an
"<code><a exception>AbortError</a></code>" {{DOMException}}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yutakahirano
Copy link
Member

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

annevk commented Oct 27, 2021

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

annevk commented Oct 29, 2021

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

domenic commented Oct 29, 2021

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

ricea commented Nov 1, 2021

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

annevk commented Nov 1, 2021

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

MattiasBuelens commented Nov 1, 2021

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

annevk commented Nov 1, 2021

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

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

annevk commented Nov 2, 2021

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

yutakahirano commented Nov 2, 2021

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

@annevk
Copy link
Member

annevk commented Nov 2, 2021

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

annevk commented Nov 3, 2021

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

yutakahirano commented Nov 4, 2021

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 added a commit that referenced this pull request Nov 9, 2021
Follow-up: integrating abort reason is #1343.

Fixes #1349.
annevk added a commit that referenced this pull request Nov 9, 2021
Follow-up: integrating abort reason is #1343.

Fixes #1349.
andreubotella pushed a commit to andreubotella/deno that referenced this pull request Dec 16, 2021
The spec PR for this (whatwg/fetch#1343) hasn't been finalized yet
because some questions regarding service worker integration are not
settled yet, but everything else seems settled, and it seems best to
not wait for another Deno version to support abort reasons in the fetch
API.
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits, very nice!

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
Co-authored-by: Domenic Denicola <[email protected]>
@nidhijaju
Copy link
Contributor Author

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

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fetch.bs Outdated
@@ -231,6 +231,9 @@ lt="authentication entry">authentication entries</a> (for HTTP authentication).

<dt><dfn for="fetch controller">report timing steps</dfn> (default null)
<dd>Null or an algorithm accepting a <a for=/>global object</a>.

<dt><dfn for="fetch controller">serialized abort reason</dfn> (default null)
<dd>Null or a serialization from [$StructuredSerialize$].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nit!

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Oct 4, 2022

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

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

noamr commented Oct 5, 2022

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

@annevk annevk merged commit c9039b9 into whatwg:main Oct 5, 2022
@annevk
Copy link
Member

annevk commented Oct 5, 2022

Thanks all!

nidhijaju added a commit to web-platform-tests/wpt that referenced this pull request Oct 6, 2022
Add test cases to check the functionality of AbortSignal's abort reason when aborting fetch, including serialization and that the service worker can observe the reason.

See whatwg/fetch#1343 for accompanying spec changes.
@nidhijaju nidhijaju deleted the update-with-abortreason branch October 6, 2022 00:47
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 21, 2022
… reason, a=testonly

Automatic update from web-platform-tests
Fetch: Add tests for AbortSignal's abort reason (#35374)

Add test cases to check the functionality of AbortSignal's abort reason when aborting fetch, including serialization and that the service worker can observe the reason.

See whatwg/fetch#1343 for accompanying spec changes.
--

wpt-commits: 0e5f85c08e05fb1b9b67fb12c4d7c0de77a4ee9b
wpt-pr: 35374
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 26, 2022
… reason, a=testonly

Automatic update from web-platform-tests
Fetch: Add tests for AbortSignal's abort reason (#35374)

Add test cases to check the functionality of AbortSignal's abort reason when aborting fetch, including serialization and that the service worker can observe the reason.

See whatwg/fetch#1343 for accompanying spec changes.
--

wpt-commits: 0e5f85c08e05fb1b9b67fb12c4d7c0de77a4ee9b
wpt-pr: 35374
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants