Re: [whatwg/fetch] Add abort reason to abort fetch (PR #1343)

@annevk commented on this pull request.

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.

> @@ -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$].

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

> @@ -254,8 +257,20 @@ given a <a>fetch controller</a> <var>controller</var>:
 </ol>
 
 <p>To <dfn export for="fetch controller">abort</dfn> a <a for=/>fetch controller</a>
-<var>controller</var>, set <var>controller</var>'s <a for="fetch controller">state</a> to
-"<code>aborted</code>".
+<var>controller</var> with optional <var>error</var>:

```suggestion
<var>controller</var> with an optional <var>error</var>:
```

> @@ -254,8 +257,20 @@ given a <a>fetch controller</a> <var>controller</var>:
 </ol>
 
 <p>To <dfn export for="fetch controller">abort</dfn> a <a for=/>fetch controller</a>
-<var>controller</var>, set <var>controller</var>'s <a for="fetch controller">state</a> to
-"<code>aborted</code>".
+<var>controller</var> with optional <var>error</var>:
+
+<ol>
+ <li><p>Set <var>controller</var>'s <a for="fetch controller">state</a> to "<code>aborted</code>".
+
+ <li><p>Let <var>error</var> be an "{{AbortError}}" {{DOMException}}, if it is not given.

```suggestion
 <li><p>Set <var>error</var> to an "{{AbortError}}" {{DOMException}} if it is not given.
```

> @@ -7800,12 +7831,10 @@ method steps are:
  <li><p>Return <var>p</var>.
 </ol>
 
-<p>To <dfn>abort fetch</dfn> with a <var>promise</var>, <var>request</var>, and
-<var>responseObject</var>, run these steps:
+<p>To <dfn lt="abort the fetch() call" export id=abort-fetch-algorithm>abort a fetch() call</dfn> with a

We should keep the existing ID `abort-fetch` here. Also, if we rename this to include the name of the method, `fetch()` should be inside `<code>`.

> @@ -7784,9 +7803,21 @@ method steps are:
   <ol>
    <li><p>If <var>locallyAborted</var> is true, terminate these substeps.
 
-   <li><p>If <var>response</var>'s <a for=response>aborted flag</a> is set, then <a>abort fetch</a>
-   with <var>p</var>, <var>request</var>, and <var>responseObject</var>, and terminate these
-   substeps.
+   <li><p>If <var>response</var>'s <a for=response>aborted flag</a> is set, then:

In the actual fetch algorithm we also have this check and there is this line:

> If stream is [readable](https://streams.spec.whatwg.org/#readablestream-readable), [error](https://streams.spec.whatwg.org/#readablestream-error) stream with an "[AbortError](https://webidl.spec.whatwg.org/#aborterror)" [DOMException](https://webidl.spec.whatwg.org/#idl-DOMException).

I suspect that needs to be updated as well to use the abort reason on the controller?

It seems that the flow of this should be something like:

1. Website invokes `fetch()` with a signal.
2. Website aborts signal.
3. Signal is used in the fetch algorithm to terminate the actual fetch and cancel streams and such using the abort reason.
4. `fetch()` API observes the fetch algorithm being terminated and exposes the abort reason.

More concretely, why don't we use the abort reason on the controller in the steps below? I don't think we should end up in a situation where a response is aborted but the controller isn't yet.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/1343#pullrequestreview-1120224143
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/fetch/pull/1343/review/1120224143@github.com>