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

Fetch cancellation #19274

Merged
merged 6 commits into from Nov 21, 2017

Fetch cancellation: Listen for cancellation and prematurely abort if …

…cancelled
  • Loading branch information
Manishearth committed Nov 21, 2017
commit 7249fd6bd82b70fa097ce2c66dc8bdb4bf023da8
@@ -1087,6 +1087,7 @@ fn http_network_fetch(request: &Request,
let devtools_sender = context.devtools_chan.clone();
let meta_status = meta.status.clone();
let meta_headers = meta.headers.clone();
let cancellation_listener = context.cancellation_listener.clone();
thread::Builder::new().name(format!("fetch worker thread")).spawn(move || {
match StreamedResponse::from_http_response(res) {
Ok(mut res) => {
@@ -1109,6 +1110,11 @@ fn http_network_fetch(request: &Request,
}

loop {
if cancellation_listener.lock().unwrap().cancelled() {
*res_body.lock().unwrap() = ResponseBody::Done(vec![]);

This comment has been minimized.

Copy link
@gterzian

gterzian Nov 18, 2017

Member

In the context of #18676 I think that at this point the response would have been cached already, and such a cancellation would result in any subsequent request for the same url getting the empty cancelled response from the cache.

One way to solve this would be for the cache to disregard any cached responses that would have been cancelled. Checking for an empty response body might be one way to do so, perhaps a clearer way would be to use ResponseBody::Empty here instead, or even introduce a new ResponseBody::Cancelled?

This comment has been minimized.

Copy link
@gterzian

gterzian Nov 18, 2017

Member

Having had a look at the spec, the following can be considered:

  1. At step 5 above, if at that point the fetch has already been aborted/terminated, you could return a network error.
  2. If the fetch is terminated/aborted while the fetch worker is already performing step 12, in addition to what is done here and instead of what I suggested earlier, you could set a new aborted flag on the response. The cache could then also ignore any such aborted responses that would be held in it, when asked to construct a response. The spec mentions such a flag, see https://fetch.spec.whatwg.org/#concept-response-aborted, and it doesn't look like it has been implemented yet.

This comment has been minimized.

Copy link
@jdm

jdm Nov 20, 2017

Member

Let's add the flag to the response and set that here, and use the network error for prior to this point like gterzian suggested.

let _ = done_sender.send(Data::Done);
return;
}
match read_block(&mut res) {
Ok(Data::Payload(chunk)) => {
if let ResponseBody::Receiving(ref mut body) = *res_body.lock().unwrap() {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.