Re: [whatwg/fetch] WIP: Cancelation (#523)

annevk commented on this pull request.

I have some nits, but I also think I need some guidance given that this changes more than I anticipated. I thought the idea was that only the nested asynchronous operations (such as cache or network requests) would end up getting wrapped, but it seems that much more is wrapped. It's rather unfortunate if that's needed as it would mess up blame rather badly.

> @@ -1212,8 +1213,17 @@ or "<code>worker</code>".
    is true, then <a>queue a fetch task</a> on <var>request</var> to
    <a>process request end-of-body</a> for <var>request</var> and abort these steps.
 
-   <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:
+
+    <ol>
+     <li><p>If the ongoing fetch has <a for=fetch>terminated</a>, abort these steps.

Newline before `<p>`.

> @@ -1485,21 +1495,29 @@ for each associated <a for="fetch group">fetch record</a> whose
 <var>origin</var> and <var>credentials</var>, run these steps:
 
 <ol>
- <li><p>If <a for=connection>connection pool</a> contains a
- <a>connection</a> whose <b>origin</b> is <var>origin</var> and
- <b>credentials</b> is <var>credentials</var>, return that
- <a>connection</a>.
+ <li><p>If <a for=connection>connection pool</a> contains a <a>connection</a> whose <b>origin</b> is
+ <var>origin</var> and <b>credentials</b> is <var>credentials</var>, return that <a>connection</a>.

then return*

>  
-  <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>, close <var>connection</var> and return failure.

then close*

>  
-  <p>If <var>credentials</var> is false, do <em>not</em> send a TLS client certificate.
+  <ol>
+   <li>
+    <p>Set <var>connection</var> to the result of establishing an HTTP connection to
+    <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]] [[!TLS]]
+
+    <p>If <var>credentials</var> is false, do <em>not</em> send a TLS client certificate.

then do*

>  
-  <p>If <var>credentials</var> is false, do <em>not</em> send a TLS client certificate.
+  <ol>
+   <li>
+    <p>Set <var>connection</var> to the result of establishing an HTTP connection to
+    <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]] [[!TLS]]
+
+    <p>If <var>credentials</var> is false, do <em>not</em> send a TLS client certificate.
+
+    <p>If establishing a connection does not succeed (e.g., a DNS, TCP, or TLS error), return

then return*

>  
-  <p>If <var>credentials</var> is false, do <em>not</em> send a TLS client certificate.
+  <ol>
+   <li>
+    <p>Set <var>connection</var> to the result of establishing an HTTP connection to
+    <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]] [[!TLS]]

`[[!TLS]]` needs to be on a new line.

> @@ -1773,8 +1791,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>).
+{{ReadableStream}} object <var>stream</var> with <var>reason</var>, return the result of calling
+<a abstract-op>ReadableStreamCancel</a>(<var>stream</var>, <var>reason</var>).
 
 <p class="note no-backref">Because the reader grants exclusive access, the actual mechanism of how
 to read cannot be observed. Implementations could use more direct mechanism if convenient.

It seems the reference to a reader no longer makes sense given the change you made above.

>    </ol>
 
+ <li><p>If fetch is <a for=fetch>terminated</a>, return a <a>network error</a> with <a
+ for=response>termination reason</a> set to the termination <var>reason</var>.

You should probably introduce _reason_ in the conditional somehow. "If fetch is terminated with _reason_" maybe?

>    </ol>
 
+ <li><p>If fetch is <a for=fetch>terminated</a>, return a <a>network error</a> with <a
+ for=response>termination reason</a> set to the termination <var>reason</var>.

I also somewhat wonder if we need to wrap all these steps, since these are all synchronous operations in the place where they occur, no?

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