Skip to content

Commit

Permalink
Set stale-updating as an explicit CachePhase
Browse files Browse the repository at this point in the history
This cache phase indicates when we are serving a stale cache hit, but
there is another request currently updating the asset.
  • Loading branch information
drcaramelsyrup authored and andrewhavck committed Aug 9, 2024
1 parent 35810d6 commit 7003ac3
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 8 deletions.
2 changes: 1 addition & 1 deletion .bleep
Original file line number Diff line number Diff line change
@@ -1 +1 @@
f6e596e897d73c69369ae85edf58cec64b106969
a626a70391421eb80e6cdc6c3ceb4d16f38367e8
19 changes: 17 additions & 2 deletions pingora-cache/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ pub enum CachePhase {
Miss,
/// A staled (expired) asset is found
Stale,
/// A staled (expired) asset was found, but another request is revalidating it
StaleUpdating,
/// A staled (expired) asset was found, so a fresh one was fetched
Expired,
/// A staled (expired) asset was found, and it was revalidated to be fresh
Expand All @@ -96,6 +98,7 @@ impl CachePhase {
CachePhase::Hit => "hit",
CachePhase::Miss => "miss",
CachePhase::Stale => "stale",
CachePhase::StaleUpdating => "stale-updating",
CachePhase::Expired => "expired",
CachePhase::Revalidated => "revalidated",
CachePhase::RevalidatedNoCache(_) => "revalidated-nocache",
Expand Down Expand Up @@ -260,7 +263,7 @@ impl HttpCache {
use CachePhase::*;
match self.phase {
Disabled(_) | Bypass | Miss | Expired | Revalidated | RevalidatedNoCache(_) => true,
Hit | Stale => false,
Hit | Stale | StaleUpdating => false,
Uninit | CacheKey => false, // invalid states for this call, treat them as false to keep it simple
}
}
Expand Down Expand Up @@ -509,6 +512,7 @@ impl HttpCache {
match self.phase {
CachePhase::Hit
| CachePhase::Stale
| CachePhase::StaleUpdating
| CachePhase::Revalidated
| CachePhase::RevalidatedNoCache(_) => self.inner_mut().body_reader.as_mut().unwrap(),
_ => panic!("wrong phase {:?}", self.phase),
Expand Down Expand Up @@ -544,6 +548,7 @@ impl HttpCache {
| CachePhase::Miss
| CachePhase::Expired
| CachePhase::Stale
| CachePhase::StaleUpdating
| CachePhase::Revalidated
| CachePhase::RevalidatedNoCache(_) => {
let inner = self.inner_mut();
Expand Down Expand Up @@ -786,6 +791,14 @@ impl HttpCache {
// TODO: remove this asset from cache once finished?
}

/// Mark this asset as stale, but being updated separately from this request.
pub fn set_stale_updating(&mut self) {
match self.phase {
CachePhase::Stale => self.phase = CachePhase::StaleUpdating,
_ => panic!("wrong phase {:?}", self.phase),
}
}

/// Update the variance of the [CacheMeta].
///
/// Note that this process may change the lookup `key`, and eventually (when the asset is
Expand Down Expand Up @@ -854,6 +867,7 @@ impl HttpCache {
match self.phase {
// TODO: allow in Bypass phase?
CachePhase::Stale
| CachePhase::StaleUpdating
| CachePhase::Expired
| CachePhase::Hit
| CachePhase::Revalidated
Expand Down Expand Up @@ -882,6 +896,7 @@ impl HttpCache {
match self.phase {
CachePhase::Miss
| CachePhase::Stale
| CachePhase::StaleUpdating
| CachePhase::Expired
| CachePhase::Hit
| CachePhase::Revalidated
Expand Down Expand Up @@ -1006,7 +1021,7 @@ impl HttpCache {

/// Whether this request's cache hit is staled
fn has_staled_asset(&self) -> bool {
self.phase == CachePhase::Stale
matches!(self.phase, CachePhase::Stale | CachePhase::StaleUpdating)
}

/// Whether this asset is staled and stale if error is allowed
Expand Down
5 changes: 4 additions & 1 deletion pingora-proxy/src/proxy_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,9 @@ impl<SV> HttpProxy<SV> {
} else {
break None;
}
} // else continue to serve stale
}
// else continue to serve stale
session.cache.set_stale_updating();
} else if session.cache.is_cache_lock_writer() {
// stale while revalidate logic for the writer
let will_serve_stale = session.cache.can_serve_stale_updating()
Expand All @@ -182,6 +184,7 @@ impl<SV> HttpProxy<SV> {
new_app.process_subrequest(subrequest, sub_req_ctx).await;
});
// continue to serve stale for this request
session.cache.set_stale_updating();
} else {
// return to fetch from upstream
break None;
Expand Down
8 changes: 4 additions & 4 deletions pingora-proxy/tests/test_upstream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1373,7 +1373,7 @@ mod test_cache {
.unwrap();
assert_eq!(res.status(), StatusCode::OK);
let headers = res.headers();
assert_eq!(headers["x-cache-status"], "stale");
assert_eq!(headers["x-cache-status"], "stale-updating");
assert_eq!(res.text().await.unwrap(), "hello world");
});
// sleep just a little to make sure the req above gets the cache lock
Expand All @@ -1387,7 +1387,7 @@ mod test_cache {
.unwrap();
assert_eq!(res.status(), StatusCode::OK);
let headers = res.headers();
assert_eq!(headers["x-cache-status"], "stale");
assert_eq!(headers["x-cache-status"], "stale-updating");
assert_eq!(res.text().await.unwrap(), "hello world");
});
let task3 = tokio::spawn(async move {
Expand All @@ -1399,7 +1399,7 @@ mod test_cache {
.unwrap();
assert_eq!(res.status(), StatusCode::OK);
let headers = res.headers();
assert_eq!(headers["x-cache-status"], "stale");
assert_eq!(headers["x-cache-status"], "stale-updating");
assert_eq!(res.text().await.unwrap(), "hello world");
});

Expand Down Expand Up @@ -1436,7 +1436,7 @@ mod test_cache {
.unwrap();
assert_eq!(res.status(), StatusCode::OK);
let headers = res.headers();
assert_eq!(headers["x-cache-status"], "stale");
assert_eq!(headers["x-cache-status"], "stale-updating");
assert_eq!(res.text().await.unwrap(), "hello world");

// wait for the background request to finish
Expand Down
3 changes: 3 additions & 0 deletions pingora-proxy/tests/utils/server_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,9 @@ impl ProxyHttp for ExampleProxyCache {
CachePhase::Hit => upstream_response.insert_header("x-cache-status", "hit")?,
CachePhase::Miss => upstream_response.insert_header("x-cache-status", "miss")?,
CachePhase::Stale => upstream_response.insert_header("x-cache-status", "stale")?,
CachePhase::StaleUpdating => {
upstream_response.insert_header("x-cache-status", "stale-updating")?
}
CachePhase::Expired => {
upstream_response.insert_header("x-cache-status", "expired")?
}
Expand Down

0 comments on commit 7003ac3

Please sign in to comment.