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.

Sign up for GitHub

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

Jump to bottom

cache: fix stale-while-revalidate and stale-if-error #3865

Merged
merged 4 commits into from
Nov 25, 2024
Merged

cache: fix stale-while-revalidate and stale-if-error #3865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa398ba
cache: fix stale-while-revalidate and stale-if-error
flakey5 Nov 21, 2024
79ee31f
type tests
flakey5 Nov 25, 2024
3d36431
Merge branch 'main' into flakey5/20241120/3852
flakey5 Nov 25, 2024
9a1f2bc
test fixup
flakey5 Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/cache/memory-cache-store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class MemoryCacheStore {
headers: entry.headers,
body: entry.body,
etag: entry.etag,
cacheControlDirectives: entry.cacheControlDirectives,
cachedAt: entry.cachedAt,
staleAt: entry.staleAt,
deleteAt: entry.deleteAt
Expand Down
11 changes: 10 additions & 1 deletion lib/cache/sqlite-cache-store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ class SqliteCacheStore {
statusCode INTEGER NOT NULL,
statusMessage TEXT NOT NULL,
headers TEXT NULL,
cacheControlDirectives TEXT NULL,
etag TEXT NULL,
vary TEXT NULL,
cachedAt INTEGER NOT NULL,
Expand All @@ -128,6 +129,7 @@ class SqliteCacheStore {
statusMessage,
headers,
etag,
cacheControlDirectives,
vary,
cachedAt,
staleAt
Expand All @@ -147,6 +149,7 @@ class SqliteCacheStore {
statusMessage = ?,
headers = ?,
etag = ?,
cacheControlDirectives = ?,
cachedAt = ?,
staleAt = ?,
deleteAt = ?
Expand All @@ -164,11 +167,12 @@ class SqliteCacheStore {
statusMessage,
headers,
etag,
cacheControlDirectives,
vary,
cachedAt,
staleAt,
deleteAt
) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
`)

this.#deleteByUrlQuery = this.#db.prepare(
Expand Down Expand Up @@ -223,6 +227,9 @@ class SqliteCacheStore {
statusMessage: value.statusMessage,
headers: value.headers ? JSON.parse(value.headers) : undefined,
etag: value.etag ? value.etag : undefined,
cacheControlDirectives: value.cacheControlDirectives
? JSON.parse(value.cacheControlDirectives)
: undefined,
cachedAt: value.cachedAt,
staleAt: value.staleAt,
deleteAt: value.deleteAt
Expand Down Expand Up @@ -275,6 +282,7 @@ class SqliteCacheStore {
value.statusMessage,
value.headers ? JSON.stringify(value.headers) : null,
value.etag ? value.etag : null,
value.cacheControlDirectives ? JSON.stringify(value.cacheControlDirectives) : null,
value.cachedAt,
value.staleAt,
value.deleteAt,
Expand All @@ -292,6 +300,7 @@ class SqliteCacheStore {
value.statusMessage,
value.headers ? JSON.stringify(value.headers) : null,
value.etag ? value.etag : null,
value.cacheControlDirectives ? JSON.stringify(value.cacheControlDirectives) : null,
value.vary ? JSON.stringify(value.vary) : null,
value.cachedAt,
value.staleAt,
Expand Down
20 changes: 14 additions & 6 deletions lib/handler/cache-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ class CacheHandler {
statusMessage,
headers: strippedHeaders,
vary: varyDirectives,
cacheControlDirectives,
cachedAt: now,
staleAt,
deleteAt
Expand Down Expand Up @@ -170,7 +171,7 @@ class CacheHandler {
*
* @param {number} statusCode
* @param {Record<string, string | string[]>} headers
* @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives
* @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives
*/
function canCacheResponse (statusCode, headers, cacheControlDirectives) {
if (statusCode !== 200 && statusCode !== 307) {
Expand Down Expand Up @@ -217,7 +218,7 @@ function canCacheResponse (statusCode, headers, cacheControlDirectives) {
/**
* @param {number} now
* @param {Record<string, string | string[]>} headers
* @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives
* @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives
*
* @returns {number | undefined} time that the value is stale at or undefined if it shouldn't be cached
*/
Expand Down Expand Up @@ -253,21 +254,28 @@ function determineStaleAt (now, headers, cacheControlDirectives) {

/**
* @param {number} now
* @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives
* @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives
* @param {number} staleAt
*/
function determineDeleteAt (now, cacheControlDirectives, staleAt) {
let staleWhileRevalidate = -Infinity
let staleIfError = -Infinity

if (cacheControlDirectives['stale-while-revalidate']) {
return now + (cacheControlDirectives['stale-while-revalidate'] * 1000)
staleWhileRevalidate = now + (cacheControlDirectives['stale-while-revalidate'] * 1000)
}

if (cacheControlDirectives['stale-if-error']) {
staleIfError = now + (cacheControlDirectives['stale-if-error'] * 1000)
}

return staleAt
return Math.max(staleAt, staleWhileRevalidate, staleIfError)
}

/**
* Strips headers required to be removed in cached responses
* @param {Record<string, string | string[]>} headers
* @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives
* @param {import('../../types/cache-interceptor.d.ts').default.CacheControlDirectives} cacheControlDirectives
* @returns {Record<string, string | string []>}
*/
function stripNecessaryHeaders (headers, cacheControlDirectives) {
Expand Down
21 changes: 16 additions & 5 deletions lib/handler/cache-revalidation-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ const assert = require('node:assert')
*/
class CacheRevalidationHandler {
#successful = false

/**
* @type {((boolean, any) => void) | null}
*/
#callback

/**
* @type {(import('../../types/dispatcher.d.ts').default.DispatchHandler)}
*/
Expand All @@ -29,19 +31,26 @@ class CacheRevalidationHandler {
#context

/**
* @param {(boolean, any) => void} callback Function to call if the cached value is valid
* @param {import('../../types/dispatcher.d.ts').default.DispatchHandler} handler
* @type {boolean}
*/
#allowErrorStatusCodes

/**
* @param {(boolean) => void} callback Function to call if the cached value is valid
* @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler
* @param {boolean} allowErrorStatusCodes
*/
constructor (callback, handler) {
constructor (callback, handler, allowErrorStatusCodes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For stale-if-error, error status codes when revalidating mean to use the stale response

if (typeof callback !== 'function') {
throw new TypeError('callback must be a function')
}

this.#callback = callback
this.#handler = handler
this.#allowErrorStatusCodes = allowErrorStatusCodes
}

onRequestStart (controller, context) {
onRequestStart (_, context) {
this.#successful = false
this.#context = context
}
Expand All @@ -59,7 +68,9 @@ class CacheRevalidationHandler {
assert(this.#callback != null)

// https://www.rfc-editor.org/rfc/rfc9111.html#name-handling-a-validation-respo
this.#successful = statusCode === 304
// https://datatracker.ietf.org/doc/html/rfc5861#section-4
this.#successful = statusCode === 304 ||
(this.#allowErrorStatusCodes && statusCode >= 500 && statusCode <= 504)
this.#callback(this.#successful, this.#context)
this.#callback = null

Expand Down
Loading
Loading