Aborting fetch #6484

jakearchibald · 2017-07-06T16:12:50Z

This change is

jakearchibald · 2017-07-06T16:14:17Z

fetch/api/abort/general.js

+
+  assert_false(request.bodyUsed, "Body has not been used");
+  assert_equals(await request.text(), 'foo', "Correct body");
+}, "Request is not 'used' if signal is aborted before fetching");


@annevk I felt like the above was the expected behaviour, but seems like it'll be a pain to spec. What do you think?

So the idea is that if the signal is aborted it's a no-op when passed to fetch()? It seems like that should be fairly easy to specify (just don't pass it along), but I'm not sure it's the model we want.

The signal is used, but the request isn't, as in the body isn't read.

My thinking is that the abort should happen straight away if the signal is already aborted. No request should be made to the server. Therefore, request.body shouldn't be consumed/locked/disturbed.

Oh, I missed the subtle distinction. I guess the Request constructor (which is what fetch() uses) can just bypass a whole bunch of things when passed an aborted signal?

This is where it gets messy I think. Here are my gut feelings:

const request = new Request(url, { signal: abortedSignal, body }); const anotherRequest = new Request(request);

It feels like request's body should be disturbed/locked, since the body is being explicitly moved from one request to another.

fetch(invalidURL, { signal: abortedSignal });

The above should reject based on the invalid URL, not aborted.

fetch(request, { signal: abortedSignal, body });

In the above, the body shouldn't be consumed, as it doesn't need to be sent to the server, as the signal is already aborted.

So, from the developer's point of view, the order is:

Validate the args.

Abort if already aborted.

Make the request.

Locking/disturbing the body as part of "Validate the args" doesn't fit well with me, although I realise that's how it's specced now.

I guess fetch could undisturb/unlock the body if it's already aborted?

So you basically want fetch() to do something else than invoke the constructor. We could do that, but it'll be tricky to ensure it's correct and has no regressions. (I don't like undisturbing/unlocking. I'd rather not do the "damage" to begin with.)

Given that mixed-content and CSP could be considered exiting early before using the body (yet they disturb/lock the body), I think I'm over complicating it by trying to make aborting different.

Happy to replace this with a test that ensures the body is locked.

Yeah, it does seem like a lot of refactoring effort for unclear benefit. If we had other features that needed that kind of refactoring it might be worth looking into, but not sure.

jakearchibald · 2017-07-06T16:14:50Z

fetch/api/abort/general.js

+
+  // I'm hoping this will give the browser enough time to (incorrectly) make the request
+  // above, if it intends to.
+  await fetch('../resources/data.json').then(r => r.json());


@wanderview Any better ideas than the hack above?

Not really. Doing another operation that takes the same path is what we do other places in wpt to approximate this.

jakearchibald · 2017-07-06T16:16:43Z

tools/wptserve/wptserve/response.py

        except socket.error:
            # This can happen if the socket got closed by the remote end
-            pass
+            return False


@annevk any idea who owns this?

@jgraham should review that.

@jgraham I'm using this to stop an infinite response, but also to track when/if a browser closes the connection related to an aborted fetch.

@jgraham I want to use this in other tests too, eg to stop generating content when the client is no longer interested.

I think this is fine.

jakearchibald · 2017-07-07T11:31:26Z

fetch/api/abort/general.js

+  const request = new Request('');
+  assert_true(Boolean(request.signal), "Signal member is present & truthy");
+  assert_equals(request.signal.constructor, AbortSignal);
+}, "Request objects have a signal property");


@annevk what do you think about this? I'm undecided.

The idea is that requests will always have a signal. If you pass a signal to the fetch constructor, it'll listen to it & repeat the same action on its own signal.

I figured this would make things easier when it came to replacing this with fetchSignal later, as you don't have to worry that request.signal is one of a variety of types (or null).

So instead of forwarding the signal it gets copied? It seems you might still get different types or shapes over time as we add more features, but copying rather than forwarding might be nice if you want to reuse the request... It's not clear if we should take care of that or whether there should be a utility to copy signals.

This is what I was thinking: https://github.com/whatwg/fetch/pull/523/files#diff-e8c7e042697e1e48e4466c612dda6a3bR5006

Then, we can later change all request.signals to FetchSignals, and developers can rely on behaviour like "If self.FetchSignal is supported, all requests will have one as .signal".

