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
Prev

Add aborted flag to response, set when fetch is aborted

  • Loading branch information
Manishearth committed Nov 21, 2017
commit 6f59b152f1738ddecfbc1891bafd6f55c34992bc
@@ -37,6 +37,7 @@ pub type Target<'a> = &'a mut (FetchTaskTarget + Send);
pub enum Data {
Payload(Vec<u8>),
Done,
Cancelled,
}

pub struct FetchContext {
@@ -347,17 +348,17 @@ pub fn main_fetch(request: &mut Request,
};

// Execute deferred rebinding of response.
let response = if let Some(error) = internal_error {
let mut response = if let Some(error) = internal_error {
Response::network_error(error)
} else {
response
};

// Step 19.
let mut response_loaded = false;
let response = if !response.is_network_error() && !request.integrity_metadata.is_empty() {
let mut response = if !response.is_network_error() && !request.integrity_metadata.is_empty() {
// Step 19.1.
wait_for_response(&response, target, done_chan);
wait_for_response(&mut response, target, done_chan);
response_loaded = true;

// Step 19.2.
@@ -376,9 +377,9 @@ pub fn main_fetch(request: &mut Request,
if request.synchronous {
// process_response is not supposed to be used
// by sync fetch, but we overload it here for simplicity
target.process_response(&response);
target.process_response(&mut response);
if !response_loaded {
wait_for_response(&response, target, done_chan);
wait_for_response(&mut response, target, done_chan);
}
// overloaded similarly to process_response
target.process_response_eof(&response);
@@ -400,7 +401,7 @@ pub fn main_fetch(request: &mut Request,

// Step 23.
if !response_loaded {
wait_for_response(&response, target, done_chan);
wait_for_response(&mut response, target, done_chan);
}

// Step 24.
@@ -411,7 +412,7 @@ pub fn main_fetch(request: &mut Request,
response
}

fn wait_for_response(response: &Response, target: Target, done_chan: &mut DoneChannel) {
fn wait_for_response(response: &mut Response, target: Target, done_chan: &mut DoneChannel) {
if let Some(ref ch) = *done_chan {
loop {
match ch.1.recv()
@@ -420,6 +421,10 @@ fn wait_for_response(response: &Response, target: Target, done_chan: &mut DoneCh
target.process_response_chunk(vec);
},
Data::Done => break,
Data::Cancelled => {
response.aborted = true;
break;
}
}
}
} else {
@@ -1088,6 +1088,9 @@ fn http_network_fetch(request: &Request,
let meta_status = meta.status.clone();
let meta_headers = meta.headers.clone();
let cancellation_listener = context.cancellation_listener.clone();
if cancellation_listener.lock().unwrap().cancelled() {
return Response::network_error(NetworkError::Internal("Fetch aborted".into()))
}
thread::Builder::new().name(format!("fetch worker thread")).spawn(move || {
match StreamedResponse::from_http_response(res) {
Ok(mut res) => {
@@ -1112,7 +1115,7 @@ 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);
let _ = done_sender.send(Data::Cancelled);
return;
}
match read_block(&mut res) {
@@ -1134,6 +1137,7 @@ fn http_network_fetch(request: &Request,
let _ = done_sender.send(Data::Done);
break;
}
Ok(Data::Cancelled) => unreachable!() // read_block doesn't return Data::Cancelled
}
}
}
@@ -112,6 +112,8 @@ pub struct Response {
pub internal_response: Option<Box<Response>>,
/// whether or not to try to return the internal_response when asked for actual_response
pub return_internal: bool,
/// https://fetch.spec.whatwg.org/#concept-response-aborted
pub aborted: bool,
}

impl Response {
@@ -133,6 +135,7 @@ impl Response {
location_url: None,
internal_response: None,
return_internal: true,
aborted: false,
}
}

@@ -162,6 +165,7 @@ impl Response {
location_url: None,
internal_response: None,
return_internal: true,
aborted: false,
}
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.