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

Http cache: introduce cache entries, prevent cache misses when resource will be fetched #24203

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -5,6 +5,7 @@
use crate::data_loader::decode;
use crate::fetch::cors_cache::CorsCache;
use crate::filemanager_thread::{fetch_file_in_chunks, FileManager, FILE_CHUNK_SIZE};
use crate::http_cache::HttpCacheEntry;
use crate::http_loader::{determine_request_referrer, http_fetch, HttpState};
use crate::http_loader::{set_default_accept, set_default_accept_language};
use crate::subresource_integrity::is_response_integrity_valid;
@@ -40,6 +41,7 @@ lazy_static! {

pub type Target<'a> = &'a mut (dyn FetchTaskTarget + Send);

#[derive(Clone)]
pub enum Data {
Payload(Vec<u8>),
Done,
@@ -224,6 +226,13 @@ pub fn main_fetch(
// Step 11.
// Not applicable: see fetch_async.

// Get the cache entry corresponding to the url, after potential hsts switch.
let cache_entry = if let Ok(mut http_cache) = context.state.http_cache.write() {
http_cache.get_entry(&request)

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Sep 12, 2019

Member

Hmm, is this spec-complaint? Shouldn't anything that touches the cache be in http_network_or_cache_fetch?

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 13, 2019

Author Member

Strictly-speaking, I think it's ok, since at this point we're not actually running any specified cache related logic, we're just getting the entry corresponding to the current request, later to be used to check the cache. So basically the "entry" is the interface to the "cache" from the spec perspective, and it will be used later in http_network_or_cache_fetch.

I think the spec in this case is not really providing the full algorithm, rather describing the outcome from a black-box perspective.

For example, see the diagram for the state-machine of the Chromium cache, little of which shows up in the spec: https://www.chromium.org/developers/design-documents/network-stack/http-cache

Same thing goes for the Gecko cache: https://developer.mozilla.org/en-US/docs/Mozilla/HTTP_cache#Implementation (more specifically, see their CacheEntry description).

If anything, ours matches the spec most closely, and yet ours seems to be the most naive implementation of them all.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Sep 16, 2019

Member

I'd still prefer a solution that kept all the caching logic inside http_network_or_cache_fetch, I find it confusing moving some bits of caching out. What's the motivation for doing that?

} else {
None
};

// Step 12.
let mut response = response.unwrap_or_else(|| {
let current_url = request.current_url();
@@ -245,15 +254,15 @@ pub fn main_fetch(
request.response_tainting = ResponseTainting::Basic;

// Substep 2.
scheme_fetch(request, cache, target, done_chan, context)
scheme_fetch(request, cache, target, done_chan, context, &cache_entry)
} else if request.mode == RequestMode::SameOrigin {
Response::network_error(NetworkError::Internal("Cross-origin response".into()))
} else if request.mode == RequestMode::NoCors {
// Substep 1.
request.response_tainting = ResponseTainting::Opaque;

// Substep 2.
scheme_fetch(request, cache, target, done_chan, context)
scheme_fetch(request, cache, target, done_chan, context, &cache_entry)
} else if !matches!(current_url.scheme(), "http" | "https") {
Response::network_error(NetworkError::Internal("Non-http scheme".into()))
} else if request.use_cors_preflight ||
@@ -267,7 +276,15 @@ pub fn main_fetch(
request.response_tainting = ResponseTainting::CorsTainting;
// Substep 2.
let response = http_fetch(
request, cache, true, true, false, target, done_chan, context,
request,
cache,
true,
true,
false,
target,
done_chan,
context,
&cache_entry,
);
// Substep 3.
if response.is_network_error() {
@@ -280,7 +297,15 @@ pub fn main_fetch(
request.response_tainting = ResponseTainting::CorsTainting;
// Substep 2.
http_fetch(
request, cache, true, false, false, target, done_chan, context,
request,
cache,
true,
false,
false,
target,
done_chan,
context,
&cache_entry,
)
}
});
@@ -455,8 +480,14 @@ pub fn main_fetch(
// Step 24.
target.process_response_eof(&response);

if let Ok(mut http_cache) = context.state.http_cache.write() {
http_cache.update_awaiting_consumers(&request, &response);
// Note: we need to get an entry again, to take potential re-directs into account.
let cache_entry = if let Ok(mut http_cache) = context.state.http_cache.write() {
http_cache.get_entry(&request)
} else {
None
};
if let Some(entry) = cache_entry {
entry.update_awaiting_consumers(&request, &response);
}

// Steps 25-27.
@@ -477,7 +508,7 @@ fn wait_for_response(response: &mut Response, target: Target, done_chan: &mut Do
},
Data::Done => break,
Data::Cancelled => {
response.aborted.store(true, Ordering::Relaxed);
response.aborted.store(true, Ordering::Release);
break;
},
}
@@ -570,6 +601,7 @@ fn scheme_fetch(
target: Target,
done_chan: &mut DoneChannel,
context: &FetchContext,
cache_entry: &Option<HttpCacheEntry>,
) -> Response {
let url = request.current_url();

@@ -586,7 +618,15 @@ fn scheme_fetch(
},

"http" | "https" => http_fetch(
request, cache, false, false, false, target, done_chan, context,
request,
cache,
false,
false,
false,
target,
done_chan,
context,
cache_entry,
),

"data" => match decode(&url) {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.