jakearchibald · 2017-07-07T13:50:37Z

fetch/api/abort/general.js

+    }
+  }, `Fetch aborted & connection closed when aborted after calling response.${bodyMethod}()`);
+}
+


@domenic I don't suppose you have time to check these aborting-streams tests?

annevk · 2017-07-07T13:41:40Z

fetch/api/abort/cache.https.html

+    await new Promise(resolve => {
+      signal.addEventListener('abort', resolve);
+      controller.abort();
+    });


What's the point of the promise here?

I thought it queued a task, but it doesn't. Will fix.

annevk · 2017-07-07T13:46:32Z

fetch/api/abort/general.js

+
+  // TODO: we may want to discuss this design idea
+  assert_true(Boolean(request.signal), "Signal member is present & truthy");
+  assert_equals(request.signal.constructor, AbortSignal);


I hope folks won't do these kind of checks when branching...

I could change it to instanceof?

It's fine as-is, I doubt anyone would copy from the tests.

Though maybe instanceof is better so we don't have to update this test and it'll keep passing in newer UAs.

Won't we update it to instanceof FetchSignal later?

Yeah, I guess we might do that anyway. Okay, never mind.

annevk · 2017-07-07T13:48:44Z

fetch/api/abort/general.js

+  return Promise.all(
+    keys.map(key => fetch(`../resources/stash-put.py?key=${key}&value=close`))
+  );
+}


Move requestKeys and abortRequests done to where they are actually used? I guess I can kinda see they are global, but to me at least I kept wondering when they'd show up and when they did I had forgotten the setup.

jakearchibald · 2017-07-07T15:12:38Z

I've written all the tests I intended to write.

domenic

In general the same comments everywhere: use promise_rejects instead of your custom assertion function, and consider also testing .closed.

domenic · 2017-07-13T18:19:46Z

fetch/api/resources/basic.html

@@ -0,0 +1 @@
+<!DOCTYPE html>


You should be able to use /common/blank.html for this

That means I'd have to install a service worker at that scope. Is that likely to interfere with other tests?

I don't think tests run concurrently so if you clean up that could work, but it might be clearer to just add a comment as to why you duplicated and keep the scope as-is.

R+ with added newline and the comment.

domenic · 2017-07-13T18:21:27Z

fetch/api/abort/serviceworker-intercepted.https.html

