From ca32d2c4d08b37baeaef7c6b58313d5733fda940 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 7 Oct 2019 17:08:45 -0400 Subject: [PATCH 1/4] Format some cache code for readability. --- components/net/http_cache.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/net/http_cache.rs b/components/net/http_cache.rs index 43f82824b1da8..e1e2371845311 100644 --- a/components/net/http_cache.rs +++ b/components/net/http_cache.rs @@ -223,7 +223,8 @@ fn get_response_expiry(response: &Response) -> Duration { // If the response has a Last-Modified header field, // caches are encouraged to use a heuristic expiration value // that is no more than some fraction of the interval since that time. - response.headers.typed_get::() { + response.headers.typed_get::() + { let current = time::now().to_timespec(); let last_modified: SystemTime = last_modified.into(); let last_modified = last_modified.duration_since(SystemTime::UNIX_EPOCH).unwrap(); From e093eee44757bb93bfa27a118bdaa046cc4687b2 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 7 Oct 2019 17:09:35 -0400 Subject: [PATCH 2/4] Add useful cache debugging messages. --- components/net/http_cache.rs | 16 ++++++++++++++-- components/net/http_loader.rs | 13 +++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/components/net/http_cache.rs b/components/net/http_cache.rs index e1e2371845311..9e44b0cc073a7 100644 --- a/components/net/http_cache.rs +++ b/components/net/http_cache.rs @@ -231,6 +231,7 @@ fn get_response_expiry(response: &Response) -> Duration { let last_modified = Timespec::new(last_modified.as_secs() as i64, 0); // A typical setting of this fraction might be 10%. let raw_heuristic_calc = (current - last_modified) / 10; + trace!("calculated {:?} vs. {:?} ({:?})", current, last_modified, raw_heuristic_calc); let result = if raw_heuristic_calc < max_heuristic { raw_heuristic_calc } else { @@ -332,8 +333,12 @@ fn create_cached_response( // TODO: take must-revalidate into account // TODO: if this cache is to be considered shared, take proxy-revalidate into account // - let has_expired = - (adjusted_expires < time_since_validated) || (adjusted_expires == time_since_validated); + let has_expired = adjusted_expires <= time_since_validated; + trace!( + "time_since_validated: {:?}, adjusted_expires: {:?}", + time_since_validated, + adjusted_expires + ); CachedResponse { response: response, needs_validation: has_expired, @@ -722,6 +727,11 @@ impl HttpCache { assert_eq!(response.status.map(|s| s.0), Some(StatusCode::NOT_MODIFIED)); let entry_key = CacheKey::new(&request); if let Some(cached_resources) = self.entries.get_mut(&entry_key) { + trace!( + "there are {} cached responses for {:?}", + cached_resources.len(), + request.url() + ); for cached_resource in cached_resources.iter_mut() { // done_chan will have been set to Some(..) by http_network_fetch. // If the body is not receiving data, set the done_chan back to None. @@ -833,6 +843,7 @@ impl HttpCache { return; } let expiry = get_response_expiry(&response); + debug!("new cached response has expiry of {:?}", expiry); let cacheable_metadata = CachedMetadata { headers: Arc::new(Mutex::new(response.headers.clone())), data: Measurable(MeasurableCachedMetadata { @@ -858,6 +869,7 @@ impl HttpCache { last_validated: time::now(), }), }; + debug!("storing new cached response for {:?}", request.url()); let entry = self.entries.entry(entry_key).or_insert(vec![]); entry.push(entry_resource); // TODO: Complete incomplete responses, including 206 response, when stored here. diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index fa936501fe564..7986dc0a5b75c 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -1040,6 +1040,7 @@ fn http_network_or_cache_fetch( ), }; if needs_revalidation { + debug!("Need to revalidate {:?}", http_request.url()); revalidating_flag = true; // Substep 5 if let Some(http_date) = response_headers.typed_get::() { @@ -1054,6 +1055,7 @@ fn http_network_or_cache_fetch( .insert(header::IF_NONE_MATCH, entity_tag.clone()); } } else { + debug!("Using cached response for {:?}", http_request.url()); // Substep 6 response = cached_response; } @@ -1108,6 +1110,10 @@ fn http_network_or_cache_fetch( // Substep 3 if let Some((200..=399, _)) = forward_response.raw_status { if !http_request.method.is_safe() { + debug!( + "Unsafe method for response; invalidating cached response for {:?}", + http_request.url(), + ); if let Ok(mut http_cache) = context.state.http_cache.write() { http_cache.invalidate(&http_request, &forward_response); } @@ -1120,6 +1126,7 @@ fn http_network_or_cache_fetch( .as_ref() .map_or(false, |s| s.0 == StatusCode::NOT_MODIFIED) { + debug!("Refreshing cached response for {:?}", http_request.url()); if let Ok(mut http_cache) = context.state.http_cache.write() { response = http_cache.refresh(&http_request, forward_response.clone(), done_chan); wait_for_cached_response(done_chan, &mut response); @@ -1129,10 +1136,16 @@ fn http_network_or_cache_fetch( // Substep 5 if response.is_none() { if http_request.cache_mode != CacheMode::NoStore { + debug!("Storing new cached response for {:?}", http_request.url()); // Subsubstep 2, doing it first to avoid a clone of forward_response. if let Ok(mut http_cache) = context.state.http_cache.write() { http_cache.store(&http_request, &forward_response); } + } else { + debug!( + "Can't store new response for {:?} in cached", + http_request.url() + ); } // Subsubstep 1 response = Some(forward_response); From da6644369a999df74250ac0892d4d2c2cd38ebf8 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 7 Oct 2019 17:10:10 -0400 Subject: [PATCH 3/4] Calculate expiry for 304 response after merging with existing response's headers. --- components/net/http_cache.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/components/net/http_cache.rs b/components/net/http_cache.rs index 9e44b0cc073a7..82855a259b40e 100644 --- a/components/net/http_cache.rs +++ b/components/net/http_cache.rs @@ -767,10 +767,12 @@ impl HttpCache { constructed_response.referrer_policy = request.referrer_policy.clone(); constructed_response.raw_status = cached_resource.data.raw_status.clone(); constructed_response.url_list = cached_resource.data.url_list.clone(); + { + let mut stored_headers = cached_resource.data.metadata.headers.lock().unwrap(); + stored_headers.extend(response.headers); + constructed_response.headers = stored_headers.clone(); + } cached_resource.data.expires = get_response_expiry(&constructed_response); - let mut stored_headers = cached_resource.data.metadata.headers.lock().unwrap(); - stored_headers.extend(response.headers); - constructed_response.headers = stored_headers.clone(); return Some(constructed_response); } } From 2313f7f37002981cedae4b58bf90e71d6044fb83 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 7 Oct 2019 17:10:56 -0400 Subject: [PATCH 4/4] Replace existing cached response when revalidation returns a new response. --- components/net/http_cache.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/components/net/http_cache.rs b/components/net/http_cache.rs index 82855a259b40e..9b1d16a006a3f 100644 --- a/components/net/http_cache.rs +++ b/components/net/http_cache.rs @@ -873,6 +873,30 @@ impl HttpCache { }; debug!("storing new cached response for {:?}", request.url()); let entry = self.entries.entry(entry_key).or_insert(vec![]); + + if response + .status + .as_ref() + .map_or(false, |s| s.0 == StatusCode::OK) + { + // Ensure that any existing complete response is overwritten by the new + // complete response. + let existing_complete_response = entry.iter().position(|response| { + response + .data + .status + .as_ref() + .map_or(false, |s| s.0 == StatusCode::OK) + }); + if let Some(idx) = existing_complete_response { + debug!( + "Removing existing cached 200 OK response for {:?}", + request.url() + ); + entry.remove(idx); + } + } + 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