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

@domenic commented on this pull request.

So this PR seems to be halfway between the simple version and the complex version.

The simple version here ignores service workers. For that, I think you don't need any serialization/deserialization; in fact, I don't think you need the "cloned error" concept at all. You just need the changes you made to fetch(), where if the signal is already aborted, or if the abort steps for the signal are triggered, you reject the returned promise with the signal's reason, instead of with an "AbortError" DOMException.

The complex version involves updating the `request.signal.reason` seen in a service worker, when the main page does `controller.abort(reason)` from outside the service worker. This requires more plumbing than what you have here; in particular, you need to specify how the message gets sent across realms.

I don't understand @noamr's recommendations about how to use the controller for this. In particular I don't understand how we go from "abort a fetch controller with a `reason` JavaScript object" to "post a message to the service worker realm containing the `Request` representing this request, to signal abort on that `Request`'s [signal](https://fetch.spec.whatwg.org/#request-signal) using a deserialization of that request". Maybe he can suggest how this would best be done...


> @@ -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">cloned error</dfn> (default null)
+ <dd>Null, "<code><a exception>AbortError</a></code>" {{DOMException}}, or a Javascript value.

```suggestion
 <dd>Null, an "{{AbortError}}" {{DOMException}}, or a JavaScript value.
```

> @@ -254,8 +257,10 @@ 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>, set <var>controller</var>'s
+<a for="fetch controller">state</a> to "<code>aborted</code>", and set <var>controller</var>'s
+<a for="fetch controller">cloned error</a> to <var>error</var> if given, and otherwise to
+"<code><a exception>AbortError</a></code>" {{DOMException}}.

```suggestion
an "{{AbortError}}" {{DOMException}}.
```

> @@ -7731,12 +7736,15 @@ method steps are:
 
  <li><p>Let <var>request</var> be <var>requestObject</var>'s <a for=Request>request</a>.
 
+ <li><p>Let <var>error</var> be <var>requestObject</var>'s <a for=Request>signal</a>'s
+ <a for=AbortSignal>abort reason</a>.

I don't think the modifications to fetch() work as written. Remember, they are running synchronously, while the fetch proceeds in parallel. So consider code like this:

```js
const controller = new AbortController();
const p = fetch(url, { signal: controller.signal });
setTimeout(() => controller.abort(new Error("boo!")), 1000);
```

In your inserted step 4 here, you set _error_ to a clone of `controller.signal.reason`. But `controller.signal.reason` is undefined, because `controller.abort()` is not called yet. So when we get down to step 14+15, we will serialize undefined and store it, which is pretty useless.

What happens if we just remove steps 14+15?

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

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