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

domenic commented on this pull request.



> @@ -1782,11 +1806,8 @@ from a {{ReadableStream}} object with <var>reader</var>, run these steps:
 </ol>
 
 <p>To <dfn export for=ReadableStream id=concept-cancel-readablestream>cancel</dfn> a
-{{ReadableStream}} object with <var>reader</var> and <var>reason</var>, return the result of calling
-<a abstract-op>ReadableStreamCancel</a>(<var>reader</var>, <var>reason</var>).
-
-<p class="note no-backref">Because the reader grants exclusive access, the actual mechanism of how

Why remove this?

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

Maybe this would be clearer by adding an additional step, "Let _reason_ be the provided termination reason". Or, in an inline version: "If fetch is terminated, then let _reason_ be the provided termination reason, and do X with _reason_."

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

Closing a tab is not what's in play here. This is about the user calling .cancel() on a stream they are reading. Or, because it goes through the same JS API, the browser calling https://fetch.spec.whatwg.org/branch-snapshots/cancelation/#concept-cancel-readablestream .

That is called by https://fetch.spec.whatwg.org/branch-snapshots/cancelation/#abort-fetch , in fact. So this is saying that any fetch which is aborted through an AbortController will [manifest to XHR](https://xhr.spec.whatwg.org/#handle-errors) not as an abort, but instead as a network error with (I believe) no events fired.

I don't think this is right, in short.

>  
-     <li><p>Let <var>codings</var> be the result of <a>extracting header list values</a> given
-     `<code>Content-Encoding</code>` and <var>response</var>'s <a for=response>header list</a>.
+      <ol>
+       <li><p>let <var>bytes</var> be the transmitted bytes.

Uppercase

>  
-     <li><p>If <var>stream</var> is <a for=ReadableStream>readable</a>,
-     <a abstract-op>error</a> <var>stream</var> with a
-     <code>TypeError</code>.
+     <li><p>Otherwise, if the bytes transmission for <var>response</var>'s message body is done
+     normally and <var>stream</var> is <a for=ReadableStream>readable</a>, then
+     <a abstract-op>close</a> <var>stream</var> and abort these steps.

abort these steps -> break

>  
  <li>
   <p>Run these substeps <a>in parallel</a>:
 
   <ol>
    <li>
-    <p>Whenever one or more bytes are transmitted from <var>response</var>'s message body, let
-    <var>bytes</var> be the transmitted bytes and run these subsubsteps:
-    <!-- XXX xref message body -->
+    <p>While true, breaking if fetch <a for=fetch lt=terminated>terminates</a>, providing a
+    <var>reason</var>:

If this loop terminates due to reaching the end of the stream, the next step (step 2) still assumes reason exists, which it does not.

In fact, all of steps 2-5 seem wrong if the body successfully finishes uploading.

> @@ -4950,12 +5049,23 @@ constructor must run these steps:
    to <var>method</var>.
   </ol>
 
+ <li><p>If <var>init</var>'s <code>signal</code> member is present, then set <var>signal</var> to
+ it.

So, if you do `const r = new Request(otherRequest, { })` then otherRequest's signal will control aborting `r`. But if you do `const r = new Request(otherRequest, { signal })`, then `signal` will control aborting `r`, and `otherRequest` will have no impact at all. Is that correct? Is that what's tested?

-- 
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-58483055