+</head>
+<body>
+<script>
+  function assert_abort_error(err) {


Use promise_rejects(t, "AbortError", promise) instead of this function.

domenic · 2017-07-13T18:22:11Z

fetch/api/abort/serviceworker-intercepted.https.html

+    }, `response.${bodyMethod}() rejects if already aborted`);
+  }
+
+  promise_test(async t => {


Apart from the promise_rejects issue, this looks good. You may also want to test reader.closed.

Fixed the promise_rejects stuff & added reader.closed tests.

jakearchibald · 2017-07-17T14:39:25Z

This is a high-level overview of the intent behind the controller, signal, & fetch abort design.

Aborting a fetch

Fetching can involve setting up connections, making an HTTP request, receiving headers, and reading a response body. A single fetch can involve many iterations of these, through preflights and redirects. The aim is to make the process abortable during all of these phases. The spec should be explicit about how to react to an abort signal at various points during the process, including which points cannot be aborted.

Abort controllers and abort signals

const controller = new AbortController();
const signal = controller.signal;

const fetchPromise = fetch(url, {signal});

The feedback we got from developers was to avoid the fetchPromise being externally abortable – it should be possible to hand someone the deferred result of the fetch without giving them the ability to abort.

As a result, we designed AbortController and AbortSignal to decouple receiving the result, controlling the result, and signalling intent to the result.

signal.addEventListener('abort', () => {
  console.log('Abort signalled!');
});

controller.abort();

The controller gives you the ability to say "I would like to signal 'abort'". The signal broadcasts that intent.

A single signal can be passed to many APIs – a bunch of fetches can be aborted by a single signal.

The signal is just a signal, it doesn't reflect whether anything was actually aborted. For instance, you can signal abort long after the fetch has completed and been fully read. In this case the signal will be in the aborted state, but the fetch will have successfully completed.

A signal may be postMessaged to another thread. It achieves this by being cloned, but the clone listens to the original's signals, and mimics them. Note that this is a clone, not a transfer, the original signal continues to operate.

The design here is deliberately generic, as streams and webauthn want this ability. In future, AbortController and AbortSignal may be subclassed to provide additional signals. For example, fetch may gain the ability to signal priority changes.

AbortController and AbortSignal spec.

`request.signal`

const request = new Request(url, {signal});
console.log(request.signal);

All requests have a signal property whether one is provided in the constructor or not. When a signal is given to the constructor, request.signal listens to its signals, and mimics them. This means request.signal never equals the signal passed into the constructor.

This means, in future, we can change request.signal to a subclass such as FetchSignal, without request.signal being multiple types within a single user agent.

What does aborting look like to the developer?

If the abort occurs before response headers have been received, fetch()'s promise will reject with a DOMException named AbortError.

If the abort occurs when reading the body of the response, the promise (in the case of response.text() etc) will reject or the stream (in the case of response.body) will error, in either case with a DOMException named AbortError.

Aborting can be the result of the developer signalling, but also something lower-level such as the browser no longer wanting to finish the request (eg the user hit the "stop" button).

Aborting should be predictable. If aborting was signalled in the same thread as the fetch promise, the promise should reject immediately, as in rejection callbacks being called in the next microtask. Likewise, the pending/next reads of the body stream should error.

If the abort was signalled in another thread, the rejection should happen as soon as the message can be sent to the other thread.

If the browser has received the whole response, but it hasn't been read, unread data is dropped when abort is signalled, due to how streams work. Methods like response.text() should follow this behaviour and reject.

What does abort look like to the server?

The actual aborting of network actions happens independently of rejecting promises and erroring streams. The browser should close the connection (HTTP/1.1) or reset the stream (HTTP/2).

If aborting has been signalled before calling fetch(), no network activity should take place as a result.

How does this work within a service worker's fetch event?

fetchEvent.request.signal will signal the page's desire to abort the fetch. This may come from calling controller.abort() in the page, or the browser wanting to abort the request for some other reason. For example: if the page closes and the browser no longer needs the response, or the user presses the browser's "stop" button.

This means:

addEventListener('fetch', event => {
  event.respondWith(fetch(event.request));
});

…works for free. However, if the developer wants to rewrite the request (or turn it into multiple requests), they'll need to pass the signal manually:

addEventListener('fetch', event => {
  event.respondWith(
    fetch(somethingElse, { signal: event.request.signal })
  );
});

If a fetch is aborted within a page, but the service worker ignores that signal, the fetch should still abort from the perspective of the page.

jakearchibald · 2017-07-28T13:51:34Z

I can't really figure out what Travis is complaining about.

annevk · 2017-07-28T14:31:40Z

It's some intermittent issue with the Firefox bot (that also seems to be affecting the Chrome bot, but the Chrome bot doesn't count as blocking since it does that a lot). However, I can't really decipher the log either. I asked @jgraham to weigh in on IRC.

jgraham · 2017-07-28T14:53:42Z

w3c-test:mirror

jgraham · 2017-07-28T14:59:54Z

idk why the travis failure isn't being reported here correctly, but @bobholt is working on improved infrastructure there.

Anyway the problem in this case is a legit bug in the tests that needs to be fixed. In the log there are lots of lines at the end like

ERROR:/home/travis/build/w3c/web-platform-tests/tools/ci/check_stability:| `/fetch/api/abort/general.html` | `Signal can be used to abort other fetches, even if another fetch succeeded before aborting`             | **FAIL: 30/10** | `promise_test: Unhandled rejection with value: object "ReferenceError: AbortController is not defined"` |

The important point there is FAIL: 30/10 meaning we did 10 iterations, but got 30 failures for tests that are indistinguishable to the infrastructure i.e. each test ran three times. That appears to be because /fetch/api/abort/general.html is running the same tests in Window, Worker and SharedWorker context, without distinguishing which is which.

jakearchibald · 2017-07-28T15:51:43Z

Thanks! Fixed now.

domenic · 2017-07-29T00:33:05Z

fetch/api/abort/general.js

+    }
+  });
+
+  assert_true(!!cancelReason, 'Cancel called sync');


This test makes sense. Can we not assert anything more specific about cancelReason than that it's truthy?

Doh, that's supposed to be the next two lines. Copy & paste error.

domenic · 2017-07-29T00:34:50Z

