Re: [whatwg/fetch] Aborting fetch (#523)

domenic commented on this pull request.

Mostly nits, but some substantive stuff.

One overall question is whether termination reasons actually matter anymore. It seems like we've homogenized them to always be "fatal", and to not have any observable effect. Is that correct? Should we just remove them? (Perhaps in a follow-up.)

>  
-   <li><p>Otherwise, <a lt=terminated for=fetch>terminate</a> the ongoing fetch with reason
-   <i>fatal</i> and abort these steps.
+     <li><p>Otherwise, <a lt=terminated for=fetch>Terminate</a> the ongoing fetch with reason

Nit: lowercase t

>  
-  <p>If establishing a connection does not succeed (e.g., a DNS, TCP, or TLS error), return failure.
+ <li><p>If fetch is <a for=fetch>terminated</a>, then close <var>connection</var> and return

Connection may be null at this point

>    </ol>
 
+ <li><p>If fetch is <a for=fetch>terminated</a>, providing a <var>reason</var>, then return a

What if it's terminated not providing a reason? (If a reason is always provided, then perhaps just s/, providing a/with/ will clarify.)

This recurs at least once more.

>     <li>
-    <p>If <var>request</var>'s <a for=request>method</a> is not
-    `<code>GET</code>` or <var>blob</var> is null, then return a
-    <a>network error</a>.
+    <p>Run these substeps, but abort the substeps if fetch is <a for=fetch>terminated</a>, providing
+    a <var>reason</var>:

This time the "providing" is up here, for unknown reasons... better to be consistent.

>  
- <li><p>Let <var>cancel</var> be an action that
- <a lt=terminated for=fetch>terminates</a> the ongoing fetch with reason
- <i>end-user abort</i>.
+ <li><p>Let <var>cancel</var> be an action that <a lt=terminated for=fetch>terminates</a> the
+ ongoing fetch with reason <i>fatal</i>.

Why the change in reason?

>   a new associated {{Headers}} object whose <a for=Headers>guard</a>
- is "<code>request</code>".
+ is "<code>request</code>", and a new {{AbortSignal}} object.

This sentence as exists is confusing, and you made it a bit worse. A band-aid is to maybe add "associated", but a proper fix would be to set _r_'s [=header=] and [=signal=] fields in explicit steps.

> @@ -5401,6 +5535,30 @@ method, must run these steps:
  <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:
+
+<ol>
+ <li><p>Let <var>error</var> be an "<code><a exception>AbortError</a></code>" {{DOMException}}.
+
+ <li><p>Reject <var>promise</var> with <var>error</var>.
+
+ <li><p>If <var>request</var>'s <a for=request>body</a> is not null and is
+ <a for=ReadableStream>readable</a>, then <a for=ReadableStream>cancel</a> <var>request</var>'s
+ <a for=request>body</a> with <var>error</var>.
+
+ <li><p>If <var>responseObject</var> is null, then return.

_responseObject_ is never null

> @@ -5401,6 +5535,30 @@ method, must run these steps:
  <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:
+
+<ol>
+ <li><p>Let <var>error</var> be an "<code><a exception>AbortError</a></code>" {{DOMException}}.
+
+ <li><p>Reject <var>promise</var> with <var>error</var>.

It might be worth noting that this is a no-op if _promise_ is already fulfilled

> +<p>To <dfn>abort fetch</dfn> with a <var>promise</var>, <var>request</var>, and
+<var>responseObject</var>, run these steps:
+
+<ol>
+ <li><p>Let <var>error</var> be an "<code><a exception>AbortError</a></code>" {{DOMException}}.
+
+ <li><p>Reject <var>promise</var> with <var>error</var>.
+
+ <li><p>If <var>request</var>'s <a for=request>body</a> is not null and is
+ <a for=ReadableStream>readable</a>, then <a for=ReadableStream>cancel</a> <var>request</var>'s
+ <a for=request>body</a> with <var>error</var>.
+
+ <li><p>If <var>responseObject</var> is null, then return.
+
+ <li><p>Reject <var>responseObject</var>'s <a for=Response>trailer promise</a> with
+ <var>error</var>.

Again, might be a no-op.

> + <li><p>Reject <var>promise</var> with <var>error</var>.
+
+ <li><p>If <var>request</var>'s <a for=request>body</a> is not null and is
+ <a for=ReadableStream>readable</a>, then <a for=ReadableStream>cancel</a> <var>request</var>'s
+ <a for=request>body</a> with <var>error</var>.
+
+ <li><p>If <var>responseObject</var> is null, then return.
+
+ <li><p>Reject <var>responseObject</var>'s <a for=Response>trailer promise</a> with
+ <var>error</var>.
+
+ <li><p>Let <var>response</var> be <var>responseObject</var>'s <a for=Response>response</a>.
+
+ <li><p>If <var>response</var>'s <a for=response>body</a> is not null and is
+ <a for=ReadableStream>readable</a>, then <a for=ReadableStream>cancel</a> <var>response</var>'s
+ <a for=response>body</a> with <var>error</var>.

I think this needs to error, not cancel, to match the tests.

>     <li>
-    <p>If <var>read</var> is fulfilled with an object whose <code>done</code> property is false
-    and whose <code>value</code> property is a <code>Uint8Array</code> object, then run these steps:
+    <p>Run these substeps, but abort the substeps if fetch is <a for=fetch>terminated</a>:</p>

As noted in https://github.com/whatwg/infra/issues/143 I'm not sure there's any reason to use "substeps" over "steps" here and elsewhere.

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