Auto merge of #23494 - gterzian:improve_http_cache, r=jdm

Various improvements and update to the http cache

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23494)
<!-- Reviewable:end -->

components/net/fetch/methods.rs

@@ -456,11 +456,9 @@ 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);
     }
-    }
 
     // Steps 25-27.
     // TODO: remove this line when only asynchronous fetches are used

components/net/http_cache.rs

@@ -634,9 +634,27 @@ impl HttpCache {
                 done_chan,
             );
         } else {
+            while let Some(cached_resource) = candidates.pop() {
                 // Not a Range request.
-            if let Some(ref cached_resource) = candidates.first() {
+                // Do not allow 206 responses to be constructed.
-                // Returning the first response that can 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();
@@ -645,6 +663,7 @@ impl HttpCache {
                 return Some(cached_response);
             }
         }
+        // The cache wasn't able to construct anything.
         None
     }
 
@@ -653,10 +672,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()));
@@ -768,6 +798,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 {
@@ -808,5 +847,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
     }
 }

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
+    // Step 14, update the cached response, done via the shared response body.
-    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);
-        }
-    }
 
     // TODO this step isn't possible yet
     // Step 15

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 crossbeam_channel::unbounded;
-use hyper::method::Method;
+use http::header::{HeaderValue, EXPIRES};
-use hyper::status::StatusCode;
+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![]),
+    let response_bodies = vec![
+        ResponseBody::Receiving(vec![]),
         ResponseBody::Empty,
-                               ResponseBody::Done(vec![])];
+        ResponseBody::Done(vec![]),
+    ];
     let url = ServoUrl::parse("https://servo.org").unwrap();
-    let request = Request::from_init(RequestInit {
+    let request = Request::new(
-        url: url.clone(),
+        url.clone(),
-        method: Method::Get,
+        Some(Origin::Origin(url.clone().origin())),
-        destination: Destination::Document,
+        Some(TEST_PIPELINE_ID),
-        origin: url.clone().origin(),
+    );
-        pipeline_id: Some(TEST_PIPELINE_ID),
+    let timing = ResourceFetchTiming::new(ResourceTimingType::Navigation);
-        .. RequestInit::default()
+    let mut response = Response::new(url.clone(), timing);
-    });
-    let mut response = Response::new(url.clone());
     // Expires header makes the response cacheable.
-    response.headers.set(Expires(HttpDate(time::now())));
+    response
-    response_bodies.iter().for_each(|body| {
+        .headers
+        .insert(EXPIRES, HeaderValue::from_str("-10").unwrap());
     let mut cache = HttpCache::new();
-        *response.body.lock().unwrap() = body;
+    response_bodies.iter().for_each(|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);
+        response.status = Some((StatusCode::NOT_MODIFIED, String::from("304")));
-        let mut done_chan = Some(channel());
+        let mut done_chan = Some(unbounded());
-        let refreshed_response = cache.refresh(&request, response, &mut done_chan);
+        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()),
         }
     })
 }

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;