fetch/api/abort/general.html

+  fetch_tests_from_worker(new Worker("general.js"));
+  if (window.SharedWorker) {
+    fetch_tests_from_worker(new SharedWorker("general.js"));
+  }


FWIW I found generating multiple tests with a wrapper simpler than contextualTestName: see https://github.com/w3c/web-platform-tests/tree/master/streams readme and https://github.com/w3c/web-platform-tests/blob/master/streams/generate-test-wrappers.js

domenic

The portions I have been asked to comment on look good :)

annevk

Couple nits, but I think this is good to go. I'm personally happy with this landing before the change to the Fetch Standard, but I'd like to hear from @wanderview and @youennf as well. We'll start implementing this a month from now or so, so there's no particular rush.

annevk · 2017-07-31T07:59:33Z

fetch/api/resources/sw-intercept.js

+addEventListener('fetch', event => {
+  event.waitUntil(broadcast(event.request.url));
+  event.respondWith(fetch(event.request));
+});


Add a newline here?

annevk · 2017-07-31T08:00:16Z

fetch/api/resources/basic.html

@@ -0,0 +1 @@
+<!DOCTYPE html>


R+ with added newline and the comment.

annevk · 2017-07-31T08:00:28Z

fetch/api/abort/serviceworker-intercepted.https.html

+  }, "Stream errors once aborted.");
+</script>
+</body>
+</html>


Add newline.

annevk · 2017-07-31T08:00:36Z

fetch/api/abort/general.html

+  }
+</script>
+</body>
+</html>


Add newline.

annevk · 2017-07-31T08:01:02Z

fetch/api/abort/cache.https.html

+  }, "Signals are not stored in the cache API, even if they're already aborted");
+</script>
+</body>
+</html>


Add newline.

annevk · 2017-07-31T08:01:07Z

fetch/api/abort/general-serviceworker.https.html

+  })();
+</script>
+</body>
+</html>


Add newline.

jakearchibald · 2017-07-31T09:16:14Z

Nits addressed. Thanks for the reviews @annevk @domenic @jgraham!

jakearchibald · 2017-07-31T09:19:14Z

https://bugs.chromium.org/p/chromium/issues/detail?id=750599

annevk · 2017-07-31T09:20:06Z

fetch/api/resources/basic.html

+<!DOCTYPE html>
+<!--
+  Duplicating this resource to make service worker scoping simpler in


"Duplicating /common/blank.html..."

annevk · 2017-07-31T09:21:49Z

Bug for Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1378342.

jakearchibald · 2017-07-31T09:34:20Z

Bug for WebKit: https://bugs.webkit.org/show_bug.cgi?id=174980
Bug for Edge: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/13009916/

royling · 2017-08-16T03:50:27Z

In future, AbortController and AbortSignal may be subclassed to provide additional signals. For example, fetch may gain the ability to signal priority changes.

if that's the intent, SignalController and Signal may be more generic to subclass than Abort*.

jakearchibald · 2017-08-16T05:21:56Z

We don't have use cases for that right now, but we can add it later if need be.

This makes the following changes: * Integrates DOM's `AbortSignal`. * Specifies abort points and how to handle termination. * Replaces termination reason with an aborted flag. Tests: web-platform-tests/wpt#6484. Service worker integration follow-up: w3c/ServiceWorker#1178. Fixes #447. Closes #563.

