Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aborting fetch #6484

Merged
merged 13 commits into from Aug 2, 2017
Merged

Conversation

@jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Jul 6, 2017

For whatwg/fetch#523


This change is Reviewable


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");
Copy link
Contributor Author

@jakearchibald jakearchibald Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Validate the args.
  2. Abort if already aborted.
  3. 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?

Copy link
Member

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 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());
Copy link
Contributor Author

@jakearchibald jakearchibald Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wanderview Any better ideas than the hack above?

Copy link
Member

@wanderview wanderview Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

except socket.error:
# This can happen if the socket got closed by the remote end
pass
return False
Copy link
Contributor Author

@jakearchibald jakearchibald Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk any idea who owns this?

Copy link
Member

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgraham should review that.

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jgraham jgraham Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine.

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");
Copy link
Contributor Author

@jakearchibald jakearchibald Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of the promise here?

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// 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);
Copy link
Member

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could change it to instanceof?

Copy link
Member

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't we update it to instanceof FetchSignal later?

Copy link
Member

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@annevk annevk Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jakearchibald changed the title WIP: Aborting fetch Aborting fetch Jul 7, 2017
@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Jul 7, 2017

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

Copy link
Member

@domenic domenic left a comment

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

@@ -0,0 +1 @@
<!DOCTYPE html>
Copy link
Member

@domenic domenic Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@annevk annevk Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@annevk annevk Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R+ with added newline and the comment.

</head>
<body>
<script>
function assert_abort_error(err) {
Copy link
Member

@domenic domenic Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}, `response.${bodyMethod}() rejects if already aborted`);
}

promise_test(async t => {
Copy link
Member

@domenic domenic Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the promise_rejects stuff & added reader.closed tests.

@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Jul 17, 2017

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
Copy link
Contributor Author

@jakearchibald jakearchibald commented Jul 28, 2017

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

@annevk
Copy link
Member

@annevk annevk commented Jul 28, 2017

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
Copy link
Contributor

@jgraham jgraham commented Jul 28, 2017

w3c-test:mirror

@jgraham
Copy link
Contributor

@jgraham jgraham commented Jul 28, 2017

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
Copy link
Contributor Author

@jakearchibald jakearchibald commented Jul 28, 2017

Thanks! Fixed now.

}
});

assert_true(!!cancelReason, 'Cancel called sync');
Copy link
Member

@domenic domenic Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jakearchibald jakearchibald Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fetch_tests_from_worker(new Worker("general.js"));
if (window.SharedWorker) {
fetch_tests_from_worker(new SharedWorker("general.js"));
}
Copy link
Member

@domenic domenic Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@domenic domenic left a comment

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

annevk
annevk approved these changes Jul 31, 2017
Copy link
Member

@annevk annevk left a comment

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.

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

@annevk annevk Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a newline here?

@@ -0,0 +1 @@
<!DOCTYPE html>
Copy link
Member

@annevk annevk Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R+ with added newline and the comment.

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

@annevk annevk Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline.

}
</script>
</body>
</html>
Copy link
Member

@annevk annevk Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline.

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

@annevk annevk Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline.

})();
</script>
</body>
</html>
Copy link
Member

@annevk annevk Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline.

@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Jul 31, 2017

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

@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Jul 31, 2017

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

@annevk annevk Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@annevk
Copy link
Member

@annevk annevk commented Jul 31, 2017

@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Jul 31, 2017

@jakearchibald jakearchibald merged commit c0554ac into web-platform-tests:master Aug 2, 2017
1 check passed
@royling
Copy link

@royling royling commented Aug 16, 2017

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
Copy link
Contributor Author

@jakearchibald jakearchibald commented Aug 16, 2017

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

annevk added a commit to whatwg/fetch that referenced this issue Sep 20, 2017
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.
MXEBot pushed a commit to mirror/chromium that referenced this issue Sep 26, 2017
* 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}
@joaovieira joaovieira mentioned this pull request Feb 4, 2018
3 tasks
nareix pushed a commit to nareix/webrtc.third_party that referenced this issue Mar 5, 2018
* 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
@Mouvedia Mouvedia mentioned this pull request Jun 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants