From 4f41065dfbf6de00189d8713b3d6e716a5597e39 Mon Sep 17 00:00:00 2001 From: Gregory Terzian Date: Sat, 1 Jun 2019 16:40:50 +0800 Subject: [PATCH 1/4] http-cache: improve handling of network errors and partial content --- components/net/fetch/methods.rs | 6 ++--- components/net/http_cache.rs | 43 +++++++++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/components/net/fetch/methods.rs b/components/net/fetch/methods.rs index 4022ca6b9bb..d09819dcf45 100644 --- a/components/net/fetch/methods.rs +++ b/components/net/fetch/methods.rs @@ -457,10 +457,8 @@ pub fn main_fetch( // Step 24. target.process_response_eof(&response); - if !response.is_network_error() { - if let Ok(mut http_cache) = context.state.http_cache.write() { - http_cache.update_awaiting_consumers(&request, &response); - } + if let Ok(mut http_cache) = context.state.http_cache.write() { + http_cache.update_awaiting_consumers(&request, &response); } // Steps 25-27. diff --git a/components/net/http_cache.rs b/components/net/http_cache.rs index 9e430ceea3e..7753f1204e9 100644 --- a/components/net/http_cache.rs +++ b/components/net/http_cache.rs @@ -638,9 +638,27 @@ impl HttpCache { done_chan, ); } else { - // Not a Range request. - if let Some(ref cached_resource) = candidates.first() { - // Returning the first response that can be constructed + while let Some(cached_resource) = candidates.pop() { + // Not a Range request. + // Do not allow 206 responses to be constructed. + // + // See https://tools.ietf.org/html/rfc7234#section-3.1 + // + // A cache MUST NOT use an incomplete response to answer requests unless the + // response has been made complete or the request is partial and + // specifies a range that is wholly within the incomplete response. + // + // TODO: Combining partial content to fulfill a non-Range request + // see https://tools.ietf.org/html/rfc7234#section-3.3 + match cached_resource.data.raw_status { + Some((ref code, _)) => { + if *code == 206 { + continue; + } + }, + None => continue, + } + // Returning a response that can be constructed // TODO: select the most appropriate one, using a known mechanism from a selecting header field, // or using the Date header to return the most recent one. let cached_headers = cached_resource.data.metadata.headers.lock().unwrap(); @@ -649,6 +667,7 @@ impl HttpCache { return Some(cached_response); } } + // The cache wasn't able to construct anything. None } @@ -657,10 +676,21 @@ impl HttpCache { if let ResponseBody::Done(ref completed_body) = *response.body.lock().unwrap() { let entry_key = CacheKey::new(request.clone()); if let Some(cached_resources) = self.entries.get(&entry_key) { - for cached_resource in cached_resources.iter() { + // Ensure we only wake-up consumers of relevant resources, + // ie we don't want to wake-up 200 awaiting consumers with a 206. + let relevant_cached_resources = cached_resources + .iter() + .filter(|resource| resource.data.raw_status == response.raw_status); + for cached_resource in relevant_cached_resources { let mut awaiting_consumers = cached_resource.awaiting_body.lock().unwrap(); for done_sender in awaiting_consumers.drain(..) { - if cached_resource.aborted.load(Ordering::Relaxed) { + if cached_resource.aborted.load(Ordering::Relaxed) || + response.is_network_error() + { + // In the case of an aborted fetch or a network errror, + // wake-up all awaiting consumers. + // Each will then start a new network request. + // TODO: Wake-up only one consumer, and make it the producer on which others wait. let _ = done_sender.send(Data::Cancelled); } else { let _ = done_sender.send(Data::Payload(completed_body.clone())); @@ -812,5 +842,8 @@ impl HttpCache { }; let entry = self.entries.entry(entry_key).or_insert(vec![]); entry.push(entry_resource); + // TODO: Complete incomplete responses, including 206 response, when stored here. + // See A cache MAY complete a stored incomplete response by making a subsequent range request + // https://tools.ietf.org/html/rfc7234#section-3.1 } } From 049817c5a71c0d0a05badba85fbdde8db3b070fa Mon Sep 17 00:00:00 2001 From: Gregory Terzian Date: Wed, 19 Jun 2019 05:25:32 -0700 Subject: [PATCH 2/4] http-cache: remove double-store of resources --- components/net/http_loader.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 6574623232d..e0522f633be 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -1382,12 +1382,7 @@ fn http_network_fetch( // Step 13 // TODO this step isn't possible yet (CSP) - // Step 14 - if !response.is_network_error() && request.cache_mode != CacheMode::NoStore { - if let Ok(mut http_cache) = context.state.http_cache.write() { - http_cache.store(&request, &response); - } - } + // Step 14, update the cached response, done via the shared response body. // TODO this step isn't possible yet // Step 15 From 67494d477667001910148647b03118534b7d7f82 Mon Sep 17 00:00:00 2001 From: Gregory Terzian Date: Thu, 20 Jun 2019 06:18:25 -0700 Subject: [PATCH 3/4] http-cache: do not cache responses from requests with authorization --- components/net/http_cache.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/components/net/http_cache.rs b/components/net/http_cache.rs index 7753f1204e9..f906a53ac75 100644 --- a/components/net/http_cache.rs +++ b/components/net/http_cache.rs @@ -802,6 +802,15 @@ impl HttpCache { // Only Get requests are cached. return; } + if request.headers.contains_key(header::AUTHORIZATION) { + // https://tools.ietf.org/html/rfc7234#section-3.1 + // A shared cache MUST NOT use a cached response + // to a request with an Authorization header field + // + // TODO: unless a cache directive that allows such + // responses to be stored is present in the response. + return; + }; let entry_key = CacheKey::new(request.clone()); let metadata = match response.metadata() { Ok(FetchMetadata::Filtered { From 689b7971b82b6268de3c4eaff2d54dfc77910088 Mon Sep 17 00:00:00 2001 From: Gregory Terzian Date: Sat, 22 Jun 2019 06:28:32 -0700 Subject: [PATCH 4/4] http-cache: re-enable and update test --- components/net/tests/http_cache.rs | 51 +++++++++++++++--------------- components/net/tests/main.rs | 1 + 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/components/net/tests/http_cache.rs b/components/net/tests/http_cache.rs index fceec1aadc7..743e6f43188 100644 --- a/components/net/tests/http_cache.rs +++ b/components/net/tests/http_cache.rs @@ -2,48 +2,49 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use hyper::header::{Expires, HttpDate}; -use hyper::method::Method; -use hyper::status::StatusCode; +use crossbeam_channel::unbounded; +use http::header::{HeaderValue, EXPIRES}; +use http::StatusCode; use msg::constellation_msg::TEST_PIPELINE_ID; use net::http_cache::HttpCache; -use net_traits::request::{Destination, Request, RequestInit}; +use net_traits::request::{Origin, Request}; use net_traits::response::{Response, ResponseBody}; +use net_traits::{ResourceFetchTiming, ResourceTimingType}; use servo_url::ServoUrl; -use std::sync::mpsc::channel; - #[test] fn test_refreshing_resource_sets_done_chan_the_appropriate_value() { - let response_bodies = vec![ResponseBody::Receiving(vec![]), - ResponseBody::Empty, - ResponseBody::Done(vec![])]; + let response_bodies = vec![ + ResponseBody::Receiving(vec![]), + ResponseBody::Empty, + ResponseBody::Done(vec![]), + ]; let url = ServoUrl::parse("https://servo.org").unwrap(); - let request = Request::from_init(RequestInit { - url: url.clone(), - method: Method::Get, - destination: Destination::Document, - origin: url.clone().origin(), - pipeline_id: Some(TEST_PIPELINE_ID), - .. RequestInit::default() - }); - let mut response = Response::new(url.clone()); + let request = Request::new( + url.clone(), + Some(Origin::Origin(url.clone().origin())), + Some(TEST_PIPELINE_ID), + ); + let timing = ResourceFetchTiming::new(ResourceTimingType::Navigation); + let mut response = Response::new(url.clone(), timing); // Expires header makes the response cacheable. - response.headers.set(Expires(HttpDate(time::now()))); + response + .headers + .insert(EXPIRES, HeaderValue::from_str("-10").unwrap()); + let mut cache = HttpCache::new(); response_bodies.iter().for_each(|body| { - let mut cache = HttpCache::new(); - *response.body.lock().unwrap() = body; + *response.body.lock().unwrap() = body.clone(); // First, store the 'normal' response. cache.store(&request, &response); // Second, mutate the response into a 304 response, and refresh the stored one. - response.status = Some(StatusCode::NotModified); - let mut done_chan = Some(channel()); - let refreshed_response = cache.refresh(&request, response, &mut done_chan); + response.status = Some((StatusCode::NOT_MODIFIED, String::from("304"))); + let mut done_chan = Some(unbounded()); + let refreshed_response = cache.refresh(&request, response.clone(), &mut done_chan); // Ensure a resource was found, and refreshed. assert!(refreshed_response.is_some()); match body { ResponseBody::Receiving(_) => assert!(done_chan.is_some()), - ResponseBody::Empty | ResponseBody::Done(_) => assert!(done_chan.is_none()) + ResponseBody::Empty | ResponseBody::Done(_) => assert!(done_chan.is_none()), } }) } diff --git a/components/net/tests/main.rs b/components/net/tests/main.rs index 1580eb8310a..d473ee2368e 100644 --- a/components/net/tests/main.rs +++ b/components/net/tests/main.rs @@ -14,6 +14,7 @@ mod fetch; mod file_loader; mod filemanager_thread; mod hsts; +mod http_cache; mod http_loader; mod mime_classifier; mod resource_thread;