* Updated the versions in WPT_HEAD in checkout.sh and README.chromium. * All wpt commands now have a single entry point, wpt. Include this dispatch module in WPTWhiteList. * The commands for generating manifest, starting wptserve and running wpt lint are now "wpt manifest", "wpt serve" and "wpt lint". Change webkitpy and PRESUBMIT.py accordingly. * The new wptserve contains a change for fetch/api/abort/general-serviceworker.https.html (web-platform-tests/wpt#6484), so the test is rebaselined. Bug: 749879 Change-Id: I214860078add1c391266f063193aa279db414bb0 Reviewed-on: https://chromium-review.googlesource.com/677557 Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Commit-Queue: Robert Ma <robertma@chromium.org> Cr-Commit-Position: refs/heads/master@{#504060}

* Updated the versions in WPT_HEAD in checkout.sh and README.chromium. * All wpt commands now have a single entry point, wpt. Include this dispatch module in WPTWhiteList. * The commands for generating manifest, starting wptserve and running wpt lint are now "wpt manifest", "wpt serve" and "wpt lint". Change webkitpy and PRESUBMIT.py accordingly. * The new wptserve contains a change for fetch/api/abort/general-serviceworker.https.html (web-platform-tests/wpt#6484), so the test is rebaselined. Bug: 749879 Change-Id: I214860078add1c391266f063193aa279db414bb0 Reviewed-on: https://chromium-review.googlesource.com/677557 Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Commit-Queue: Robert Ma <robertma@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#504060} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 26a8ae1db8e8caa1f7c8b84611d6c72948120320

wpt-pr-bot added fetch infra labels Jul 6, 2017

wpt-pr-bot requested review from annevk, gsnedders, jdm, jgraham, mnot and youennf July 6, 2017 16:13

jakearchibald commented Jul 6, 2017

View reviewed changes

jakearchibald commented Jul 7, 2017

View reviewed changes

annevk reviewed Jul 7, 2017

View reviewed changes

jakearchibald changed the title ~~WIP: Aborting fetch~~ Aborting fetch Jul 7, 2017

annevk added the status:needs-spec-decision label Jul 10, 2017

jakearchibald mentioned this pull request Jul 10, 2017

Fetch abort & no-cors requests whatwg/fetch#563

Closed

domenic reviewed Jul 13, 2017

View reviewed changes

This was referenced Jul 18, 2017

Abort controller design whatwg/dom#438

Closed

Aborting a fetch: The Next Generation whatwg/fetch#447

Closed

jakearchibald added 8 commits July 27, 2017 15:05

Some initial tests

f6426f7

More tests

dafbd68

Stream & no-cors tests

b076f77

Don't need a promise here

fdf63fc

Intercepted by service worker tests

568ae5d

Whitespace fixing

c1000fc

Asserting unreached rather than logging events

ee16ec3

Removing custom abort error checks. Adding reader.closed test.

3c232fc

Making test names unique

3e36e5e

domenic reviewed Jul 29, 2017

View reviewed changes

annevk approved these changes Jul 31, 2017

View reviewed changes

Addressing nits

2951a23

annevk reviewed Jul 31, 2017

View reviewed changes

Better comment

b7e0b64

jakearchibald merged commit c0554ac into web-platform-tests:master Aug 2, 2017

TimothyGu mentioned this pull request Aug 23, 2017

Aborting a fetch node-fetch/node-fetch#95

Closed

annevk mentioned this pull request Sep 20, 2017

Aborting fetch whatwg/fetch#523

Merged

bitinn mentioned this pull request Dec 13, 2017

2.0 stable node-fetch/node-fetch#228

Closed

joaovieira mentioned this pull request Feb 4, 2018

Support abort API JakeChampion/fetch#592

Merged

3 tasks

Mouvedia mentioned this pull request Jun 10, 2018

abort Mouvedia/cb-fetch#21

Closed

AMorgaut mentioned this pull request Sep 12, 2019

AbortSignal consumers can potentialy emit abort event on it whatwg/dom#784

Closed

annevk mentioned this pull request Nov 2, 2021

Add abort reason to abort fetch whatwg/fetch#1343

Merged

3 tasks

Aborting fetch #6484

Aborting fetch #6484

Conversation

jakearchibald commented Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald commented Jul 7, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald commented Jul 17, 2017 • edited by marcoscaceres Loading

Aborting a fetch

Abort controllers and abort signals

request.signal

What does aborting look like to the developer?

What does abort look like to the server?

How does this work within a service worker's fetch event?

jakearchibald commented Jul 28, 2017

annevk commented Jul 28, 2017

jgraham commented Jul 28, 2017

jgraham commented Jul 28, 2017

jakearchibald commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald commented Jul 31, 2017

jakearchibald commented Jul 31, 2017

Choose a reason for hiding this comment

annevk commented Jul 31, 2017

jakearchibald commented Jul 31, 2017

royling commented Aug 16, 2017

jakearchibald commented Aug 16, 2017

jakearchibald commented Jul 6, 2017 •

edited

Loading

annevk Jul 7, 2017 •

edited

Loading

jakearchibald Jul 7, 2017 •

edited

Loading

annevk Jul 7, 2017 •

edited

Loading

jakearchibald commented Jul 17, 2017 •

edited by marcoscaceres

Loading

`request.signal`