From f69a8b2e6ff3786b67185a2040f56535e763aad9 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Tue, 27 Aug 2024 18:49:05 -0700 Subject: [PATCH 01/58] feat: http caching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements bare-bones http caching as per rfc9111 Closes #3231 Closes #2760 Closes #2256 Closes #1146 Co-authored-by: Carlos Fuentes Co-authored-by: Robert Nagy Co-authored-by: Isak Törnros Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- index.js | 7 +- lib/cache/memory-cache-store.js | 101 +++++++ lib/handler/cache-handler.js | 353 ++++++++++++++++++++++ lib/handler/cache-revalidation-handler.js | 105 +++++++ lib/interceptor/cache.js | 158 ++++++++++ lib/util/cache.js | 161 ++++++++++ test/cache-interceptor/cache-stores.js | 167 ++++++++++ test/cache-interceptor/interceptor.js | 134 ++++++++ test/cache-interceptor/utils.js | 164 ++++++++++ types/cache-interceptor.d.ts | 79 +++++ types/index.d.ts | 3 + types/interceptors.d.ts | 3 + 12 files changed, 1434 insertions(+), 1 deletion(-) create mode 100644 lib/cache/memory-cache-store.js create mode 100644 lib/handler/cache-handler.js create mode 100644 lib/handler/cache-revalidation-handler.js create mode 100644 lib/interceptor/cache.js create mode 100644 lib/util/cache.js create mode 100644 test/cache-interceptor/cache-stores.js create mode 100644 test/cache-interceptor/interceptor.js create mode 100644 test/cache-interceptor/utils.js create mode 100644 types/cache-interceptor.d.ts diff --git a/index.js b/index.js index 444706560ae..be2269ad90e 100644 --- a/index.js +++ b/index.js @@ -39,7 +39,12 @@ module.exports.RedirectHandler = RedirectHandler module.exports.interceptors = { redirect: require('./lib/interceptor/redirect'), retry: require('./lib/interceptor/retry'), - dump: require('./lib/interceptor/dump') + dump: require('./lib/interceptor/dump'), + cache: require('./lib/interceptor/cache') +} + +module.exports.cacheStores = { + MemoryCacheStore: require('./lib/cache/memory-cache-store') } module.exports.buildConnector = buildConnector diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js new file mode 100644 index 00000000000..c43ae358f3c --- /dev/null +++ b/lib/cache/memory-cache-store.js @@ -0,0 +1,101 @@ +'use strict' + +/** + * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore + * @implements {CacheStore} + */ +class MemoryCacheStore { + /** + * @type {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts} opts + */ + #opts = {} + /** + * @type {Map} + */ + #data = new Map() + + /** + * @param {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts | undefined} opts + */ + constructor (opts) { + this.#opts = opts ?? {} + + if (!this.#opts.maxEntrySize) { + this.#opts.maxEntrySize = Infinity + } + } + + get maxEntrySize () { + return this.#opts.maxEntrySize + } + + /** + * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req + * @returns {Promise} + */ + get (req) { + const key = this.#makeKey(req) + + const values = this.#data.get(key) + if (!values) { + return + } + + let value + const now = Date.now() + for (let i = values.length - 1; i >= 0; i--) { + const current = values[i] + if (now >= current.deleteAt) { + // Delete the expired ones + values.splice(i, 1) + continue + } + + let matches = true + + if (current.vary) { + for (const key in current.vary) { + if (current.vary[key] !== req.headers[key]) { + matches = false + break + } + } + } + + if (matches) { + value = current + break + } + } + + return value + } + + /** + * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req + * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue} value + */ + put (req, value) { + const key = this.#makeKey(req) + + let values = this.#data.get(key) + if (!values) { + values = [] + this.#data.set(key, values) + } + + values.push(value) + } + + /** + * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req + * @returns {string} + */ + #makeKey (req) { + // TODO origin is undefined + // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 + return `${req.origin}:${req.path}:${req.method}` + } +} + +module.exports = MemoryCacheStore diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js new file mode 100644 index 00000000000..9ea2e41666c --- /dev/null +++ b/lib/handler/cache-handler.js @@ -0,0 +1,353 @@ +'use strict' + +const util = require('../core/util') +const DecoratorHandler = require('../handler/decorator-handler') +const { parseCacheControlHeader, parseVaryHeader } = require('../util/cache') + +class CacheHandler extends DecoratorHandler { + /** + * @type {import('../../types/cache-interceptor.d.ts').default.CacheOptions} + */ + #opts = null + /** + * @type {import('../../types/dispatcher.d.ts').default.RequestOptions} + */ + #req = null + /** + * @type {import('../../types/dispatcher.d.ts').default.DispatchHandlers} + */ + #handler = null + /** + * @type {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue | undefined} + */ + #value = null + + /** + * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions} opts + * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req + * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler + */ + constructor (opts, req, handler) { + super(handler) + + this.#opts = opts + this.#req = req + this.#handler = handler + } + + /** + * @see {DispatchHandlers.onHeaders} + * + * @param {number} statusCode + * @param {Buffer[]} rawHeaders + * @param {() => void} resume + * @param {string} statusMessage + * @param {string[] | undefined} headers + * @returns + */ + onHeaders ( + statusCode, + rawHeaders, + resume, + statusMessage, + headers = util.parseHeaders(rawHeaders) + ) { + const cacheControlHeader = headers['cache-control'] + const contentLengthHeader = headers['content-length'] + + if (!cacheControlHeader || !contentLengthHeader) { + // Don't have the headers we need, can't cache + return this.#handler.onHeaders( + statusCode, + rawHeaders, + resume, + statusMessage, + headers + ) + } + + const maxEntrySize = this.#getMaxEntrySize() + const contentLength = Number(headers['content-length']) + const currentSize = + this.#getSizeOfBuffers(rawHeaders) + (statusMessage?.length ?? 0) + 64 + if ( + isNaN(contentLength) || + contentLength > maxEntrySize || + currentSize > maxEntrySize + ) { + return this.#handler.onHeaders( + statusCode, + rawHeaders, + resume, + statusMessage, + headers + ) + } + + const cacheControlDirectives = parseCacheControlHeader(cacheControlHeader) + if (!canCacheResponse(statusCode, headers, cacheControlDirectives)) { + return this.#handler.onHeaders( + statusCode, + rawHeaders, + resume, + statusMessage, + headers + ) + } + + const now = Date.now() + const staleAt = determineStaleAt(headers, cacheControlDirectives) + if (staleAt) { + const varyDirectives = headers.vary + ? parseVaryHeader(headers.vary, this.#req.headers) + : undefined + const deleteAt = determineDeleteAt(cacheControlDirectives, staleAt) + + const strippedHeaders = stripNecessaryHeaders( + rawHeaders, + headers, + cacheControlDirectives + ) + + this.#value = { + complete: false, + statusCode, + statusMessage, + rawHeaders: strippedHeaders, + body: [], + vary: varyDirectives, + size: currentSize, + cachedAt: now, + staleAt: now + staleAt, + deleteAt: now + deleteAt + } + } + + return this.#handler.onHeaders( + statusCode, + rawHeaders, + resume, + statusMessage, + headers + ) + } + + /** + * @see {DispatchHandlers.onData} + * + * @param {Buffer} chunk + * @returns {boolean} + */ + onData (chunk) { + if (this.#value) { + this.#value.size += chunk.length + + if (this.#value.size > this.#getMaxEntrySize()) { + this.#value = null + } else { + this.#value.body.push(chunk) + } + } + + return this.#handler.onData(chunk) + } + + /** + * @see {DispatchHandlers.onComplete} + * + * @param {string[] | null} rawTrailers + */ + onComplete (rawTrailers) { + if (this.#value) { + this.#value.complete = true + this.#value.rawTrailers = rawTrailers + this.#value.size += this.#getSizeOfBuffers(rawTrailers) + + // If we're still under the max entry size, let's add it to the cache + if (this.#getMaxEntrySize() > this.#value.size) { + const result = this.#opts.store.put(this.#req, this.#value) + if (result && result.constructor.name === 'Promise') { + result.catch(err => { + throw err + }) + } + } + } + + return this.#handler.onComplete(rawTrailers) + } + + /** + * @see {DispatchHandlers.onError} + * + * @param {Error} err + */ + onError (err) { + this.#value = undefined + this.#handler.onError(err) + } + + /** + * @returns {number} + */ + #getMaxEntrySize () { + return this.#opts.store.maxEntrySize ?? Infinity + } + + /** + * @param {string[] | Buffer[]} arr + * @returns {number} + */ + #getSizeOfBuffers (arr) { + let size = 0 + + for (const buffer of arr) { + size += buffer.length + } + + return size + } +} + +/** + * @see https://www.rfc-editor.org/rfc/rfc9111.html#name-storing-responses-to-authen + * + * @param {number} statusCode + * @param {Record} headers + * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives + */ +function canCacheResponse (statusCode, headers, cacheControlDirectives) { + if (![200, 307].includes(statusCode)) { + return false + } + + if ( + !cacheControlDirectives.public && + !cacheControlDirectives['s-maxage'] && + !cacheControlDirectives['must-revalidate'] + ) { + // Response can't be used in a shared cache + return false + } + + if ( + // TODO double check these + cacheControlDirectives.private === true || + cacheControlDirectives['no-cache'] === true || + cacheControlDirectives['no-store'] || + cacheControlDirectives['no-transform'] || + cacheControlDirectives['must-understand'] || + cacheControlDirectives['proxy-revalidate'] + ) { + return false + } + + // https://www.rfc-editor.org/rfc/rfc9111.html#section-4.1-5 + if (headers.vary === '*') { + return false + } + + // https://www.rfc-editor.org/rfc/rfc9111.html#name-storing-responses-to-authen + if (headers['authorization']) { + if ( + Array.isArray(cacheControlDirectives['no-cache']) && + cacheControlDirectives['no-cache'].includes('authorization') + ) { + return false + } + + if ( + Array.isArray(cacheControlDirectives['private']) && + cacheControlDirectives['private'].includes('authorization') + ) { + return false + } + } + + return true +} + +/** + * @param {Record} headers + * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives + * + * @returns {number | undefined} time that the value is stale at or undefined if it shouldn't be cached + */ +function determineStaleAt (headers, cacheControlDirectives) { + // Prioritize s-maxage since we're a shared cache + // s-maxage > max-age > Expire + // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.10-3 + const sMaxAge = cacheControlDirectives['s-maxage'] + if (sMaxAge) { + return sMaxAge * 1000 + } + + if (cacheControlDirectives.immutable) { + // https://www.rfc-editor.org/rfc/rfc8246.html#section-2.2 + return 31536000 + } + + const maxAge = cacheControlDirectives['max-age'] + if (maxAge) { + return maxAge * 1000 + } + + if (headers.expire) { + // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.3 + return new Date() - new Date(headers.expire) + } + + return undefined +} + +/** + * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives + * @param {number} staleAt + */ +function determineDeleteAt (cacheControlDirectives, staleAt) { + if (cacheControlDirectives['stale-while-revalidate']) { + return (cacheControlDirectives['stale-while-revalidate'] * 1000) + } + + return staleAt +} + +/** + * Strips headers required to be removed in cached responses + * @param {Buffer[]} rawHeaders + * @param {string[]} parsedHeaders + * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives + * @returns {Buffer[]} + */ +function stripNecessaryHeaders (rawHeaders, parsedHeaders, cacheControlDirectives) { + const headersToRemove = ['connection'] + + if (Array.isArray(cacheControlDirectives['no-cache'])) { + headersToRemove.push(...cacheControlDirectives['no-cache']) + } + + if (Array.isArray(cacheControlDirectives['private'])) { + headersToRemove.push(...cacheControlDirectives['private']) + } + + let strippedRawHeaders + for (let i = 0; i < parsedHeaders.length; i++) { + const header = parsedHeaders[i] + const kvDelimiterIndex = header.indexOf(':') + const headerName = header.substring(0, kvDelimiterIndex) + + if (headerName in headersToRemove) { + if (!strippedRawHeaders) { + strippedRawHeaders = rawHeaders.slice(0, i - 1) + } else { + strippedRawHeaders.push(rawHeaders[i]) + } + } + } + + strippedRawHeaders ??= rawHeaders + + return strippedRawHeaders +} + +module.exports = CacheHandler diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js new file mode 100644 index 00000000000..5ea70c20070 --- /dev/null +++ b/lib/handler/cache-revalidation-handler.js @@ -0,0 +1,105 @@ +'use strict' + +const DecoratorHandler = require('../handler/decorator-handler') + +/** + * This takes care of revalidation requests we send to the origin. If we get + * a response indicating that what we have is cached (via a HTTP 304), we can + * continue using the cached value. Otherwise, we'll receive the new response + * here, which we then just pass on to the next handler (most likely a + * CacheHandler). Note that this assumes the proper headers were already + * included in the request to tell the origin that we want to revalidate the + * response (i.e. if-modified-since). + * + * @see https://www.rfc-editor.org/rfc/rfc9111.html#name-validation + * + * @typedef {import('../../types/dispatcher.d.ts').default.DispatchHandlers} DispatchHandlers + * @implements {DispatchHandlers} + */ +class CacheRevalidationHandler extends DecoratorHandler { + #successful = false + /** + * @type {() => void} + */ + #successCallback = null + /** + * @type {import('../../types/dispatcher.d.ts').default.DispatchHandlers} + */ + #handler = null + + /** + * @param {() => void} successCallback Function to call if the cached value is valid + * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler + */ + constructor (successCallback, handler) { + super(handler) + + this.#successCallback = successCallback + this.#handler = handler + } + + /** + * @see {DispatchHandlers.onHeaders} + * + * @param {number} statusCode + * @param {Buffer[]} rawHeaders + * @param {() => void} resume + * @param {string} statusMessage + * @param {string[] | undefined} headers + * @returns {boolean} + */ + onHeaders ( + statusCode, + rawHeaders, + resume, + statusMessage, + headers = undefined + ) { + // https://www.rfc-editor.org/rfc/rfc9111.html#name-handling-a-validation-respo + if (statusCode === 304) { + this.#successful = true + this.#successCallback() + return true + } + + return this.#handler.onHeaders( + statusCode, + rawHeaders, + resume, + statusMessage, + headers + ) + } + + /** + * @see {DispatchHandlers.onData} + * + * @param {Buffer} chunk + * @returns {boolean} + */ + onData (chunk) { + return this.#successful ? true : this.#handler.onData(chunk) + } + + /** + * @see {DispatchHandlers.onComplete} + * + * @param {string[] | null} rawTrailers + */ + onComplete (rawTrailers) { + if (!this.#successful) { + this.#handler.onComplete(rawTrailers) + } + } + + /** + * @see {DispatchHandlers.onError} + * + * @param {Error} err + */ + onError (err) { + this.#handler.onError(err) + } +} + +module.exports = CacheRevalidationHandler diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js new file mode 100644 index 00000000000..7967f2aa981 --- /dev/null +++ b/lib/interceptor/cache.js @@ -0,0 +1,158 @@ +'use strict' + +const CacheHandler = require('../handler/cache-handler') +const MemoryCacheStore = require('../cache/memory-cache-store') +const CacheRevalidationHandler = require('../handler/cache-revalidation-handler') + +/** + * Gives the downstream handler the request's cached response or dispatches + * it if it isn't cached + * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions} globalOpts + * @param {*} dispatch TODO type + * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} opts + * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler + * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue | undefined} value + */ +function handleCachedResult ( + globalOpts, + dispatch, + opts, + handler, + value +) { + if (!value) { + // Request isn't cached, let's continue dispatching it + dispatch(opts, new CacheHandler(globalOpts, opts, handler)) + return + } + + // Dump body on error + opts.body?.on('error', () => {}).resume() + + const respondWithCachedValue = () => { + const ac = new AbortController() + const signal = ac.signal + + try { + handler.onConnect(ac.abort) + signal.throwIfAborted() + + // Add the age header + // https://www.rfc-editor.org/rfc/rfc9111.html#name-age + const age = Math.round((Date.now() - value.cachedAt) / 1000) + + // Copy the headers in case we got this from an in-memory store. We don't + // want to modify the original response. + const headers = [...value.rawHeaders] + headers.push(Buffer.from('age'), Buffer.from(`${age}`)) + + handler.onHeaders(value.statusCode, headers, () => {}, value.statusMessage) + signal.throwIfAborted() + + if (opts.method === 'HEAD') { + handler.onComplete([]) + } else { + for (const chunk of value.body) { + while (!handler.onData(chunk)) { + signal.throwIfAborted() + } + } + + handler.onComplete(value.rawTrailers ?? null) + } + } catch (err) { + handler.onError(err) + } + } + + // Check if the response is stale + const now = Date.now() + if (now > value.staleAt) { + if (now > value.deleteAt) { + // Safety check in case the store gave us a response that should've been + // deleted already + dispatch(opts, new CacheHandler(globalOpts, opts, handler)) + return + } + + // Need to revalidate the request + opts.headers.push(`if-modified-since: ${new Date(value.cachedAt).toUTCString()}`) + + dispatch( + opts, + new CacheRevalidationHandler( + respondWithCachedValue, + new CacheHandler(globalOpts, opts, handler) + ) + ) + + return + } + + // Response is still fresh, let's return it + respondWithCachedValue() +} + +/** + * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions | undefined} globalOpts + * @returns {import('../../types/dispatcher.d.ts').default.DispatcherComposeInterceptor} + */ +module.exports = globalOpts => { + if (!globalOpts) { + globalOpts = {} + } + + if (globalOpts.store) { + for (const fn of ['get', 'put', 'delete']) { + if (typeof globalOpts.store[fn] !== 'function') { + throw new Error(`CacheStore needs a \`${fn}()\` function`) + } + } + + if (!globalOpts.store.maxEntrySize) { + throw new Error('CacheStore needs a maxEntrySize getter') + } + + if (globalOpts.store.maxEntrySize <= 0) { + throw new Error('CacheStore maxEntrySize needs to be >= 1') + } + } else { + globalOpts.store = new MemoryCacheStore() + } + + if (!globalOpts.methods) { + globalOpts.methods = ['GET'] + } + + return dispatch => { + return (opts, handler) => { + if (!globalOpts.methods.includes(opts.method)) { + // Not a method we want to cache, skip + return dispatch(opts, handler) + } + + const result = globalOpts.store.get(opts) + if (result && result.constructor.name === 'Promise') { + result.then(value => { + handleCachedResult( + globalOpts, + dispatch, + opts, + handler, + value + ) + }) + } else { + handleCachedResult( + globalOpts, + dispatch, + opts, + handler, + result + ) + } + + return true + } + } +} diff --git a/lib/util/cache.js b/lib/util/cache.js new file mode 100644 index 00000000000..f6b7d50d09f --- /dev/null +++ b/lib/util/cache.js @@ -0,0 +1,161 @@ +/** + * @see https://www.rfc-editor.org/rfc/rfc9111.html#name-cache-control + * @see https://www.iana.org/assignments/http-cache-directives/http-cache-directives.xhtml + * + * @typedef {{ + * 'max-stale'?: number; + * 'min-fresh'?: number; + * 'max-age'?: number; + * 's-maxage'?: number; + * 'stale-while-revalidate'?: number; + * 'stale-if-error'?: number; + * public?: true; + * private?: true; + * 'no-store'?: true; + * 'no-cache'?: true | string[]; + * 'must-revalidate'?: true; + * 'proxy-revalidate'?: true; + * immutable?: true; + * 'no-transform'?: true; + * 'must-understand'?: true; + * 'only-if-cached'?: true; + * }} CacheControlDirectives + * + * @param {string} header + * @returns {CacheControlDirectives} + */ +function parseCacheControlHeader (header) { + /** + * @type {import('../util/cache.js').CacheControlDirectives} + */ + const output = {} + + const directives = header.toLowerCase().split(',') + for (let i = 0; i < directives.length; i++) { + const directive = directives[i] + const keyValueDelimiter = directive.indexOf('=') + + let key + let value + if (keyValueDelimiter !== -1) { + key = directive.substring(0, keyValueDelimiter).trim() + value = directive + .substring(keyValueDelimiter + 1, directive.length) + .trim() + .toLowerCase() + } else { + key = directive.trim() + } + + switch (key) { + case 'min-fresh': + case 'max-stale': + case 'max-age': + case 's-maxage': + case 'stale-while-revalidate': + case 'stale-if-error': { + const parsedValue = parseInt(value, 10) + if (isNaN(parsedValue)) { + continue + } + + output[key] = parsedValue + + break + } + case 'private': + case 'no-cache': { + if (value) { + // The private and no-cache directives can be unqualified (aka just + // `private` or `no-cache`) or qualified (w/ a value). When they're + // qualified, it's a list of headers like `no-cache=header1`, + // `no-cache="header1"`, or `no-cache="header1, header2"` + // If we're given multiple headers, the comma messes us up since + // we split the full header by commas. So, let's loop through the + // remaining parts in front of us until we find one that ends in a + // quote. We can then just splice all of the parts in between the + // starting quote and the ending quote out of the directives array + // and continue parsing like normal. + // https://www.rfc-editor.org/rfc/rfc9111.html#name-no-cache-2 + if (value[0] === '"') { + // Something like `no-cache="some-header"` OR `no-cache="some-header, another-header"`. + + // Add the first header on and cut off the leading quote + const headers = [value.substring(1)] + + let foundEndingQuote = false + for (let j = i; j < directives.length; j++) { + const nextPart = directives[j] + if (nextPart.endsWith('"')) { + foundEndingQuote = true + headers.push(...directives.splice(i + 1, j - 1).map(header => header.trim())) + + const lastHeader = header[headers.length - 1] + headers[headers.length - 1] = lastHeader.substring(0, lastHeader.length - 1) + break + } + } + + if (!foundEndingQuote) { + // Something like `no-cache="some-header` with no end quote, + // let's just ignore it + continue + } + + output[key] = headers + } else { + // Something like `no-cache=some-header` + output[key] = [value] + } + + break + } + } + // eslint-disable-next-line no-fallthrough + case 'public': + case 'no-store': + case 'must-revalidate': + case 'proxy-revalidate': + case 'immutable': + case 'no-transform': + case 'must-understand': + case 'only-if-cached': + if (value) { + continue + } + + output[key] = true + break + default: + // Ignore unknown directives as per https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.3-1 + continue + } + } + + return output +} + +/** + * @param {string} varyHeader Vary header from the server + * @param {Record} headers Request headers + * @returns {Record} + */ +function parseVaryHeader (varyHeader, headers) { + const output = {} + + const varyingHeaders = varyHeader.toLowerCase().split(',') + for (const header of varyingHeaders) { + const trimmedHeader = header.trim() + + if (headers[trimmedHeader]) { + output[trimmedHeader] = headers[trimmedHeader] + } + } + + return output +} + +module.exports = { + parseCacheControlHeader, + parseVaryHeader +} diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js new file mode 100644 index 00000000000..70bb39fff13 --- /dev/null +++ b/test/cache-interceptor/cache-stores.js @@ -0,0 +1,167 @@ +const { describe, test } = require('node:test') +const { deepStrictEqual, strictEqual } = require('node:assert') +const MemoryCacheStore = require('../../lib/cache/memory-cache-store') + +const cacheStoresToTest = [ + MemoryCacheStore +] + +for (const CacheStore of cacheStoresToTest) { + describe(CacheStore.prototype.constructor.name, () => { + test('basic functionality', async () => { + // Checks that it can store & fetch different responses + const store = new CacheStore() + + const request = { + origin: 'localhost', + path: '/', + method: 'GET', + headers: {} + } + const requestValue = { + complete: true, + statusCode: 200, + statusMessage: '', + rawHeaders: [1, 2, 3], + rawTrailers: [4, 5, 6], + body: ['part1', 'part2'], + size: 16, + cachedAt: Date.now(), + staleAt: Date.now() + 10000, + deleteAt: Date.now() + 20000 + } + + // Sanity check + strictEqual(await store.get(request), undefined) + + // Add a response to the cache and try fetching it with a deep copy of + // the original request + await store.put(request, requestValue) + deepStrictEqual(store.get(structuredClone(request)), requestValue) + + const anotherRequest = { + origin: 'localhost', + path: '/asd', + method: 'GET', + headers: {} + } + const anotherValue = { + complete: true, + statusCode: 200, + statusMessage: '', + rawHeaders: [1, 2, 3], + rawTrailers: [4, 5, 6], + body: ['part1', 'part2'], + size: 16, + cachedAt: Date.now(), + staleAt: Date.now() + 10000, + deleteAt: Date.now() + 20000 + } + + // We haven't cached this one yet, make sure it doesn't confuse it with + // another request + strictEqual(await store.get(anotherRequest), undefined) + + // Add a response to the cache and try fetching it with a deep copy of + // the original request + await store.put(anotherRequest, anotherValue) + deepStrictEqual(store.get(structuredClone(anotherRequest)), anotherValue) + }) + + test('returns stale response if possible', async () => { + const request = { + origin: 'localhost', + path: '/', + method: 'GET', + headers: {} + } + const requestValue = { + complete: true, + statusCode: 200, + statusMessage: '', + rawHeaders: [1, 2, 3], + rawTrailers: [4, 5, 6], + body: ['part1', 'part2'], + size: 16, + cachedAt: Date.now() - 10000, + staleAt: Date.now() - 1, + deleteAt: Date.now() + 20000 + } + + const store = new CacheStore() + await store.put(request, requestValue) + deepStrictEqual(await store.get(request), requestValue) + }) + + test('doesn\'t return response past deletedAt', async () => { + const request = { + origin: 'localhost', + path: '/', + method: 'GET', + headers: {} + } + const requestValue = { + complete: true, + statusCode: 200, + statusMessage: '', + rawHeaders: [1, 2, 3], + rawTrailers: [4, 5, 6], + body: ['part1', 'part2'], + size: 16, + cachedAt: Date.now() - 20000, + staleAt: Date.now() - 10000, + deleteAt: Date.now() - 5 + } + + const store = new CacheStore() + await store.put(request, requestValue) + strictEqual(await store.get(request), undefined) + }) + + test('respects vary directives', async () => { + const store = new CacheStore() + + const request = { + origin: 'localhost', + path: '/', + method: 'GET', + headers: { + 'some-header': 'hello world' + } + } + const requestValue = { + complete: true, + statusCode: 200, + statusMessage: '', + rawHeaders: [1, 2, 3], + rawTrailers: [4, 5, 6], + body: ['part1', 'part2'], + vary: { + 'some-header': 'hello world' + }, + size: 16, + cachedAt: Date.now(), + staleAt: Date.now() + 10000, + deleteAt: Date.now() + 20000 + } + + // Sanity check + strictEqual(await store.get(request), undefined) + + await store.put(request, requestValue) + deepStrictEqual(store.get(request), requestValue) + + const nonMatchingRequest = { + origin: 'localhost', + path: '/', + method: 'GET', + headers: { + 'some-header': 'another-value' + } + } + deepStrictEqual(store.get(nonMatchingRequest), undefined) + + deepStrictEqual(store.get(structuredClone(request)), requestValue) + }) + }) +} diff --git a/test/cache-interceptor/interceptor.js b/test/cache-interceptor/interceptor.js new file mode 100644 index 00000000000..2303649ebe5 --- /dev/null +++ b/test/cache-interceptor/interceptor.js @@ -0,0 +1,134 @@ +'use strict' + +const { describe, test, after } = require('node:test') +const { strictEqual } = require('node:assert') +const { createServer } = require('node:http') +const { Client, interceptors } = require('../../index') +const { once } = require('node:events') + +// e2e tests, checks just the public api stuff basically +describe('Cache Interceptor', () => { + test('doesn\'t cache request w/ no cache-control header', async () => { + let requestsToOrigin = 0 + + const server = createServer((_, res) => { + requestsToOrigin++ + res.end('asd') + }).listen(0) + + after(() => server.close()) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + strictEqual(requestsToOrigin, 0) + + // Send initial request. This should reach the origin + let response = await client.request({ method: 'GET', path: '/' }) + strictEqual(requestsToOrigin, 1) + strictEqual(await getResponse(response.body), 'asd') + + // Send second request that should be handled by cache + response = await client.request({ method: 'GET', path: '/' }) + strictEqual(requestsToOrigin, 2) + strictEqual(await getResponse(response.body), 'asd') + }) + + test('caches request successfully', async () => { + let requestsToOrigin = 0 + + const server = createServer((_, res) => { + requestsToOrigin++ + res.setHeader('cache-control', 'public, s-maxage=10') + res.end('asd') + }).listen(0) + + after(() => server.close()) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + strictEqual(requestsToOrigin, 0) + + // Send initial request. This should reach the origin + let response = await client.request({ method: 'GET', path: '/' }) + strictEqual(requestsToOrigin, 1) + strictEqual(await getResponse(response.body), 'asd') + + // Send second request that should be handled by cache + response = await client.request({ method: 'GET', path: '/' }) + strictEqual(requestsToOrigin, 1) + strictEqual(await getResponse(response.body), 'asd') + strictEqual(response.headers.age, '0') + }) + + test('respects vary header', async () => { + let requestsToOrigin = 0 + + const server = createServer((req, res) => { + requestsToOrigin++ + res.setHeader('cache-control', 'public, s-maxage=10') + res.setHeader('vary', 'some-header, another-header') + + if (req.headers['some-header'] === 'abc123') { + res.end('asd') + } else { + res.end('dsa') + } + }).listen(0) + + after(() => server.close()) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + strictEqual(requestsToOrigin, 0) + + // Send initial request. This should reach the origin + let response = await client.request({ + method: 'GET', + path: '/', + headers: { + 'some-header': 'abc123', + 'another-header': '123abc' + } + }) + strictEqual(requestsToOrigin, 1) + strictEqual(await getResponse(response.body), 'asd') + + // Make another request with changed headers, this should miss + const secondResponse = await client.request({ + method: 'GET', + path: '/', + headers: { + 'some-header': 'qwerty', + 'another-header': 'asdfg' + } + }) + strictEqual(requestsToOrigin, 2) + strictEqual(await getResponse(secondResponse.body), 'dsa') + + // Resend the first request again which should still be cahced + response = await client.request({ + method: 'GET', + path: '/', + headers: { + 'some-header': 'abc123', + 'another-header': '123abc' + } + }) + strictEqual(requestsToOrigin, 2) + strictEqual(await getResponse(response.body), 'asd') + }) +}) + +async function getResponse (body) { + const buffers = [] + for await (const data of body) { + buffers.push(data) + } + return Buffer.concat(buffers).toString('utf8') +} diff --git a/test/cache-interceptor/utils.js b/test/cache-interceptor/utils.js new file mode 100644 index 00000000000..9b4e247ddca --- /dev/null +++ b/test/cache-interceptor/utils.js @@ -0,0 +1,164 @@ +'use strict' + +const { describe, test } = require('node:test') +const { deepStrictEqual } = require('node:assert') +const { parseCacheControlHeader, parseVaryHeader } = require('../../lib/util/cache') + +describe('parseCacheControlHeader', () => { + test('all directives are parsed properly when in their correct format', () => { + const directives = parseCacheControlHeader( + 'max-stale=1, min-fresh=1, max-age=1, s-maxage=1, stale-while-revalidate=1, stale-if-error=1, public, private, no-store, no-cache, must-revalidate, proxy-revalidate, immutable, no-transform, must-understand, only-if-cached' + ) + deepStrictEqual(directives, { + 'max-stale': 1, + 'min-fresh': 1, + 'max-age': 1, + 's-maxage': 1, + 'stale-while-revalidate': 1, + 'stale-if-error': 1, + public: true, + private: true, + 'no-store': true, + 'no-cache': true, + 'must-revalidate': true, + 'proxy-revalidate': true, + immutable: true, + 'no-transform': true, + 'must-understand': true, + 'only-if-cached': true + }) + }) + + test('handles weird spacings', () => { + const directives = parseCacheControlHeader( + 'max-stale=1, min-fresh=1, max-age=1,s-maxage=1, stale-while-revalidate=1,stale-if-error=1,public,private' + ) + deepStrictEqual(directives, { + 'max-stale': 1, + 'min-fresh': 1, + 'max-age': 1, + 's-maxage': 1, + 'stale-while-revalidate': 1, + 'stale-if-error': 1, + public: true, + private: true + }) + }) + + test('unknown directives are ignored', () => { + const directives = parseCacheControlHeader('max-age=123, something-else=456') + deepStrictEqual(directives, { 'max-age': 123 }) + }) + + test('directives with incorrect types are ignored', () => { + const directives = parseCacheControlHeader('max-age=true, only-if-cached=123') + deepStrictEqual(directives, {}) + }) + + test('the last instance of a directive takes precedence', () => { + const directives = parseCacheControlHeader('max-age=1, max-age=2') + deepStrictEqual(directives, { 'max-age': 2 }) + }) + + test('case insensitive', () => { + const directives = parseCacheControlHeader('Max-Age=123') + deepStrictEqual(directives, { 'max-age': 123 }) + }) + + test('no-cache with headers', () => { + let directives = parseCacheControlHeader('max-age=10, no-cache=some-header, only-if-cached') + deepStrictEqual(directives, { + 'max-age': 10, + 'no-cache': [ + 'some-header' + ], + 'only-if-cached': true + }) + + directives = parseCacheControlHeader('max-age=10, no-cache="some-header", only-if-cached') + deepStrictEqual(directives, { + 'max-age': 10, + 'no-cache': [ + 'some-header' + ], + 'only-if-cached': true + }) + + directives = parseCacheControlHeader('max-age=10, no-cache="some-header, another-one", only-if-cached') + deepStrictEqual(directives, { + 'max-age': 10, + 'no-cache': [ + 'some-header', + 'another-one' + ], + 'only-if-cached': true + }) + }) + + test('private with headers', () => { + let directives = parseCacheControlHeader('max-age=10, private=some-header, only-if-cached') + deepStrictEqual(directives, { + 'max-age': 10, + private: [ + 'some-header' + ], + 'only-if-cached': true + }) + + directives = parseCacheControlHeader('max-age=10, private="some-header", only-if-cached') + deepStrictEqual(directives, { + 'max-age': 10, + private: [ + 'some-header' + ], + 'only-if-cached': true + }) + + directives = parseCacheControlHeader('max-age=10, private="some-header, another-one", only-if-cached') + deepStrictEqual(directives, { + 'max-age': 10, + private: [ + 'some-header', + 'another-one' + ], + 'only-if-cached': true + }) + + // Missing ending quote, invalid & should be skipped + directives = parseCacheControlHeader('max-age=10, private="some-header, another-one, only-if-cached') + deepStrictEqual(directives, { + 'max-age': 10, + 'only-if-cached': true + }) + }) +}) + +describe('parseVaryHeader', () => { + test('basic usage', () => { + const output = parseVaryHeader({ + vary: 'some-header, another-one', + 'some-header': 'asd', + 'another-one': '123', + 'third-header': 'cool' + }) + deepStrictEqual(output, new Map([ + ['some-header', 'asd'], + ['another-one', '123'] + ])) + }) + + test('handles weird spacings', () => { + const output = parseVaryHeader({ + vary: 'some-header, another-one,something-else', + 'some-header': 'asd', + 'another-one': '123', + 'something-else': 'asd123', + 'third-header': 'cool' + }) + deepStrictEqual(output, new Map([ + ['some-header', 'asd'], + ['another-one', '123'], + ['something-else', 'asd123'] + ])) + }) +}) diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts new file mode 100644 index 00000000000..a7275871960 --- /dev/null +++ b/types/cache-interceptor.d.ts @@ -0,0 +1,79 @@ +import Dispatcher from './dispatcher' + +export default CacheHandler + +declare namespace CacheHandler { + export interface CacheOptions { + store?: CacheStore + + /** + * The methods to cache, defaults to just GET + */ + methods?: ('GET' | 'HEAD' | 'POST' | 'PATCH')[] + } + + /** + * Underlying storage provider for cached responses + */ + export interface CacheStore { + /** + * The max size of each value. If the content-length header is greater than + * this or the response ends up over this, the response will not be cached + * @default Infinity + */ + get maxEntrySize(): number + + get(key: Dispatcher.RequestOptions): CacheStoreValue[] | Promise; + + put(key: Dispatcher.RequestOptions, opts: CacheStoreValue): void | Promise; + } + + export interface CacheStoreValue { + /** + * True if the response is complete, otherwise the request is still in-flight + */ + complete: boolean; + statusCode: number; + statusMessage: string; + rawHeaders: Buffer[]; + rawTrailers?: Buffer[]; + body: string[] + /** + * Headers defined by the Vary header and their respective values for + * later comparison + */ + vary?: Record; + /** + * Actual size of the response (i.e. size of headers + body + trailers) + */ + size: number; + /** + * Time in millis that this value was cached + */ + cachedAt: number; + /** + * Time in millis that this value is considered stale + */ + staleAt: number; + /** + * Time in millis that this value is to be deleted from the cache. This is + * either the same as staleAt or the `max-stale` caching directive. + */ + deleteAt: number; + } + + export interface MemoryCacheStoreOpts { + /** + * @default Infinity + */ + maxEntrySize?: number + } + + export class MemoryCacheStore implements CacheStore { + constructor (opts?: MemoryCacheStoreOpts) + + get maxEntrySize (): number + get (key: Dispatcher.RequestOptions): CacheStoreValue[] | Promise + put (key: Dispatcher.RequestOptions, opts: CacheStoreValue): Promise + } +} diff --git a/types/index.d.ts b/types/index.d.ts index 45276234925..fed36ab8643 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -64,4 +64,7 @@ declare namespace Undici { const FormData: typeof import('./formdata').FormData const caches: typeof import('./cache').caches const interceptors: typeof import('./interceptors').default + const cacheStores: { + MemoryCacheStore: typeof import('./cache-interceptor').default.MemoryCacheStore + } } diff --git a/types/interceptors.d.ts b/types/interceptors.d.ts index 53835e01299..ee15b1c6f0e 100644 --- a/types/interceptors.d.ts +++ b/types/interceptors.d.ts @@ -1,3 +1,4 @@ +import CacheHandler from './cache-interceptor' import Dispatcher from './dispatcher' import RetryHandler from './retry-handler' @@ -8,10 +9,12 @@ declare namespace Interceptors { export type RetryInterceptorOpts = RetryHandler.RetryOptions export type RedirectInterceptorOpts = { maxRedirections?: number } export type ResponseErrorInterceptorOpts = { throwOnError: boolean } + export type CacheInterceptorOpts = CacheHandler.CacheOptions export function createRedirectInterceptor (opts: RedirectInterceptorOpts): Dispatcher.DispatcherComposeInterceptor export function dump (opts?: DumpInterceptorOpts): Dispatcher.DispatcherComposeInterceptor export function retry (opts?: RetryInterceptorOpts): Dispatcher.DispatcherComposeInterceptor export function redirect (opts?: RedirectInterceptorOpts): Dispatcher.DispatcherComposeInterceptor export function responseError (opts?: ResponseErrorInterceptorOpts): Dispatcher.DispatcherComposeInterceptor + export function cache (opts?: CacheInterceptorOpts): Dispatcher.DispatcherComposeInterceptor } From 57c0f459dbd3e99db9a97fc9915452d60e8d6bbd Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Fri, 13 Sep 2024 22:24:29 -0700 Subject: [PATCH 02/58] fixup! feat: http caching Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/handler/cache-handler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 9ea2e41666c..f8fb438e305 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -43,7 +43,7 @@ class CacheHandler extends DecoratorHandler { * @param {() => void} resume * @param {string} statusMessage * @param {string[] | undefined} headers - * @returns + * @returns {boolean} */ onHeaders ( statusCode, From c88fb25cea3b59314d366dcd58683fbda090a3f7 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Sun, 15 Sep 2024 09:56:16 -0700 Subject: [PATCH 03/58] fixup! fixup! feat: http caching Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- test/cache-interceptor/cache-stores.js | 11 +++-- test/cache-interceptor/interceptor.js | 22 +++------ test/types/cache-interceptor.test-d.ts | 66 ++++++++++++++++++++++++++ test/types/index.test-d.ts | 1 + types/cache-interceptor.d.ts | 4 +- 5 files changed, 82 insertions(+), 22 deletions(-) create mode 100644 test/types/cache-interceptor.test-d.ts diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index 70bb39fff13..3bbea2411f4 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -2,11 +2,10 @@ const { describe, test } = require('node:test') const { deepStrictEqual, strictEqual } = require('node:assert') const MemoryCacheStore = require('../../lib/cache/memory-cache-store') -const cacheStoresToTest = [ - MemoryCacheStore -] - -for (const CacheStore of cacheStoresToTest) { +/** + * @param {typeof import('../../types/cache-interceptor').default.CacheStore} CacheStore + */ +function cacheStoreTests(CacheStore) { describe(CacheStore.prototype.constructor.name, () => { test('basic functionality', async () => { // Checks that it can store & fetch different responses @@ -165,3 +164,5 @@ for (const CacheStore of cacheStoresToTest) { }) }) } + +cacheStoreTests(MemoryCacheStore) diff --git a/test/cache-interceptor/interceptor.js b/test/cache-interceptor/interceptor.js index 2303649ebe5..0128ec7b0a0 100644 --- a/test/cache-interceptor/interceptor.js +++ b/test/cache-interceptor/interceptor.js @@ -27,12 +27,12 @@ describe('Cache Interceptor', () => { // Send initial request. This should reach the origin let response = await client.request({ method: 'GET', path: '/' }) strictEqual(requestsToOrigin, 1) - strictEqual(await getResponse(response.body), 'asd') + strictEqual(await response.body.text(), 'asd') // Send second request that should be handled by cache response = await client.request({ method: 'GET', path: '/' }) strictEqual(requestsToOrigin, 2) - strictEqual(await getResponse(response.body), 'asd') + strictEqual(await response.body.text(), 'asd') }) test('caches request successfully', async () => { @@ -55,12 +55,12 @@ describe('Cache Interceptor', () => { // Send initial request. This should reach the origin let response = await client.request({ method: 'GET', path: '/' }) strictEqual(requestsToOrigin, 1) - strictEqual(await getResponse(response.body), 'asd') + strictEqual(await response.body.text(), 'asd') // Send second request that should be handled by cache response = await client.request({ method: 'GET', path: '/' }) strictEqual(requestsToOrigin, 1) - strictEqual(await getResponse(response.body), 'asd') + strictEqual(await response.body.text(), 'asd') strictEqual(response.headers.age, '0') }) @@ -97,7 +97,7 @@ describe('Cache Interceptor', () => { } }) strictEqual(requestsToOrigin, 1) - strictEqual(await getResponse(response.body), 'asd') + strictEqual(await response.body.text(), 'asd') // Make another request with changed headers, this should miss const secondResponse = await client.request({ @@ -109,7 +109,7 @@ describe('Cache Interceptor', () => { } }) strictEqual(requestsToOrigin, 2) - strictEqual(await getResponse(secondResponse.body), 'dsa') + strictEqual(await secondResponse.body.text(), 'dsa') // Resend the first request again which should still be cahced response = await client.request({ @@ -121,14 +121,6 @@ describe('Cache Interceptor', () => { } }) strictEqual(requestsToOrigin, 2) - strictEqual(await getResponse(response.body), 'asd') + strictEqual(await response.body.text(), 'asd') }) }) - -async function getResponse (body) { - const buffers = [] - for await (const data of body) { - buffers.push(data) - } - return Buffer.concat(buffers).toString('utf8') -} diff --git a/test/types/cache-interceptor.test-d.ts b/test/types/cache-interceptor.test-d.ts new file mode 100644 index 00000000000..2692644a5cd --- /dev/null +++ b/test/types/cache-interceptor.test-d.ts @@ -0,0 +1,66 @@ +import { expectAssignable, expectNotAssignable } from 'tsd' +import CacheInterceptor from '../../types/cache-interceptor' +import Dispatcher from '../../types/dispatcher' + +const store: CacheInterceptor.CacheStore = { + maxEntrySize: 0, + + get(_: Dispatcher.RequestOptions): CacheInterceptor.CacheStoreValue | Promise { + throw new Error('stub') + }, + + put(_: Dispatcher.RequestOptions, _2: CacheInterceptor.CacheStoreValue): void | Promise { + throw new Error('stub') + } +} + +expectAssignable({}) +expectAssignable({ store }) +expectAssignable({ methods: [] }) +expectAssignable({ store, methods: ['GET'] }) + +expectAssignable({ + complete: true, + statusCode: 200, + statusMessage: 'OK', + rawHeaders: [], + body: [], + size: 0, + cachedAt: 0, + staleAt: 0, + deleteAt: 0, +}) + +expectAssignable({ + complete: true, + statusCode: 200, + statusMessage: 'OK', + rawHeaders: [], + rawTrailers: [], + body: [], + vary: {}, + size: 0, + cachedAt: 0, + staleAt: 0, + deleteAt: 0, + }) + +expectNotAssignable({}) +expectNotAssignable({ + complete: '', + statusCode: '123', + statusMessage: 123, + rawHeaders: '', + rawTrailers: '', + body: 0, + vary: '', + size: '', + cachedAt: '', + staleAt: '', + deleteAt: '', +}) + +expectAssignable({}) +expectAssignable({ + maxEntrySize: 0 +}) diff --git a/test/types/index.test-d.ts b/test/types/index.test-d.ts index 8fe57aba5e9..3dbfcf48fe5 100644 --- a/test/types/index.test-d.ts +++ b/test/types/index.test-d.ts @@ -14,6 +14,7 @@ expectAssignable(Undici.FormData) expectAssignable(Undici.interceptors.dump()) expectAssignable(Undici.interceptors.redirect()) expectAssignable(Undici.interceptors.retry()) +expectAssignable(Undici.interceptors.cache()) const client = new Undici.Client('', {}) const handler: Dispatcher.DispatchHandlers = {} diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index a7275871960..14d70bad004 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -23,7 +23,7 @@ declare namespace CacheHandler { */ get maxEntrySize(): number - get(key: Dispatcher.RequestOptions): CacheStoreValue[] | Promise; + get(key: Dispatcher.RequestOptions): CacheStoreValue | Promise; put(key: Dispatcher.RequestOptions, opts: CacheStoreValue): void | Promise; } @@ -73,7 +73,7 @@ declare namespace CacheHandler { constructor (opts?: MemoryCacheStoreOpts) get maxEntrySize (): number - get (key: Dispatcher.RequestOptions): CacheStoreValue[] | Promise + get (key: Dispatcher.RequestOptions): CacheStoreValue | Promise put (key: Dispatcher.RequestOptions, opts: CacheStoreValue): Promise } } From 0a442b24bd454ab3ecbae2069ddf6ea56b52e0c6 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Sun, 15 Sep 2024 10:55:26 -0700 Subject: [PATCH 04/58] fixup! fixup! fixup! feat: http caching Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- test/cache-interceptor/cache-stores.js | 4 +-- test/types/cache-interceptor.test-d.ts | 42 +++++++++++++------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index 3bbea2411f4..1890c99f04e 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -3,9 +3,9 @@ const { deepStrictEqual, strictEqual } = require('node:assert') const MemoryCacheStore = require('../../lib/cache/memory-cache-store') /** - * @param {typeof import('../../types/cache-interceptor').default.CacheStore} CacheStore + * @param {typeof import('../../types/cache-interceptor').default.CacheStore} CacheStore */ -function cacheStoreTests(CacheStore) { +function cacheStoreTests (CacheStore) { describe(CacheStore.prototype.constructor.name, () => { test('basic functionality', async () => { // Checks that it can store & fetch different responses diff --git a/test/types/cache-interceptor.test-d.ts b/test/types/cache-interceptor.test-d.ts index 2692644a5cd..af1bfed06bf 100644 --- a/test/types/cache-interceptor.test-d.ts +++ b/test/types/cache-interceptor.test-d.ts @@ -3,15 +3,15 @@ import CacheInterceptor from '../../types/cache-interceptor' import Dispatcher from '../../types/dispatcher' const store: CacheInterceptor.CacheStore = { - maxEntrySize: 0, + maxEntrySize: 0, - get(_: Dispatcher.RequestOptions): CacheInterceptor.CacheStoreValue | Promise { - throw new Error('stub') - }, + get (_: Dispatcher.RequestOptions): CacheInterceptor.CacheStoreValue | Promise { + throw new Error('stub') + }, - put(_: Dispatcher.RequestOptions, _2: CacheInterceptor.CacheStoreValue): void | Promise { - throw new Error('stub') - } + put (_: Dispatcher.RequestOptions, _2: CacheInterceptor.CacheStoreValue): void | Promise { + throw new Error('stub') + } } expectAssignable({}) @@ -28,22 +28,22 @@ expectAssignable({ size: 0, cachedAt: 0, staleAt: 0, - deleteAt: 0, + deleteAt: 0 }) expectAssignable({ - complete: true, - statusCode: 200, - statusMessage: 'OK', - rawHeaders: [], - rawTrailers: [], - body: [], - vary: {}, - size: 0, - cachedAt: 0, - staleAt: 0, - deleteAt: 0, - }) + complete: true, + statusCode: 200, + statusMessage: 'OK', + rawHeaders: [], + rawTrailers: [], + body: [], + vary: {}, + size: 0, + cachedAt: 0, + staleAt: 0, + deleteAt: 0 +}) expectNotAssignable({}) expectNotAssignable({ @@ -57,7 +57,7 @@ expectNotAssignable({ size: '', cachedAt: '', staleAt: '', - deleteAt: '', + deleteAt: '' }) expectAssignable({}) From ae6edca46f05b33ac099d1eb76ac4cfb30fcd614 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Sun, 15 Sep 2024 14:12:57 -0700 Subject: [PATCH 05/58] Update lib/handler/cache-handler.js Co-authored-by: Robert Nagy --- lib/handler/cache-handler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index f8fb438e305..044ce73cab9 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -71,7 +71,7 @@ class CacheHandler extends DecoratorHandler { const currentSize = this.#getSizeOfBuffers(rawHeaders) + (statusMessage?.length ?? 0) + 64 if ( - isNaN(contentLength) || + !Number.isInteger(contentLength) || contentLength > maxEntrySize || currentSize > maxEntrySize ) { From 26b2227bfc63608e44061a194dee98242738e5df Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 16 Sep 2024 11:21:23 -0700 Subject: [PATCH 06/58] Apply suggestions from code review Co-authored-by: Carlos Fuentes --- lib/handler/cache-handler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 044ce73cab9..98002af8cb3 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -168,7 +168,7 @@ class CacheHandler extends DecoratorHandler { const result = this.#opts.store.put(this.#req, this.#value) if (result && result.constructor.name === 'Promise') { result.catch(err => { - throw err + handler.onError(err) }) } } From 81cb0212fbbb7b548a4d71b52d28d41a166a3985 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 16 Sep 2024 12:00:01 -0700 Subject: [PATCH 07/58] fixup! fixup! fixup! fixup! feat: http caching Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 18 +++++++++++++++++- lib/handler/cache-handler.js | 3 ++- lib/interceptor/cache.js | 2 +- types/cache-interceptor.d.ts | 21 ++++++++++++++++++++- 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index c43ae358f3c..1fa0b3d4320 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -9,6 +9,7 @@ class MemoryCacheStore { * @type {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts} opts */ #opts = {} + #entryCount = 0 /** * @type {Map} */ @@ -20,11 +21,23 @@ class MemoryCacheStore { constructor (opts) { this.#opts = opts ?? {} + if (!this.#opts.maxEntries) { + this.#opts.maxEntries = Infinity + } + if (!this.#opts.maxEntrySize) { this.#opts.maxEntrySize = Infinity } } + get entryCount() { + return this.#entryCount + } + + get maxEntries() { + return this.#opts.maxEntries + } + get maxEntrySize () { return this.#opts.maxEntrySize } @@ -46,8 +59,9 @@ class MemoryCacheStore { for (let i = values.length - 1; i >= 0; i--) { const current = values[i] if (now >= current.deleteAt) { - // Delete the expired ones + // Should be deleted, so let's remove it values.splice(i, 1) + this.#entryCount-- continue } @@ -84,6 +98,8 @@ class MemoryCacheStore { this.#data.set(key, values) } + this.#entryCount++ + values.push(value) } diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 98002af8cb3..fc10d53669e 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -73,7 +73,8 @@ class CacheHandler extends DecoratorHandler { if ( !Number.isInteger(contentLength) || contentLength > maxEntrySize || - currentSize > maxEntrySize + currentSize > maxEntrySize || + this.#opts.store.entryCount >= this.#opts.store.maxEntries ) { return this.#handler.onHeaders( statusCode, diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 7967f2aa981..74f49b0c2ae 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -141,7 +141,7 @@ module.exports = globalOpts => { handler, value ) - }) + }).catch(err => handler.onError(err)) } else { handleCachedResult( globalOpts, diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 14d70bad004..a140d3bc53d 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -16,9 +16,21 @@ declare namespace CacheHandler { * Underlying storage provider for cached responses */ export interface CacheStore { + /** + * The amount of responses that are being cached + */ + get entryCount(): number + + /** + * The max amount of entries this cache can hold. If the size is greater + * than or equal to this, new responses will not be cached. + * @default Infinity + */ + get maxEntries(): number + /** * The max size of each value. If the content-length header is greater than - * this or the response ends up over this, the response will not be cached + * this or the response ends up over this, new responses will not be cached * @default Infinity */ get maxEntrySize(): number @@ -63,6 +75,10 @@ declare namespace CacheHandler { } export interface MemoryCacheStoreOpts { + /** + * @default Infinity + */ + maxEntries?: number /** * @default Infinity */ @@ -72,7 +88,10 @@ declare namespace CacheHandler { export class MemoryCacheStore implements CacheStore { constructor (opts?: MemoryCacheStoreOpts) + get entryCount(): number + get maxEntries(): number get maxEntrySize (): number + get (key: Dispatcher.RequestOptions): CacheStoreValue | Promise put (key: Dispatcher.RequestOptions, opts: CacheStoreValue): Promise } From 6bff3767972c07ecb1c37f94d03547cf781cb917 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 16 Sep 2024 16:04:30 -0700 Subject: [PATCH 08/58] fixup! fixup! fixup! fixup! fixup! feat: http caching Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 44 ++++++++---- lib/handler/cache-handler.js | 7 +- lib/interceptor/cache.js | 23 +++++-- test/cache-interceptor/interceptor.js | 97 +++++++++++++++++++++++++-- types/cache-interceptor.d.ts | 4 +- 5 files changed, 142 insertions(+), 33 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 1fa0b3d4320..db93e1db6ed 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -30,11 +30,11 @@ class MemoryCacheStore { } } - get entryCount() { + get entryCount () { return this.#entryCount } - get maxEntries() { + get maxEntries () { return this.#opts.maxEntries } @@ -44,10 +44,13 @@ class MemoryCacheStore { /** * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req - * @returns {Promise} + * @returns {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue | undefined} */ get (req) { const key = this.#makeKey(req) + if (!key) { + return undefined + } const values = this.#data.get(key) if (!values) { @@ -90,25 +93,38 @@ class MemoryCacheStore { * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue} value */ put (req, value) { - const key = this.#makeKey(req) - - let values = this.#data.get(key) - if (!values) { - values = [] - this.#data.set(key, values) - } + const existingValue = this.get(req) + if (existingValue) { + // We already cached something with the same key & vary headers, override it + Object.assign(existingValue, value) + } else { + // New response to cache + const key = this.#makeKey(req) + if (!key) { + return + } - this.#entryCount++ + let values = this.#data.get(key) + if (!values) { + values = [] + this.#data.set(key, values) + } - values.push(value) + this.#entryCount++ + values.push(value) + } } /** * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req - * @returns {string} + * @returns {string | undefined} */ #makeKey (req) { - // TODO origin is undefined + // Can't cache if we don't have the origin + if (!req.origin) { + return undefined + } + // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 return `${req.origin}:${req.path}:${req.method}` } diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index fc10d53669e..c5c84d5e500 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -4,6 +4,9 @@ const util = require('../core/util') const DecoratorHandler = require('../handler/decorator-handler') const { parseCacheControlHeader, parseVaryHeader } = require('../util/cache') +/** + * Writes a response to a CacheStore and then passes it on to the next handler + */ class CacheHandler extends DecoratorHandler { /** * @type {import('../../types/cache-interceptor.d.ts').default.CacheOptions} @@ -168,9 +171,7 @@ class CacheHandler extends DecoratorHandler { if (this.#getMaxEntrySize() > this.#value.size) { const result = this.#opts.store.put(this.#req, this.#value) if (result && result.constructor.name === 'Promise') { - result.catch(err => { - handler.onError(err) - }) + result.catch(err => this.#handler.onError(err)) } } } diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 74f49b0c2ae..61fff718293 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -75,9 +75,13 @@ function handleCachedResult ( return } - // Need to revalidate the request - opts.headers.push(`if-modified-since: ${new Date(value.cachedAt).toUTCString()}`) + if (!opts.headers) { + opts.headers = {} + } + + opts.headers['if-modified-since'] = new Date(value.cachedAt).toUTCString() + // Need to revalidate the response dispatch( opts, new CacheRevalidationHandler( @@ -103,18 +107,23 @@ module.exports = globalOpts => { } if (globalOpts.store) { - for (const fn of ['get', 'put', 'delete']) { + for (const fn of ['get', 'put']) { if (typeof globalOpts.store[fn] !== 'function') { throw new Error(`CacheStore needs a \`${fn}()\` function`) } } - if (!globalOpts.store.maxEntrySize) { - throw new Error('CacheStore needs a maxEntrySize getter') + for (const getter of ['entryCount', 'maxEntries', 'maxEntrySize']) { + const actualType = typeof globalOpts.store[getter] + if (actualType !== 'number') { + throw new Error(`CacheStore needs a ${getter} property with type number, current type: ${actualType}`) + } } - if (globalOpts.store.maxEntrySize <= 0) { - throw new Error('CacheStore maxEntrySize needs to be >= 1') + for (const value of ['maxEntries', 'maxEntry']) { + if (globalOpts.store[value] <= 0) { + throw new Error(`CacheStore ${value} needs to be >= 1`) + } } } else { globalOpts.store = new MemoryCacheStore() diff --git a/test/cache-interceptor/interceptor.js b/test/cache-interceptor/interceptor.js index 0128ec7b0a0..23d69f65b13 100644 --- a/test/cache-interceptor/interceptor.js +++ b/test/cache-interceptor/interceptor.js @@ -1,12 +1,11 @@ 'use strict' const { describe, test, after } = require('node:test') -const { strictEqual } = require('node:assert') +const { strictEqual, notEqual } = require('node:assert') const { createServer } = require('node:http') -const { Client, interceptors } = require('../../index') const { once } = require('node:events') +const { Client, interceptors } = require('../../index') -// e2e tests, checks just the public api stuff basically describe('Cache Interceptor', () => { test('doesn\'t cache request w/ no cache-control header', async () => { let requestsToOrigin = 0 @@ -25,12 +24,20 @@ describe('Cache Interceptor', () => { strictEqual(requestsToOrigin, 0) // Send initial request. This should reach the origin - let response = await client.request({ method: 'GET', path: '/' }) + let response = await client.request({ + origin: 'localhost', + method: 'GET', + path: '/' + }) strictEqual(requestsToOrigin, 1) strictEqual(await response.body.text(), 'asd') // Send second request that should be handled by cache - response = await client.request({ method: 'GET', path: '/' }) + response = await client.request({ + origin: 'localhost', + method: 'GET', + path: '/' + }) strictEqual(requestsToOrigin, 2) strictEqual(await response.body.text(), 'asd') }) @@ -53,12 +60,20 @@ describe('Cache Interceptor', () => { strictEqual(requestsToOrigin, 0) // Send initial request. This should reach the origin - let response = await client.request({ method: 'GET', path: '/' }) + let response = await client.request({ + origin: 'localhost', + method: 'GET', + path: '/' + }) strictEqual(requestsToOrigin, 1) strictEqual(await response.body.text(), 'asd') // Send second request that should be handled by cache - response = await client.request({ method: 'GET', path: '/' }) + response = await client.request({ + origin: 'localhost', + method: 'GET', + path: '/' + }) strictEqual(requestsToOrigin, 1) strictEqual(await response.body.text(), 'asd') strictEqual(response.headers.age, '0') @@ -89,6 +104,7 @@ describe('Cache Interceptor', () => { // Send initial request. This should reach the origin let response = await client.request({ + origin: 'localhost', method: 'GET', path: '/', headers: { @@ -113,6 +129,7 @@ describe('Cache Interceptor', () => { // Resend the first request again which should still be cahced response = await client.request({ + origin: 'localhost', method: 'GET', path: '/', headers: { @@ -123,4 +140,70 @@ describe('Cache Interceptor', () => { strictEqual(requestsToOrigin, 2) strictEqual(await response.body.text(), 'asd') }) + + test('revalidates request when needed', async () => { + let requestsToOrigin = 0 + + const server = createServer((req, res) => { + res.setHeader('cache-control', 'public, s-maxage=1, stale-while-revalidate=10') + + requestsToOrigin++ + + if (requestsToOrigin > 1) { + notEqual(req.headers['if-modified-since'], undefined) + + if (requestsToOrigin === 3) { + res.end('asd123') + } else { + res.statusCode = 304 + res.end() + } + } else { + res.end('asd') + } + }).listen(0) + + after(() => server.close()) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + strictEqual(requestsToOrigin, 0) + + const request = { + origin: 'localhost', + method: 'GET', + path: '/' + } + + // Send initial request. This should reach the origin + let response = await client.request(request) + strictEqual(requestsToOrigin, 1) + strictEqual(await response.body.text(), 'asd') + + // Now we send two more requests. Both of these should reach the origin, + // but now with a conditional header asking if the resource has been + // updated. These need to be ran after the response is stale. + const completed = new Promise((resolve, reject) => { + setTimeout(async () => { + try { + // No update for the second request + response = await client.request(request) + strictEqual(requestsToOrigin, 2) + strictEqual(await response.body.text(), 'asd') + + // This should be updated, even though the value isn't expired. + response = await client.request(request) + strictEqual(requestsToOrigin, 3) + strictEqual(await response.body.text(), 'asd123') + + resolve() + } catch (e) { + reject(e) + } + }, 1500) + }) + await completed + }) }) diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index a140d3bc53d..179b29390fb 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -88,8 +88,8 @@ declare namespace CacheHandler { export class MemoryCacheStore implements CacheStore { constructor (opts?: MemoryCacheStoreOpts) - get entryCount(): number - get maxEntries(): number + get entryCount (): number + get maxEntries (): number get maxEntrySize (): number get (key: Dispatcher.RequestOptions): CacheStoreValue | Promise From 2edee2940bd27128bd5fb4bd9efba7c9790531f6 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 16 Sep 2024 17:41:22 -0700 Subject: [PATCH 09/58] clarify type for MemoryCacheStore Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- types/cache-interceptor.d.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 179b29390fb..ec3687c75d9 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -92,7 +92,7 @@ declare namespace CacheHandler { get maxEntries (): number get maxEntrySize (): number - get (key: Dispatcher.RequestOptions): CacheStoreValue | Promise - put (key: Dispatcher.RequestOptions, opts: CacheStoreValue): Promise + get (key: Dispatcher.RequestOptions): CacheStoreValue + put (key: Dispatcher.RequestOptions, opts: CacheStoreValue): void } } From f128e9a4422585c4e00449ece49d5e93c14fecef Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 16 Sep 2024 18:19:51 -0700 Subject: [PATCH 10/58] Apply suggestions from code review Co-authored-by: Aras Abbasi --- lib/cache/memory-cache-store.js | 49 +++++++++++++++++++++++---------- lib/handler/cache-handler.js | 11 +++++--- lib/interceptor/cache.js | 2 +- types/cache-interceptor.d.ts | 2 +- 4 files changed, 43 insertions(+), 21 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index db93e1db6ed..e96733b7600 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -1,14 +1,16 @@ 'use strict' +const fastTimers = require('../util/timers.js') + /** * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore * @implements {CacheStore} */ class MemoryCacheStore { - /** - * @type {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts} opts - */ - #opts = {} + #maxEntries = Infinity + + #maxEntrySize = Infinity + #entryCount = 0 /** * @type {Map} @@ -19,14 +21,32 @@ class MemoryCacheStore { * @param {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts | undefined} opts */ constructor (opts) { - this.#opts = opts ?? {} + if (opts) { + if (typeof opts !== 'object') { + throw new TypeError('MemoryCacheStore options must be an object') + } - if (!this.#opts.maxEntries) { - this.#opts.maxEntries = Infinity - } + if (opts.maxEntries !== undefined) { + if ( + typeof opts.maxEntries !== 'number' || + !Number.isInteger(opts.maxEntries) || + opts.maxEntries < 0 + ) { + throw new TypeError('MemoryCacheStore options.maxEntries must be a non-negative integer') + } + this.#maxEntries = opts.maxEntries + } - if (!this.#opts.maxEntrySize) { - this.#opts.maxEntrySize = Infinity + if (opts.maxEntrySize !== undefined) { + if ( + typeof opts.maxEntrySize !== 'number' || + !Number.isInteger(opts.maxEntrySize) || + opts.maxEntrySize < 0 + ) { + throw new TypeError('MemoryCacheStore options.maxEntrySize must be a non-negative integer') + } + this.#maxEntrySize = opts.maxEntrySize + } } } @@ -35,12 +55,11 @@ class MemoryCacheStore { } get maxEntries () { - return this.#opts.maxEntries + return this.#maxEntries } get maxEntrySize () { - return this.#opts.maxEntrySize - } + return this.#maxEntrySize /** * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req @@ -58,7 +77,7 @@ class MemoryCacheStore { } let value - const now = Date.now() + const now = fastTimers.now() for (let i = values.length - 1; i >= 0; i--) { const current = values[i] if (now >= current.deleteAt) { @@ -100,7 +119,7 @@ class MemoryCacheStore { } else { // New response to cache const key = this.#makeKey(req) - if (!key) { + if (key === undefined) { return } diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index c5c84d5e500..d12a51b9fe1 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -45,7 +45,7 @@ class CacheHandler extends DecoratorHandler { * @param {Buffer[]} rawHeaders * @param {() => void} resume * @param {string} statusMessage - * @param {string[] | undefined} headers + * @param {Record | undefined} headers * @returns {boolean} */ onHeaders ( @@ -219,7 +219,10 @@ class CacheHandler extends DecoratorHandler { * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives */ function canCacheResponse (statusCode, headers, cacheControlDirectives) { - if (![200, 307].includes(statusCode)) { + if ( + statusCode !== 200 && + statusCode !== 307 + ) { return false } @@ -270,7 +273,7 @@ function canCacheResponse (statusCode, headers, cacheControlDirectives) { } /** - * @param {Record} headers + * @param {Record} headers * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives * * @returns {number | undefined} time that the value is stale at or undefined if it shouldn't be cached @@ -296,7 +299,7 @@ function determineStaleAt (headers, cacheControlDirectives) { if (headers.expire) { // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.3 - return new Date() - new Date(headers.expire) + return Date.now() - new Date(headers.expire).getTime() } return undefined diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 61fff718293..a6a83d8df83 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -150,7 +150,7 @@ module.exports = globalOpts => { handler, value ) - }).catch(err => handler.onError(err)) + }).catch(handler.onError) } else { handleCachedResult( globalOpts, diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index ec3687c75d9..008052fc564 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -35,7 +35,7 @@ declare namespace CacheHandler { */ get maxEntrySize(): number - get(key: Dispatcher.RequestOptions): CacheStoreValue | Promise; + get(key: Dispatcher.RequestOptions): CacheStoreValue | Promise | undefined; put(key: Dispatcher.RequestOptions, opts: CacheStoreValue): void | Promise; } From 807e76405029f94054eb2a8b41c392baaa39c011 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 16 Sep 2024 18:56:47 -0700 Subject: [PATCH 11/58] Apply suggestions from code review Co-authored-by: Aras Abbasi --- lib/util/cache.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/util/cache.js b/lib/util/cache.js index f6b7d50d09f..798cbf5721e 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -10,7 +10,7 @@ * 'stale-while-revalidate'?: number; * 'stale-if-error'?: number; * public?: true; - * private?: true; + * private?: true | string[]; * 'no-store'?: true; * 'no-cache'?: true | string[]; * 'must-revalidate'?: true; @@ -40,9 +40,8 @@ function parseCacheControlHeader (header) { if (keyValueDelimiter !== -1) { key = directive.substring(0, keyValueDelimiter).trim() value = directive - .substring(keyValueDelimiter + 1, directive.length) + .substring(keyValueDelimiter + 1) .trim() - .toLowerCase() } else { key = directive.trim() } @@ -55,7 +54,7 @@ function parseCacheControlHeader (header) { case 'stale-while-revalidate': case 'stale-if-error': { const parsedValue = parseInt(value, 10) - if (isNaN(parsedValue)) { + if (parsedValue !== parsedValue) { continue } @@ -86,7 +85,8 @@ function parseCacheControlHeader (header) { let foundEndingQuote = false for (let j = i; j < directives.length; j++) { const nextPart = directives[j] - if (nextPart.endsWith('"')) { + const nextPartLength = nextPart.length + if (nextPartLength !== 0 && nextPart[nextPartLength] === '"') { foundEndingQuote = true headers.push(...directives.splice(i + 1, j - 1).map(header => header.trim())) @@ -96,7 +96,7 @@ function parseCacheControlHeader (header) { } } - if (!foundEndingQuote) { + if (foundEndingQuote === false) { // Something like `no-cache="some-header` with no end quote, // let's just ignore it continue @@ -141,7 +141,7 @@ function parseCacheControlHeader (header) { * @returns {Record} */ function parseVaryHeader (varyHeader, headers) { - const output = {} + const output = /** @type {Record} */ ({}) const varyingHeaders = varyHeader.toLowerCase().split(',') for (const header of varyingHeaders) { From 4f8139a2143a67df365ded84f7b64fc43fdd6657 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 16 Sep 2024 18:52:00 -0700 Subject: [PATCH 12/58] tmp Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 3 ++- lib/handler/cache-handler.js | 45 ++++++++++++++++++++++++--------- lib/interceptor/cache.js | 10 +++++--- types/cache-interceptor.d.ts | 6 ++--- 4 files changed, 45 insertions(+), 19 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index e96733b7600..107f3dc9be6 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -60,6 +60,7 @@ class MemoryCacheStore { get maxEntrySize () { return this.#maxEntrySize + } /** * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req @@ -104,7 +105,7 @@ class MemoryCacheStore { } } - return value + return { ...value } } /** diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index d12a51b9fe1..6c369dd6b2a 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -127,13 +127,15 @@ class CacheHandler extends DecoratorHandler { } } - return this.#handler.onHeaders( - statusCode, - rawHeaders, - resume, - statusMessage, - headers - ) + if (typeof this.#handler.onHeaders === 'function') { + return this.#handler.onHeaders( + statusCode, + rawHeaders, + resume, + statusMessage, + headers + ) + } } /** @@ -153,7 +155,9 @@ class CacheHandler extends DecoratorHandler { } } - return this.#handler.onData(chunk) + if (typeof this.#handler.onData === 'function') { + return this.#handler.onData(chunk) + } } /** @@ -176,7 +180,9 @@ class CacheHandler extends DecoratorHandler { } } - return this.#handler.onComplete(rawTrailers) + if (typeof this.#handler.onComplete === 'function') { + return this.#handler.onComplete(rawTrailers) + } } /** @@ -186,7 +192,9 @@ class CacheHandler extends DecoratorHandler { */ onError (err) { this.#value = undefined - this.#handler.onError(err) + if (typeof this.#handler.onError === 'function') { + this.#handler.onError(err) + } } /** @@ -203,8 +211,16 @@ class CacheHandler extends DecoratorHandler { #getSizeOfBuffers (arr) { let size = 0 - for (const buffer of arr) { - size += buffer.length + if (arr.length > 0) { + if (typeof arr[0] === 'string') { + for (const buffer of arr) { + size += buffer.length + } + } else { + for (const buffer of arr) { + size += buffer.byteLength + } + } } return size @@ -339,6 +355,11 @@ function stripNecessaryHeaders (rawHeaders, parsedHeaders, cacheControlDirective for (let i = 0; i < parsedHeaders.length; i++) { const header = parsedHeaders[i] const kvDelimiterIndex = header.indexOf(':') + if (kvDelimiterIndex === -1) { + // We should never get here but just for safety + throw new Error('header missing kv delimiter') + } + const headerName = header.substring(0, kvDelimiterIndex) if (headerName in headersToRemove) { diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index a6a83d8df83..7944b57b332 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -4,6 +4,8 @@ const CacheHandler = require('../handler/cache-handler') const MemoryCacheStore = require('../cache/memory-cache-store') const CacheRevalidationHandler = require('../handler/cache-revalidation-handler') +const AGE_HEADER = Buffer.from('age') + /** * Gives the downstream handler the request's cached response or dispatches * it if it isn't cached @@ -27,7 +29,9 @@ function handleCachedResult ( } // Dump body on error - opts.body?.on('error', () => {}).resume() + if (typeof opts.body === 'object' && opts.body.constructor.name === 'Readable') { + opts.body?.on('error', () => {}).resume() + } const respondWithCachedValue = () => { const ac = new AbortController() @@ -44,7 +48,7 @@ function handleCachedResult ( // Copy the headers in case we got this from an in-memory store. We don't // want to modify the original response. const headers = [...value.rawHeaders] - headers.push(Buffer.from('age'), Buffer.from(`${age}`)) + headers.push(AGE_HEADER, Buffer.from(`${age}`)) handler.onHeaders(value.statusCode, headers, () => {}, value.statusMessage) signal.throwIfAborted() @@ -141,7 +145,7 @@ module.exports = globalOpts => { } const result = globalOpts.store.get(opts) - if (result && result.constructor.name === 'Promise') { + if (result && typeof result.then === 'function') { result.then(value => { handleCachedResult( globalOpts, diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 008052fc564..7aae138e35f 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -9,7 +9,7 @@ declare namespace CacheHandler { /** * The methods to cache, defaults to just GET */ - methods?: ('GET' | 'HEAD' | 'POST' | 'PATCH')[] + methods?: ('GET' | 'HEAD' | 'POST' | 'PATCH' | 'PUT' | 'DELETE')[] } /** @@ -35,7 +35,7 @@ declare namespace CacheHandler { */ get maxEntrySize(): number - get(key: Dispatcher.RequestOptions): CacheStoreValue | Promise | undefined; + get(key: Dispatcher.RequestOptions): CacheStoreValue | Promise | undefined; put(key: Dispatcher.RequestOptions, opts: CacheStoreValue): void | Promise; } @@ -92,7 +92,7 @@ declare namespace CacheHandler { get maxEntries (): number get maxEntrySize (): number - get (key: Dispatcher.RequestOptions): CacheStoreValue + get (key: Dispatcher.RequestOptions): CacheStoreValue | undefined put (key: Dispatcher.RequestOptions, opts: CacheStoreValue): void } } From fabf5589c56faf9c3185eef9c9b55d36d29a33cc Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 16 Sep 2024 21:04:10 -0700 Subject: [PATCH 13/58] fixup! tmp Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 4 +-- lib/interceptor/cache.js | 48 ++++++++++++++++++++------------- lib/util/cache.js | 5 ++++ 3 files changed, 35 insertions(+), 22 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 107f3dc9be6..985f1b17dae 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -1,7 +1,5 @@ 'use strict' -const fastTimers = require('../util/timers.js') - /** * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore * @implements {CacheStore} @@ -78,7 +76,7 @@ class MemoryCacheStore { } let value - const now = fastTimers.now() + const now = Date.now() for (let i = values.length - 1; i >= 0; i--) { const current = values[i] if (now >= current.deleteAt) { diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 7944b57b332..60527a7ee3d 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -38,34 +38,44 @@ function handleCachedResult ( const signal = ac.signal try { - handler.onConnect(ac.abort) - signal.throwIfAborted() + if (typeof handler.onConnect === 'function') { + handler.onConnect(ac.abort) + signal.throwIfAborted() + } - // Add the age header - // https://www.rfc-editor.org/rfc/rfc9111.html#name-age - const age = Math.round((Date.now() - value.cachedAt) / 1000) + if (typeof handler.onHeaders === 'function') { + // Add the age header + // https://www.rfc-editor.org/rfc/rfc9111.html#name-age + const age = Math.round((Date.now() - value.cachedAt) / 1000) - // Copy the headers in case we got this from an in-memory store. We don't - // want to modify the original response. - const headers = [...value.rawHeaders] - headers.push(AGE_HEADER, Buffer.from(`${age}`)) + value.rawHeaders.push(AGE_HEADER, Buffer.from(`${age}`)) - handler.onHeaders(value.statusCode, headers, () => {}, value.statusMessage) - signal.throwIfAborted() + handler.onHeaders(value.statusCode, value.rawHeaders, () => {}, value.statusMessage) + signal.throwIfAborted() + } - if (opts.method === 'HEAD') { - handler.onComplete([]) - } else { - for (const chunk of value.body) { - while (!handler.onData(chunk)) { - signal.throwIfAborted() + let trailers = null + if (opts.method !== 'HEAD') { + if (typeof handler.onData === 'function') { + for (const chunk of value.body) { + while (!handler.onData(chunk)) { + signal.throwIfAborted() + } } } - handler.onComplete(value.rawTrailers ?? null) + if (value.rawTrailers !== undefined) { + trailers = value.rawTrailers + } + } + + if (typeof handler.onComplete === 'function') { + handler.onComplete(trailers) } } catch (err) { - handler.onError(err) + if (typeof handler.onError === 'function') { + handler.onError(err) + } } } diff --git a/lib/util/cache.js b/lib/util/cache.js index 798cbf5721e..7806350fb47 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -53,7 +53,12 @@ function parseCacheControlHeader (header) { case 's-maxage': case 'stale-while-revalidate': case 'stale-if-error': { + if (value === undefined) { + continue + } + const parsedValue = parseInt(value, 10) + // eslint-disable-next-line no-self-compare if (parsedValue !== parsedValue) { continue } From 4546799d2140c6e8fb6f43791ddafd6ca147e950 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 18 Sep 2024 21:52:20 -0700 Subject: [PATCH 14/58] Apply suggestions from code review Co-authored-by: Aras Abbasi --- lib/handler/cache-revalidation-handler.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index 5ea70c20070..24900fa1cf9 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -19,11 +19,11 @@ const DecoratorHandler = require('../handler/decorator-handler') class CacheRevalidationHandler extends DecoratorHandler { #successful = false /** - * @type {() => void} + * @type {(() => void)|null} */ #successCallback = null /** - * @type {import('../../types/dispatcher.d.ts').default.DispatchHandlers} + * @type {(import('../../types/dispatcher.d.ts').default.DispatchHandlers)|null} */ #handler = null From 5a215d216f5b992af14cba05484b1a4203ba8be6 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 18 Sep 2024 21:52:30 -0700 Subject: [PATCH 15/58] perf things, deleteByOrigin Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 103 +++++++++++++++------- lib/handler/cache-handler.js | 50 +++++++---- lib/handler/cache-revalidation-handler.js | 34 ++++--- lib/interceptor/cache.js | 24 +++-- lib/util/cache.js | 12 ++- test/types/cache-interceptor.test-d.ts | 6 ++ types/cache-interceptor.d.ts | 27 +++++- 7 files changed, 184 insertions(+), 72 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 985f1b17dae..57590355570 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -11,7 +11,7 @@ class MemoryCacheStore { #entryCount = 0 /** - * @type {Map} + * @type {Map>} */ #data = new Map() @@ -65,12 +65,7 @@ class MemoryCacheStore { * @returns {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue | undefined} */ get (req) { - const key = this.#makeKey(req) - if (!key) { - return undefined - } - - const values = this.#data.get(key) + const values = this.#getValues(req) if (!values) { return } @@ -80,15 +75,20 @@ class MemoryCacheStore { for (let i = values.length - 1; i >= 0; i--) { const current = values[i] if (now >= current.deleteAt) { - // Should be deleted, so let's remove it - values.splice(i, 1) - this.#entryCount-- - continue + // We've reached expired values, let's delete them + this.#entryCount -= values.length - i + values.length = i + break } let matches = true if (current.vary) { + if (!req.headers) { + matches = false + break + } + for (const key in current.vary) { if (current.vary[key] !== req.headers[key]) { matches = false @@ -103,7 +103,7 @@ class MemoryCacheStore { } } - return { ...value } + return value ? { ...value } : undefined } /** @@ -113,38 +113,79 @@ class MemoryCacheStore { put (req, value) { const existingValue = this.get(req) if (existingValue) { - // We already cached something with the same key & vary headers, override it + // We already cached something with the same key & vary headers, update it Object.assign(existingValue, value) - } else { - // New response to cache - const key = this.#makeKey(req) - if (key === undefined) { - return - } + return + } - let values = this.#data.get(key) - if (!values) { - values = [] - this.#data.set(key, values) - } + // New response to cache + const values = this.#getValues(req) + + this.#entryCount++ + + if (!values) { + // We've never cached anything at this origin before + const pathValues = new Map() + pathValues.set(`${req.path}:${req.method}`, [value]) - this.#entryCount++ + this.#data.set(req.origin, pathValues) + return + } + + if ( + values.length === 0 || + value.deleteAt < values[values.length - 1].deleteAt + ) { values.push(value) } + + if (value.deleteAt >= values[0].deleteAt) { + values.unshift(value) + return + } + + let startIndex = 0 + let endIndex = values.length + while (true) { + if (startIndex === endIndex) { + values.splice(startIndex, 0, value) + break + } + + const middleIndex = (startIndex + endIndex) / 2 + const middleValue = values[middleIndex] + if (value.deleteAt === middleIndex) { + values.splice(middleIndex, 0, value) + break + } else if (value.deleteAt > middleValue.deleteAt) { + endIndex = middleIndex + continue + } else { + startIndex = middleIndex + continue + } + } + } + + /** + * @param {string} origin + */ + deleteByOrigin (origin) { + this.#data.delete(origin) } /** * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req - * @returns {string | undefined} + * @returns {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue[] | undefined} */ - #makeKey (req) { - // Can't cache if we don't have the origin - if (!req.origin) { + #getValues (req) { + // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 + const cachedPaths = this.#data.get(req.origin) + if (!cachedPaths) { return undefined } - // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 - return `${req.origin}:${req.path}:${req.method}` + return cachedPaths.get(`${req.path}:${req.method}`) } } diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 6c369dd6b2a..d5225079918 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -2,28 +2,28 @@ const util = require('../core/util') const DecoratorHandler = require('../handler/decorator-handler') -const { parseCacheControlHeader, parseVaryHeader } = require('../util/cache') +const { parseCacheControlHeader, parseVaryHeader, UNSAFE_METHODS } = require('../util/cache') /** * Writes a response to a CacheStore and then passes it on to the next handler */ class CacheHandler extends DecoratorHandler { /** - * @type {import('../../types/cache-interceptor.d.ts').default.CacheOptions} + * @type {import('../../types/cache-interceptor.d.ts').default.CacheOptions} | null */ #opts = null /** - * @type {import('../../types/dispatcher.d.ts').default.RequestOptions} + * @type {import('../../types/dispatcher.d.ts').default.RequestOptions | null} */ #req = null /** - * @type {import('../../types/dispatcher.d.ts').default.DispatchHandlers} + * @type {import('../../types/dispatcher.d.ts').default.DispatchHandlers | null} */ #handler = null /** * @type {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue | undefined} */ - #value = null + #value = undefined /** * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions} opts @@ -55,6 +55,30 @@ class CacheHandler extends DecoratorHandler { statusMessage, headers = util.parseHeaders(rawHeaders) ) { + if ( + this.#req.method in UNSAFE_METHODS && + statusCode >= 200 && + statusCode <= 399 + ) { + // https://www.rfc-editor.org/rfc/rfc9111.html#name-invalidating-stored-respons + const result = this.#opts.store.deleteByOrigin(this.#req.origin) + if ( + result && + typeof result.catch === 'function' && + typeof this.#handler.onError === 'function' + ) { + result.catch(this.#handler.onError) + } + + return this.#handler.onHeaders( + statusCode, + rawHeaders, + resume, + statusMessage, + headers + ) + } + const cacheControlHeader = headers['cache-control'] const contentLengthHeader = headers['content-length'] @@ -243,22 +267,10 @@ function canCacheResponse (statusCode, headers, cacheControlDirectives) { } if ( - !cacheControlDirectives.public && - !cacheControlDirectives['s-maxage'] && - !cacheControlDirectives['must-revalidate'] - ) { - // Response can't be used in a shared cache - return false - } - - if ( - // TODO double check these + !cacheControlDirectives.public || cacheControlDirectives.private === true || cacheControlDirectives['no-cache'] === true || - cacheControlDirectives['no-store'] || - cacheControlDirectives['no-transform'] || - cacheControlDirectives['must-understand'] || - cacheControlDirectives['proxy-revalidate'] + cacheControlDirectives['no-store'] ) { return false } diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index 24900fa1cf9..f262a925543 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -34,6 +34,10 @@ class CacheRevalidationHandler extends DecoratorHandler { constructor (successCallback, handler) { super(handler) + if (typeof successCallback !== 'function') { + throw new TypeError('successCallback must be a function') + } + this.#successCallback = successCallback this.#handler = handler } @@ -62,13 +66,15 @@ class CacheRevalidationHandler extends DecoratorHandler { return true } - return this.#handler.onHeaders( - statusCode, - rawHeaders, - resume, - statusMessage, - headers - ) + if (typeof this.#handler.onHeaders === 'function') { + return this.#handler.onHeaders( + statusCode, + rawHeaders, + resume, + statusMessage, + headers + ) + } } /** @@ -78,7 +84,13 @@ class CacheRevalidationHandler extends DecoratorHandler { * @returns {boolean} */ onData (chunk) { - return this.#successful ? true : this.#handler.onData(chunk) + if (this.#successful) { + return true + } + + if (typeof this.#handler.onData === 'function') { + this.#handler.onData(chunk) + } } /** @@ -87,7 +99,7 @@ class CacheRevalidationHandler extends DecoratorHandler { * @param {string[] | null} rawTrailers */ onComplete (rawTrailers) { - if (!this.#successful) { + if (!this.#successful && typeof this.#handler.onComplete === 'function') { this.#handler.onComplete(rawTrailers) } } @@ -98,7 +110,9 @@ class CacheRevalidationHandler extends DecoratorHandler { * @param {Error} err */ onError (err) { - this.#handler.onError(err) + if (typeof this.#handler.onError === 'function') { + this.#handler.onError(err) + } } } diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 60527a7ee3d..1287d8151ff 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -3,6 +3,7 @@ const CacheHandler = require('../handler/cache-handler') const MemoryCacheStore = require('../cache/memory-cache-store') const CacheRevalidationHandler = require('../handler/cache-revalidation-handler') +const { UNSAFE_METHODS } = require('../util/cache.js') const AGE_HEADER = Buffer.from('age') @@ -123,34 +124,45 @@ module.exports = globalOpts => { if (globalOpts.store) { for (const fn of ['get', 'put']) { if (typeof globalOpts.store[fn] !== 'function') { - throw new Error(`CacheStore needs a \`${fn}()\` function`) + throw new TypeError(`CacheStore needs a \`${fn}()\` function`) } } for (const getter of ['entryCount', 'maxEntries', 'maxEntrySize']) { const actualType = typeof globalOpts.store[getter] if (actualType !== 'number') { - throw new Error(`CacheStore needs a ${getter} property with type number, current type: ${actualType}`) + throw new TypeError(`CacheStore needs a ${getter} property with type number, current type: ${actualType}`) } } for (const value of ['maxEntries', 'maxEntry']) { if (globalOpts.store[value] <= 0) { - throw new Error(`CacheStore ${value} needs to be >= 1`) + throw new TypeError(`CacheStore ${value} needs to be >= 1`) } } } else { globalOpts.store = new MemoryCacheStore() } - if (!globalOpts.methods) { + if (globalOpts.methods) { + if (!Array.isArray(globalOpts.methods)) { + throw new TypeError(`methods needs to be an array, got ${typeof globalOpts.methods}`) + } + + if (globalOpts.methods.length === 0) { + throw new Error('methods must have at least one method in it') + } + } else { globalOpts.methods = ['GET'] } + // Safe methods the user wants and unsafe methods + const methods = [...globalOpts.methods, ...UNSAFE_METHODS] + return dispatch => { return (opts, handler) => { - if (!globalOpts.methods.includes(opts.method)) { - // Not a method we want to cache, skip + if (!opts.origin || !methods.includes(opts.method)) { + // Not a method we want to cache or we don't have the origin, skip return dispatch(opts, handler) } diff --git a/lib/util/cache.js b/lib/util/cache.js index 7806350fb47..30bc592d787 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -1,3 +1,7 @@ +const UNSAFE_METHODS = [ + 'POST', 'PUT', 'PATCH', 'DELETE' +] + /** * @see https://www.rfc-editor.org/rfc/rfc9111.html#name-cache-control * @see https://www.iana.org/assignments/http-cache-directives/http-cache-directives.xhtml @@ -95,8 +99,11 @@ function parseCacheControlHeader (header) { foundEndingQuote = true headers.push(...directives.splice(i + 1, j - 1).map(header => header.trim())) - const lastHeader = header[headers.length - 1] - headers[headers.length - 1] = lastHeader.substring(0, lastHeader.length - 1) + // Cut off ending quote + let lastHeader = header[headers.length - 1] + lastHeader = lastHeader.substring(0, lastHeader.length - 1) + headers[headers.length - 1] = lastHeader + break } } @@ -161,6 +168,7 @@ function parseVaryHeader (varyHeader, headers) { } module.exports = { + UNSAFE_METHODS, parseCacheControlHeader, parseVaryHeader } diff --git a/test/types/cache-interceptor.test-d.ts b/test/types/cache-interceptor.test-d.ts index af1bfed06bf..c0c43f249bf 100644 --- a/test/types/cache-interceptor.test-d.ts +++ b/test/types/cache-interceptor.test-d.ts @@ -4,6 +4,8 @@ import Dispatcher from '../../types/dispatcher' const store: CacheInterceptor.CacheStore = { maxEntrySize: 0, + entryCount: 0, + maxEntries: 0, get (_: Dispatcher.RequestOptions): CacheInterceptor.CacheStoreValue | Promise { throw new Error('stub') @@ -11,6 +13,10 @@ const store: CacheInterceptor.CacheStore = { put (_: Dispatcher.RequestOptions, _2: CacheInterceptor.CacheStoreValue): void | Promise { throw new Error('stub') + }, + + deleteByOrigin (_: string): void | Promise { + throw new Error('stub') } } diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 7aae138e35f..d4206286327 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -7,9 +7,13 @@ declare namespace CacheHandler { store?: CacheStore /** - * The methods to cache, defaults to just GET + * The methods to cache + * Note we can only cache safe methods. Unsafe methods (i.e. PUT, POST) + * invalidate the cache for a origin. + * @see https://www.rfc-editor.org/rfc/rfc9111.html#name-invalidating-stored-respons + * @see https://www.rfc-editor.org/rfc/rfc9110#section-9.2.1 */ - methods?: ('GET' | 'HEAD' | 'POST' | 'PATCH' | 'PUT' | 'DELETE')[] + methods?: ('GET' | 'HEAD' | 'OPTIONS' | 'TRACE')[] } /** @@ -35,9 +39,23 @@ declare namespace CacheHandler { */ get maxEntrySize(): number + /** + * Get a request's cached response if it exists. + * Note: it is the cache store's responsibility to enforce the vary header checks + */ get(key: Dispatcher.RequestOptions): CacheStoreValue | Promise | undefined; - put(key: Dispatcher.RequestOptions, opts: CacheStoreValue): void | Promise; + /** + * Add a new request to the cache + * @param key + * @param opts + */ + put(key: Dispatcher.RequestOptions, value: CacheStoreValue): void | Promise; + + /** + * Delete all of the cached responses from a certain origin (host) + */ + deleteByOrigin(origin: string): void | Promise } export interface CacheStoreValue { @@ -49,7 +67,7 @@ declare namespace CacheHandler { statusMessage: string; rawHeaders: Buffer[]; rawTrailers?: Buffer[]; - body: string[] + body: Buffer[] /** * Headers defined by the Vary header and their respective values for * later comparison @@ -94,5 +112,6 @@ declare namespace CacheHandler { get (key: Dispatcher.RequestOptions): CacheStoreValue | undefined put (key: Dispatcher.RequestOptions, opts: CacheStoreValue): void + deleteByOrigin (origin: string): void } } From 73564e89a9034b9c74dd47ca1751dfd3da943065 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Thu, 19 Sep 2024 18:39:07 -0700 Subject: [PATCH 16/58] incredibly messy and broken impl of streaming idea Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 264 +++++++++++++++++++++++--------- lib/handler/cache-handler.js | 50 +++--- lib/interceptor/cache.js | 206 ++++++++++--------------- types/cache-interceptor.d.ts | 54 +++---- 4 files changed, 319 insertions(+), 255 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 57590355570..e41b243941f 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -1,8 +1,19 @@ 'use strict' +const EventEmitter = require('node:events') +const { Writable, Readable } = require('node:stream') + +// todo clean this up + /** * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore * @implements {CacheStore} + * + * @typedef {{ + * complete: boolean + * value: import('../../types/cache-interceptor.d.ts').default.CacheStoreValue + * emitter: EventEmitter + * }} MemoryStoreValue */ class MemoryCacheStore { #maxEntries = Infinity @@ -10,8 +21,9 @@ class MemoryCacheStore { #maxEntrySize = Infinity #entryCount = 0 + /** - * @type {Map>} + * @type {Map>} */ #data = new Map() @@ -48,33 +60,29 @@ class MemoryCacheStore { } } - get entryCount () { - return this.#entryCount + get isFull () { + return this.#entryCount < this.#maxEntries } - get maxEntries () { - return this.#maxEntries - } - - get maxEntrySize () { - return this.#maxEntrySize - } + createReadStream (req) { + if (typeof req !== 'object') { + throw new TypeError(`expected req to be object, got ${typeof req}`) + } - /** - * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req - * @returns {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue | undefined} - */ - get (req) { - const values = this.#getValues(req) + const values = this.#getValues(req, false) if (!values) { return } + /** + * @type {MemoryStoreValue} + */ let value const now = Date.now() for (let i = values.length - 1; i >= 0; i--) { const current = values[i] - if (now >= current.deleteAt) { + const currentCacheValue = current.value + if (now >= currentCacheValue.deleteAt) { // We've reached expired values, let's delete them this.#entryCount -= values.length - i values.length = i @@ -83,14 +91,14 @@ class MemoryCacheStore { let matches = true - if (current.vary) { + if (currentCacheValue.vary) { if (!req.headers) { matches = false break } - for (const key in current.vary) { - if (current.vary[key] !== req.headers[key]) { + for (const key in currentCacheValue.vary) { + if (currentCacheValue.vary[key] !== req.headers[key]) { matches = false break } @@ -103,68 +111,68 @@ class MemoryCacheStore { } } - return value ? { ...value } : undefined + return value ? new MemoryStoreReadableStream(value) : undefined } - /** - * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req - * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue} value - */ - put (req, value) { - const existingValue = this.get(req) - if (existingValue) { - // We already cached something with the same key & vary headers, update it - Object.assign(existingValue, value) - return + createWriteStream (req, value) { + if (typeof req !== 'object') { + throw new TypeError(`expected req to be object, got ${typeof req}`) + } + if (typeof value !== 'object') { + throw new TypeError(`expected value to be object, got ${typeof value}`) } - // New response to cache - const values = this.#getValues(req) + if (this.isFull) { + return undefined + } - this.#entryCount++ + // TODO see if value already exists or not - if (!values) { - // We've never cached anything at this origin before - const pathValues = new Map() - pathValues.set(`${req.path}:${req.method}`, [value]) + const values = this.#getValues(req, true) + this.entryCount++ - this.#data.set(req.origin, pathValues) - return + /** + * TODO what's a better name for this + * @type {MemoryStoreValue} + */ + const storedValue = { + complete: false, + value, + emitter: new EventEmitter() } if ( values.length === 0 || value.deleteAt < values[values.length - 1].deleteAt ) { - values.push(value) - } - - if (value.deleteAt >= values[0].deleteAt) { - values.unshift(value) - return - } - - let startIndex = 0 - let endIndex = values.length - while (true) { - if (startIndex === endIndex) { - values.splice(startIndex, 0, value) - break - } + values.push(storedValue) + } else if (value.deleteAt >= values[0].deleteAt) { + values.unshift(storedValue) + } else { + let startIndex = 0 + let endIndex = values.length + while (true) { + if (startIndex === endIndex) { + values.splice(startIndex, 0, storedValue) + break + } - const middleIndex = (startIndex + endIndex) / 2 - const middleValue = values[middleIndex] - if (value.deleteAt === middleIndex) { - values.splice(middleIndex, 0, value) - break - } else if (value.deleteAt > middleValue.deleteAt) { - endIndex = middleIndex - continue - } else { - startIndex = middleIndex - continue + const middleIndex = (startIndex + endIndex) / 2 + const middleValue = values[middleIndex] + if (value.deleteAt === middleIndex) { + values.splice(middleIndex, 0, storedValue) + break + } else if (value.deleteAt > middleValue.deleteAt) { + endIndex = middleIndex + continue + } else { + startIndex = middleIndex + continue + } } } + + return new MemoryStoreWritableStream(storedValue) } /** @@ -176,16 +184,130 @@ class MemoryCacheStore { /** * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req - * @returns {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue[] | undefined} + * @returns {MemoryStoreValue[] | undefined} */ - #getValues (req) { + #getValues (req, makeIfDoesntExist) { // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 - const cachedPaths = this.#data.get(req.origin) + let cachedPaths = this.#data.get(req.origin) if (!cachedPaths) { - return undefined + if (!makeIfDoesntExist) { + return undefined + } + + cachedPaths = new Map() + this.#data.set(req.origin, cachedPaths) + } + + let values = cachedPaths.get(`${req.path}:${req.method}`) + if (!values && makeIfDoesntExist) { + values = [] + cachedPaths.set(`${req.path}:${req.method}`, values) + } + + return values + } +} + +class MemoryStoreReadableStream extends Readable { + /** + * @type {MemoryStoreValue} + */ + #value + /** + * @type {Buffer[]} + */ + #chunksToSend + + /** + * @param {MemoryStoreValue} value + */ + constructor (value) { + super() + + this.#value = value + this.#chunksToSend = [...this.#value.value.body] + + if (!value.complete) { + value.emitter.on('write', this.#chunksToSend.push) + value.emitter.on('final', () => { + this.#chunksToSend.push(null) + }) + } + } + + get value () { + return this.#value + } + + /** + * @param {number} size + */ + _read (size) { + // TODO what to do if we get a read but we're still waiting on more chunks? + if (size > this.#chunksToSend.length) { + size = this.#chunksToSend.length + } + + for (let i = 0; i < size; i++) { + this.push(this.#chunksToSend.pop()) } + } +} + +// TODO enforce max entry size, ... +class MemoryStoreWritableStream extends Writable { + /** + * @type {MemoryStoreValue} + */ + #value - return cachedPaths.get(`${req.path}:${req.method}`) + /** + * @param {MemoryCacheStore} value + */ + constructor (value) { + super() + this.#value = value + } + + get rawTrailers () { + return this.#value.value.rawTrailers + } + + /** + * @param {Buffer[] | undefined} trailers + */ + set rawTrailers (trailers) { + this.#value.value.rawTrailers = trailers + } + + /** + * @param {Buffer} chunk + * @param {*} _ + * @param {() => void} callback + */ + _write (chunk, _, callback) { + this.#value.value.body.push(chunk) + this.#value.emitter.emit('write', chunk) + callback() + } + + /** + * @param {() => void} callback + */ + _final (callback) { + this.#value.complete = true + this.#value.emitter.emit('final') + + callback() + } + + /** + * @param {Error} err + * @param {() => void} callback + */ + _destroy (err, callback) { + this.#value.emitter.emit('error', err) + callback(err) } } diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index d5225079918..0e5c8689700 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -21,9 +21,9 @@ class CacheHandler extends DecoratorHandler { */ #handler = null /** - * @type {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue | undefined} + * @type {import('../../types/cache-interceptor.d.ts').default.CacheStoreWriteable | undefined} */ - #value = undefined + #writeStream /** * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions} opts @@ -93,14 +93,12 @@ class CacheHandler extends DecoratorHandler { ) } - const maxEntrySize = this.#getMaxEntrySize() const contentLength = Number(headers['content-length']) const currentSize = this.#getSizeOfBuffers(rawHeaders) + (statusMessage?.length ?? 0) + 64 if ( !Number.isInteger(contentLength) || - contentLength > maxEntrySize || - currentSize > maxEntrySize || + this.#opts.store.isFull() || this.#opts.store.entryCount >= this.#opts.store.maxEntries ) { return this.#handler.onHeaders( @@ -137,8 +135,7 @@ class CacheHandler extends DecoratorHandler { cacheControlDirectives ) - this.#value = { - complete: false, + this.#writeStream = this.#opts.store.createWriteStream(this.#req, { statusCode, statusMessage, rawHeaders: strippedHeaders, @@ -148,7 +145,12 @@ class CacheHandler extends DecoratorHandler { cachedAt: now, staleAt: now + staleAt, deleteAt: now + deleteAt - } + }) + + this.#writeStream.on('drain', resume) + this.#writeStream.on('error', () => { + // TODO just pass to the #handler.onError maybe? + }) } if (typeof this.#handler.onHeaders === 'function') { @@ -169,14 +171,8 @@ class CacheHandler extends DecoratorHandler { * @returns {boolean} */ onData (chunk) { - if (this.#value) { - this.#value.size += chunk.length - - if (this.#value.size > this.#getMaxEntrySize()) { - this.#value = null - } else { - this.#value.body.push(chunk) - } + if (this.#writeStream) { + this.#writeStream.write(chunk) } if (typeof this.#handler.onData === 'function') { @@ -190,18 +186,12 @@ class CacheHandler extends DecoratorHandler { * @param {string[] | null} rawTrailers */ onComplete (rawTrailers) { - if (this.#value) { - this.#value.complete = true - this.#value.rawTrailers = rawTrailers - this.#value.size += this.#getSizeOfBuffers(rawTrailers) - - // If we're still under the max entry size, let's add it to the cache - if (this.#getMaxEntrySize() > this.#value.size) { - const result = this.#opts.store.put(this.#req, this.#value) - if (result && result.constructor.name === 'Promise') { - result.catch(err => this.#handler.onError(err)) - } + if (this.#writeStream) { + if (rawTrailers) { + this.#writeStream.rawTrailers = rawTrailers } + + this.#writeStream.end() } if (typeof this.#handler.onComplete === 'function') { @@ -215,7 +205,11 @@ class CacheHandler extends DecoratorHandler { * @param {Error} err */ onError (err) { - this.#value = undefined + if (this.#writeStream) { + this.#writeStream.destroy(err) + this.#writeStream = undefined + } + if (typeof this.#handler.onError === 'function') { this.#handler.onError(err) } diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 1287d8151ff..becc8073a7c 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -7,111 +7,6 @@ const { UNSAFE_METHODS } = require('../util/cache.js') const AGE_HEADER = Buffer.from('age') -/** - * Gives the downstream handler the request's cached response or dispatches - * it if it isn't cached - * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions} globalOpts - * @param {*} dispatch TODO type - * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} opts - * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler - * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue | undefined} value - */ -function handleCachedResult ( - globalOpts, - dispatch, - opts, - handler, - value -) { - if (!value) { - // Request isn't cached, let's continue dispatching it - dispatch(opts, new CacheHandler(globalOpts, opts, handler)) - return - } - - // Dump body on error - if (typeof opts.body === 'object' && opts.body.constructor.name === 'Readable') { - opts.body?.on('error', () => {}).resume() - } - - const respondWithCachedValue = () => { - const ac = new AbortController() - const signal = ac.signal - - try { - if (typeof handler.onConnect === 'function') { - handler.onConnect(ac.abort) - signal.throwIfAborted() - } - - if (typeof handler.onHeaders === 'function') { - // Add the age header - // https://www.rfc-editor.org/rfc/rfc9111.html#name-age - const age = Math.round((Date.now() - value.cachedAt) / 1000) - - value.rawHeaders.push(AGE_HEADER, Buffer.from(`${age}`)) - - handler.onHeaders(value.statusCode, value.rawHeaders, () => {}, value.statusMessage) - signal.throwIfAborted() - } - - let trailers = null - if (opts.method !== 'HEAD') { - if (typeof handler.onData === 'function') { - for (const chunk of value.body) { - while (!handler.onData(chunk)) { - signal.throwIfAborted() - } - } - } - - if (value.rawTrailers !== undefined) { - trailers = value.rawTrailers - } - } - - if (typeof handler.onComplete === 'function') { - handler.onComplete(trailers) - } - } catch (err) { - if (typeof handler.onError === 'function') { - handler.onError(err) - } - } - } - - // Check if the response is stale - const now = Date.now() - if (now > value.staleAt) { - if (now > value.deleteAt) { - // Safety check in case the store gave us a response that should've been - // deleted already - dispatch(opts, new CacheHandler(globalOpts, opts, handler)) - return - } - - if (!opts.headers) { - opts.headers = {} - } - - opts.headers['if-modified-since'] = new Date(value.cachedAt).toUTCString() - - // Need to revalidate the response - dispatch( - opts, - new CacheRevalidationHandler( - respondWithCachedValue, - new CacheHandler(globalOpts, opts, handler) - ) - ) - - return - } - - // Response is still fresh, let's return it - respondWithCachedValue() -} - /** * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions | undefined} globalOpts * @returns {import('../../types/dispatcher.d.ts').default.DispatcherComposeInterceptor} @@ -166,27 +61,94 @@ module.exports = globalOpts => { return dispatch(opts, handler) } - const result = globalOpts.store.get(opts) - if (result && typeof result.then === 'function') { - result.then(value => { - handleCachedResult( - globalOpts, - dispatch, - opts, - handler, - value - ) - }).catch(handler.onError) - } else { - handleCachedResult( - globalOpts, - dispatch, + const stream = globalOpts.store.createReadStream(opts) + if (!stream) { + // Request isn't cached + return dispatch(opts, handler) + } + + const { value } = stream + + // Dump body on error + if (typeof opts.body === 'object' && opts.body.constructor.name === 'Readable') { + opts.body?.on('error', () => {}).resume() + } + + const respondWithCachedValue = () => { + const ac = new AbortController() + const signal = ac.signal + + try { + if (typeof handler.onConnect === 'function') { + handler.onConnect(ac.abort) + signal.throwIfAborted() + } + + if (typeof handler.onHeaders === 'function') { + // Add the age header + // https://www.rfc-editor.org/rfc/rfc9111.html#name-age + const age = Math.round((Date.now() - value.cachedAt) / 1000) + + value.rawHeaders.push(AGE_HEADER, Buffer.from(`${age}`)) + + handler.onHeaders(value.statusCode, value.rawHeaders, () => {}, value.statusMessage) + signal.throwIfAborted() + } + + if (opts.method === 'HEAD') { + if (typeof handler.onComplete === 'function') { + handler.onComplete(null) + } + } else { + if (typeof handler.onData === 'function') { + stream.on('data', chunk => { + while (!handler.onData(chunk)) { + signal.throwIfAborted() + } + }) + } + + if (typeof handler.onComplete === 'function') { + stream.on('close', () => { + handler.onComplete(value.rawTrailers ?? null) + }) + } + } + } catch (err) { + if (typeof handler.onError === 'function') { + handler.onError(err) + } + } + } + + // Check if the response is stale + const now = Date.now() + if (now > value.staleAt) { + if (now > value.deleteAt) { + // Safety check in case the store gave us a response that should've been + // deleted already + dispatch(opts, new CacheHandler(globalOpts, opts, handler)) + return + } + + if (!opts.headers) { + opts.headers = {} + } + + opts.headers['if-modified-since'] = new Date(value.cachedAt).toUTCString() + + // Need to revalidate the response + dispatch( opts, - handler, - result + new CacheRevalidationHandler( + respondWithCachedValue, + new CacheHandler(globalOpts, opts, handler) + ) ) } + respondWithCachedValue() + return true } } diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index d4206286327..da80b0af824 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -1,3 +1,4 @@ +import { Readable, Writable } from 'node:stream' import Dispatcher from './dispatcher' export default CacheHandler @@ -21,36 +22,13 @@ declare namespace CacheHandler { */ export interface CacheStore { /** - * The amount of responses that are being cached + * Whether or not the cache is full and can not store any more responses */ - get entryCount(): number + get isFull(): boolean - /** - * The max amount of entries this cache can hold. If the size is greater - * than or equal to this, new responses will not be cached. - * @default Infinity - */ - get maxEntries(): number - - /** - * The max size of each value. If the content-length header is greater than - * this or the response ends up over this, new responses will not be cached - * @default Infinity - */ - get maxEntrySize(): number - - /** - * Get a request's cached response if it exists. - * Note: it is the cache store's responsibility to enforce the vary header checks - */ - get(key: Dispatcher.RequestOptions): CacheStoreValue | Promise | undefined; + createReadStream(req: Dispatcher.RequestOptions): CacheStoreReadable | undefined - /** - * Add a new request to the cache - * @param key - * @param opts - */ - put(key: Dispatcher.RequestOptions, value: CacheStoreValue): void | Promise; + createWriteStream(req: Dispatcher.RequestOptions, value: CacheStoreValue): CacheStoreWriteable | undefined /** * Delete all of the cached responses from a certain origin (host) @@ -58,15 +36,23 @@ declare namespace CacheHandler { deleteByOrigin(origin: string): void | Promise } + export interface CacheStoreReadable extends Readable { + get value(): Omit + } + + export interface CacheStoreWriteable extends Writable { + set rawTrailers(rawTrailers: Buffer[] | undefined) + } + export interface CacheStoreValue { /** * True if the response is complete, otherwise the request is still in-flight */ - complete: boolean; + // complete: boolean; statusCode: number; statusMessage: string; rawHeaders: Buffer[]; - rawTrailers?: Buffer[]; + rawTrailers?: string[]; body: Buffer[] /** * Headers defined by the Vary header and their respective values for @@ -106,12 +92,12 @@ declare namespace CacheHandler { export class MemoryCacheStore implements CacheStore { constructor (opts?: MemoryCacheStoreOpts) - get entryCount (): number - get maxEntries (): number - get maxEntrySize (): number + get isFull (): boolean + + createReadStream (req: Dispatcher.RequestOptions): Readable | undefined + + createWriteStream (req: Dispatcher.RequestOptions, value: CacheStoreValue): Writable - get (key: Dispatcher.RequestOptions): CacheStoreValue | undefined - put (key: Dispatcher.RequestOptions, opts: CacheStoreValue): void deleteByOrigin (origin: string): void } } From cbe7b972bda3358eba4d532d839d4e97e75a0fbc Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Fri, 20 Sep 2024 10:51:57 -0700 Subject: [PATCH 17/58] fix tests Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 25 ++-- lib/handler/cache-handler.js | 2 +- lib/interceptor/cache.js | 11 +- test/cache-interceptor/cache-stores.js | 179 +++++++++++++++++++------ types/cache-interceptor.d.ts | 6 +- 5 files changed, 162 insertions(+), 61 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index e41b243941f..6212d3e608c 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -61,7 +61,7 @@ class MemoryCacheStore { } get isFull () { - return this.#entryCount < this.#maxEntries + return this.#entryCount >= this.#maxEntries } createReadStream (req) { @@ -129,7 +129,7 @@ class MemoryCacheStore { // TODO see if value already exists or not const values = this.#getValues(req, true) - this.entryCount++ + this.#entryCount++ /** * TODO what's a better name for this @@ -157,12 +157,12 @@ class MemoryCacheStore { break } - const middleIndex = (startIndex + endIndex) / 2 + const middleIndex = Math.floor((startIndex + endIndex) / 2) const middleValue = values[middleIndex] if (value.deleteAt === middleIndex) { values.splice(middleIndex, 0, storedValue) break - } else if (value.deleteAt > middleValue.deleteAt) { + } else if (value.deleteAt > middleValue.value.deleteAt) { endIndex = middleIndex continue } else { @@ -227,7 +227,9 @@ class MemoryStoreReadableStream extends Readable { this.#value = value this.#chunksToSend = [...this.#value.value.body] - if (!value.complete) { + if (value.complete) { + this.#chunksToSend.push(null) + } else { value.emitter.on('write', this.#chunksToSend.push) value.emitter.on('final', () => { this.#chunksToSend.push(null) @@ -236,20 +238,23 @@ class MemoryStoreReadableStream extends Readable { } get value () { - return this.#value + return this.#value.value } /** * @param {number} size */ _read (size) { - // TODO what to do if we get a read but we're still waiting on more chunks? + if (this.#chunksToSend.length === 0) { + return + } + if (size > this.#chunksToSend.length) { size = this.#chunksToSend.length } for (let i = 0; i < size; i++) { - this.push(this.#chunksToSend.pop()) + this.push(this.#chunksToSend.shift()) } } } @@ -306,7 +311,9 @@ class MemoryStoreWritableStream extends Writable { * @param {() => void} callback */ _destroy (err, callback) { - this.#value.emitter.emit('error', err) + if (err) { + this.#value.emitter.emit('error', err) + } callback(err) } } diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 0e5c8689700..86b7d52930c 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -98,7 +98,7 @@ class CacheHandler extends DecoratorHandler { this.#getSizeOfBuffers(rawHeaders) + (statusMessage?.length ?? 0) + 64 if ( !Number.isInteger(contentLength) || - this.#opts.store.isFull() || + this.#opts.store.isFull || this.#opts.store.entryCount >= this.#opts.store.maxEntries ) { return this.#handler.onHeaders( diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index becc8073a7c..a1a72b8ccbb 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -64,7 +64,7 @@ module.exports = globalOpts => { const stream = globalOpts.store.createReadStream(opts) if (!stream) { // Request isn't cached - return dispatch(opts, handler) + return dispatch(opts, new CacheHandler(globalOpts, opts, handler)) } const { value } = stream @@ -79,6 +79,7 @@ module.exports = globalOpts => { const signal = ac.signal try { + console.log(' on conn') if (typeof handler.onConnect === 'function') { handler.onConnect(ac.abort) signal.throwIfAborted() @@ -91,7 +92,7 @@ module.exports = globalOpts => { value.rawHeaders.push(AGE_HEADER, Buffer.from(`${age}`)) - handler.onHeaders(value.statusCode, value.rawHeaders, () => {}, value.statusMessage) + handler.onHeaders(value.statusCode, value.rawHeaders, stream.resume, value.statusMessage) signal.throwIfAborted() } @@ -109,8 +110,8 @@ module.exports = globalOpts => { } if (typeof handler.onComplete === 'function') { - stream.on('close', () => { - handler.onComplete(value.rawTrailers ?? null) + stream.on('end', () => { + handler.onComplete(value.rawTrailers ?? []) }) } } @@ -145,6 +146,8 @@ module.exports = globalOpts => { new CacheHandler(globalOpts, opts, handler) ) ) + + return } respondWithCachedValue() diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index 1890c99f04e..3089086e7d9 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -1,16 +1,17 @@ const { describe, test } = require('node:test') -const { deepStrictEqual, strictEqual } = require('node:assert') +const { deepStrictEqual, notEqual, equal } = require('node:assert') +const { once } = require('node:events') const MemoryCacheStore = require('../../lib/cache/memory-cache-store') +cacheStoreTests(MemoryCacheStore) + /** - * @param {typeof import('../../types/cache-interceptor').default.CacheStore} CacheStore + * @param {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore */ function cacheStoreTests (CacheStore) { describe(CacheStore.prototype.constructor.name, () => { + // Checks that it can store & fetch different responses test('basic functionality', async () => { - // Checks that it can store & fetch different responses - const store = new CacheStore() - const request = { origin: 'localhost', path: '/', @@ -18,26 +19,44 @@ function cacheStoreTests (CacheStore) { headers: {} } const requestValue = { - complete: true, statusCode: 200, statusMessage: '', rawHeaders: [1, 2, 3], - rawTrailers: [4, 5, 6], - body: ['part1', 'part2'], size: 16, cachedAt: Date.now(), staleAt: Date.now() + 10000, deleteAt: Date.now() + 20000 } + const requestBody = ['asd', '123'] + const requestTrailers = ['a', 'b', 'c'] + + /** + * @type {import('../../types/cache-interceptor.d.ts').default.CacheStore} + */ + const store = new CacheStore() // Sanity check - strictEqual(await store.get(request), undefined) + equal(store.createReadStream(request), undefined) + + // Write the response to the store + let writeStream = store.createWriteStream(request, { + ...requestValue, + body: [] + }) + notEqual(writeStream, undefined) + writeResponse(writeStream, requestBody, requestTrailers) + + // Now try fetching it with a deep copy of the original request + let readStream = store.createReadStream(structuredClone(request)) + notEqual(readStream, undefined) - // Add a response to the cache and try fetching it with a deep copy of - // the original request - await store.put(request, requestValue) - deepStrictEqual(store.get(structuredClone(request)), requestValue) + deepStrictEqual(await readResponse(readStream), { + ...requestValue, + body: requestBody, + rawTrailers: requestTrailers + }) + // Now let's write another request to the store const anotherRequest = { origin: 'localhost', path: '/asd', @@ -45,26 +64,36 @@ function cacheStoreTests (CacheStore) { headers: {} } const anotherValue = { - complete: true, statusCode: 200, statusMessage: '', rawHeaders: [1, 2, 3], - rawTrailers: [4, 5, 6], - body: ['part1', 'part2'], size: 16, cachedAt: Date.now(), staleAt: Date.now() + 10000, deleteAt: Date.now() + 20000 } + const anotherBody = ['asd2', '1234'] + const anotherTrailers = ['d', 'e', 'f'] // We haven't cached this one yet, make sure it doesn't confuse it with // another request - strictEqual(await store.get(anotherRequest), undefined) + equal(store.createReadStream(anotherRequest), undefined) + + // Now let's cache it + writeStream = store.createWriteStream(anotherRequest, { + ...anotherValue, + body: [] + }) + notEqual(writeStream, undefined) + writeResponse(writeStream, anotherBody, anotherTrailers) - // Add a response to the cache and try fetching it with a deep copy of - // the original request - await store.put(anotherRequest, anotherValue) - deepStrictEqual(store.get(structuredClone(anotherRequest)), anotherValue) + readStream = store.createReadStream(anotherRequest) + notEqual(readStream, undefined) + deepStrictEqual(await readResponse(readStream), { + ...anotherValue, + body: anotherBody, + rawTrailers: anotherTrailers + }) }) test('returns stale response if possible', async () => { @@ -75,21 +104,35 @@ function cacheStoreTests (CacheStore) { headers: {} } const requestValue = { - complete: true, statusCode: 200, statusMessage: '', rawHeaders: [1, 2, 3], - rawTrailers: [4, 5, 6], - body: ['part1', 'part2'], size: 16, cachedAt: Date.now() - 10000, staleAt: Date.now() - 1, deleteAt: Date.now() + 20000 } + const requestBody = ['part1', 'part2'] + const requestTrailers = [4, 5, 6] + /** + * @type {import('../../types/cache-interceptor.d.ts').default.CacheStore} + */ const store = new CacheStore() - await store.put(request, requestValue) - deepStrictEqual(await store.get(request), requestValue) + + const writeStream = store.createWriteStream(request, { + ...requestValue, + body: [] + }) + notEqual(writeStream, undefined) + writeResponse(writeStream, requestBody, requestTrailers) + + const readStream = store.createReadStream(request) + deepStrictEqual(await readResponse(readStream), { + ...requestValue, + body: requestBody, + rawTrailers: requestTrailers + }) }) test('doesn\'t return response past deletedAt', async () => { @@ -103,23 +146,30 @@ function cacheStoreTests (CacheStore) { complete: true, statusCode: 200, statusMessage: '', - rawHeaders: [1, 2, 3], - rawTrailers: [4, 5, 6], - body: ['part1', 'part2'], size: 16, cachedAt: Date.now() - 20000, staleAt: Date.now() - 10000, deleteAt: Date.now() - 5 } + const requestBody = ['part1', 'part2'] + const rawTrailers = [4, 5, 6] + /** + * @type {import('../../types/cache-interceptor.d.ts').default.CacheStore} + */ const store = new CacheStore() - await store.put(request, requestValue) - strictEqual(await store.get(request), undefined) + + const writeStream = store.createWriteStream(request, { + ...requestValue, + body: [] + }) + notEqual(writeStream, undefined) + writeResponse(writeStream, requestBody, rawTrailers) + + equal(store.createReadStream(request), undefined) }) test('respects vary directives', async () => { - const store = new CacheStore() - const request = { origin: 'localhost', path: '/', @@ -133,8 +183,6 @@ function cacheStoreTests (CacheStore) { statusCode: 200, statusMessage: '', rawHeaders: [1, 2, 3], - rawTrailers: [4, 5, 6], - body: ['part1', 'part2'], vary: { 'some-header': 'hello world' }, @@ -143,12 +191,31 @@ function cacheStoreTests (CacheStore) { staleAt: Date.now() + 10000, deleteAt: Date.now() + 20000 } + const requestBody = ['part1', 'part2'] + const requestTrailers = [4, 5, 6] + + /** + * @type {import('../../types/cache-interceptor.d.ts').default.CacheStore} + */ + const store = new CacheStore() // Sanity check - strictEqual(await store.get(request), undefined) + equal(store.createReadStream(request), undefined) - await store.put(request, requestValue) - deepStrictEqual(store.get(request), requestValue) + const writeStream = store.createWriteStream(request, { + ...requestValue, + body: [] + }) + notEqual(writeStream, undefined) + writeResponse(writeStream, requestBody, requestTrailers) + + const readStream = store.createReadStream(structuredClone(request)) + notEqual(readStream, undefined) + deepStrictEqual(await readResponse(readStream), { + ...requestValue, + body: requestBody, + rawTrailers: requestTrailers + }) const nonMatchingRequest = { origin: 'localhost', @@ -158,11 +225,39 @@ function cacheStoreTests (CacheStore) { 'some-header': 'another-value' } } - deepStrictEqual(store.get(nonMatchingRequest), undefined) - - deepStrictEqual(store.get(structuredClone(request)), requestValue) + equal(store.createReadStream(nonMatchingRequest), undefined) }) }) } -cacheStoreTests(MemoryCacheStore) +/** + * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreWriteable} stream + * @param {string[]} body + * @param {string[]} trailers + */ +function writeResponse (stream, body, trailers) { + for (const chunk of body) { + stream.write(Buffer.from(chunk)) + } + + stream.rawTrailers = trailers + stream.end() +} + +/** + * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreReadable} stream + * @returns {Promise} + */ +async function readResponse (stream) { + const body = [] + stream.on('data', chunk => { + body.push(chunk.toString()) + }) + + await once(stream, 'end') + + return { + ...stream.value, + body + } +} diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index da80b0af824..6725b2a1e55 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -45,15 +45,11 @@ declare namespace CacheHandler { } export interface CacheStoreValue { - /** - * True if the response is complete, otherwise the request is still in-flight - */ - // complete: boolean; statusCode: number; statusMessage: string; rawHeaders: Buffer[]; rawTrailers?: string[]; - body: Buffer[] + body: Buffer[] // TODO should delete? /** * Headers defined by the Vary header and their respective values for * later comparison From edc077283a7ef26d23d7de17fb1d45cde367cd16 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Fri, 20 Sep 2024 23:31:53 -0700 Subject: [PATCH 18/58] check if the response is already cached again Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 171 +++++++++++++------------ lib/interceptor/cache.js | 15 +-- test/cache-interceptor/cache-stores.js | 8 ++ test/cache-interceptor/interceptor.js | 39 +++++- 4 files changed, 139 insertions(+), 94 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 6212d3e608c..9baa30b96b6 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -3,8 +3,6 @@ const EventEmitter = require('node:events') const { Writable, Readable } = require('node:stream') -// todo clean this up - /** * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore * @implements {CacheStore} @@ -69,47 +67,12 @@ class MemoryCacheStore { throw new TypeError(`expected req to be object, got ${typeof req}`) } - const values = this.#getValues(req, false) + const values = this.#getValuesForRequest(req, false) if (!values) { - return + return undefined } - /** - * @type {MemoryStoreValue} - */ - let value - const now = Date.now() - for (let i = values.length - 1; i >= 0; i--) { - const current = values[i] - const currentCacheValue = current.value - if (now >= currentCacheValue.deleteAt) { - // We've reached expired values, let's delete them - this.#entryCount -= values.length - i - values.length = i - break - } - - let matches = true - - if (currentCacheValue.vary) { - if (!req.headers) { - matches = false - break - } - - for (const key in currentCacheValue.vary) { - if (currentCacheValue.vary[key] !== req.headers[key]) { - matches = false - break - } - } - } - - if (matches) { - value = current - break - } - } + const value = this.#findValue(req, values) return value ? new MemoryStoreReadableStream(value) : undefined } @@ -126,48 +89,46 @@ class MemoryCacheStore { return undefined } - // TODO see if value already exists or not + const values = this.#getValuesForRequest(req, true) + let storedValue = this.#findValue(req, values) + if (!storedValue) { + this.#entryCount++ - const values = this.#getValues(req, true) - this.#entryCount++ - - /** - * TODO what's a better name for this - * @type {MemoryStoreValue} - */ - const storedValue = { - complete: false, - value, - emitter: new EventEmitter() - } + // TODO better name for this + storedValue = { + complete: false, + value, + emitter: new EventEmitter() + } - if ( - values.length === 0 || - value.deleteAt < values[values.length - 1].deleteAt - ) { - values.push(storedValue) - } else if (value.deleteAt >= values[0].deleteAt) { - values.unshift(storedValue) - } else { - let startIndex = 0 - let endIndex = values.length - while (true) { - if (startIndex === endIndex) { - values.splice(startIndex, 0, storedValue) - break - } + if ( + values.length === 0 || + value.deleteAt < values[values.length - 1].deleteAt + ) { + values.push(storedValue) + } else if (value.deleteAt >= values[0].deleteAt) { + values.unshift(storedValue) + } else { + let startIndex = 0 + let endIndex = values.length + while (true) { + if (startIndex === endIndex) { + values.splice(startIndex, 0, storedValue) + break + } - const middleIndex = Math.floor((startIndex + endIndex) / 2) - const middleValue = values[middleIndex] - if (value.deleteAt === middleIndex) { - values.splice(middleIndex, 0, storedValue) - break - } else if (value.deleteAt > middleValue.value.deleteAt) { - endIndex = middleIndex - continue - } else { - startIndex = middleIndex - continue + const middleIndex = Math.floor((startIndex + endIndex) / 2) + const middleValue = values[middleIndex] + if (value.deleteAt === middleIndex) { + values.splice(middleIndex, 0, storedValue) + break + } else if (value.deleteAt > middleValue.value.deleteAt) { + endIndex = middleIndex + continue + } else { + startIndex = middleIndex + continue + } } } } @@ -183,10 +144,12 @@ class MemoryCacheStore { } /** + * Gets all of the requests of the same origin, path, and method. Does not + * take the `vary` property into account. * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req * @returns {MemoryStoreValue[] | undefined} */ - #getValues (req, makeIfDoesntExist) { + #getValuesForRequest (req, makeIfDoesntExist) { // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 let cachedPaths = this.#data.get(req.origin) if (!cachedPaths) { @@ -206,6 +169,54 @@ class MemoryCacheStore { return values } + + /** + * Given a list of values of a certain request, this decides the best value + * to respond with. + * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req + * @param {MemoryStoreValue[]} values + * @returns {MemoryStoreValue | undefined} + */ + #findValue (req, values) { + /** + * @type {MemoryStoreValue} + */ + let value + const now = Date.now() + for (let i = values.length - 1; i >= 0; i--) { + const current = values[i] + const currentCacheValue = current.value + if (now >= currentCacheValue.deleteAt) { + // We've reached expired values, let's delete them + this.#entryCount -= values.length - i + values.length = i + break + } + + let matches = true + + if (currentCacheValue.vary) { + if (!req.headers) { + matches = false + break + } + + for (const key in currentCacheValue.vary) { + if (currentCacheValue.vary[key] !== req.headers[key]) { + matches = false + break + } + } + } + + if (matches) { + value = current + break + } + } + + return value + } } class MemoryStoreReadableStream extends Readable { diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index a1a72b8ccbb..b86d3e751f2 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -17,23 +17,14 @@ module.exports = globalOpts => { } if (globalOpts.store) { - for (const fn of ['get', 'put']) { + for (const fn of ['createReadStream', 'createWriteStream', 'deleteByOrigin']) { if (typeof globalOpts.store[fn] !== 'function') { throw new TypeError(`CacheStore needs a \`${fn}()\` function`) } } - for (const getter of ['entryCount', 'maxEntries', 'maxEntrySize']) { - const actualType = typeof globalOpts.store[getter] - if (actualType !== 'number') { - throw new TypeError(`CacheStore needs a ${getter} property with type number, current type: ${actualType}`) - } - } - - for (const value of ['maxEntries', 'maxEntry']) { - if (globalOpts.store[value] <= 0) { - throw new TypeError(`CacheStore ${value} needs to be >= 1`) - } + if (typeof globalOpts.store.isFull !== 'boolean') { + throw new TypeError(`CacheStore needs a isFull getter with type boolean, current type: ${typeof globalOpts.store.isFull}`) } } else { globalOpts.store = new MemoryCacheStore() diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index 3089086e7d9..6708531ff23 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -10,6 +10,14 @@ cacheStoreTests(MemoryCacheStore) */ function cacheStoreTests (CacheStore) { describe(CacheStore.prototype.constructor.name, () => { + test('matches interface', async () => { + const store = new CacheStore() + equal(typeof store.isFull, 'boolean') + equal(typeof store.createReadStream, 'function') + equal(typeof store.createWriteStream, 'function') + equal(typeof store.deleteByOrigin, 'function') + }) + // Checks that it can store & fetch different responses test('basic functionality', async () => { const request = { diff --git a/test/cache-interceptor/interceptor.js b/test/cache-interceptor/interceptor.js index 23d69f65b13..54d259ff360 100644 --- a/test/cache-interceptor/interceptor.js +++ b/test/cache-interceptor/interceptor.js @@ -1,10 +1,10 @@ 'use strict' const { describe, test, after } = require('node:test') -const { strictEqual, notEqual } = require('node:assert') +const { strictEqual, notEqual, fail } = require('node:assert') const { createServer } = require('node:http') const { once } = require('node:events') -const { Client, interceptors } = require('../../index') +const { Client, interceptors, cacheStores } = require('../../index') describe('Cache Interceptor', () => { test('doesn\'t cache request w/ no cache-control header', async () => { @@ -206,4 +206,39 @@ describe('Cache Interceptor', () => { }) await completed }) + + test('respects cache store\'s isFull property', async () => { + const server = createServer((_, res) => { + res.end('asd') + }).listen(0) + + after(() => server.close()) + await once(server, 'listening') + + const store = new cacheStores.MemoryCacheStore() + Object.defineProperty(store, 'isFull', { + value: true + }) + + store.createWriteStream = (...args) => { + fail('shouln\'t have reached this') + } + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache({ store })) + + await client.request({ + origin: 'localhost', + method: 'GET', + path: '/', + headers: { + 'some-header': 'abc123', + 'another-header': '123abc' + } + }) + }) + + test('shares responses still in-flight to the same request', async () => { + // TODO + }) }) From d7b24a4e778deda3a2b66cd9cd8866b3146a58f2 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Sat, 21 Sep 2024 09:15:09 -0700 Subject: [PATCH 19/58] backpressure patch Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/handler/cache-handler.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 86b7d52930c..7694c690390 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -171,13 +171,17 @@ class CacheHandler extends DecoratorHandler { * @returns {boolean} */ onData (chunk) { + let paused = false + if (this.#writeStream) { - this.#writeStream.write(chunk) + paused ||= this.#writeStream.write(chunk) === false } if (typeof this.#handler.onData === 'function') { - return this.#handler.onData(chunk) + paused ||= this.#handler.onData(chunk) === false } + + return !paused } /** From 0877f952ff61f20f8d807a40d77e40c3a4f2601e Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Sat, 21 Sep 2024 21:30:46 -0700 Subject: [PATCH 20/58] move body out of CacheStoreValue, remove size property Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 59 ++++++++++++++++++++------ lib/handler/cache-handler.js | 15 +------ lib/interceptor/cache.js | 1 - test/cache-interceptor/cache-stores.js | 7 --- types/cache-interceptor.d.ts | 5 --- 5 files changed, 47 insertions(+), 40 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 9baa30b96b6..4e7b6fb81fa 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -10,6 +10,7 @@ const { Writable, Readable } = require('node:stream') * @typedef {{ * complete: boolean * value: import('../../types/cache-interceptor.d.ts').default.CacheStoreValue + * body: Buffer[] * emitter: EventEmitter * }} MemoryStoreValue */ @@ -98,6 +99,7 @@ class MemoryCacheStore { storedValue = { complete: false, value, + body: [], emitter: new EventEmitter() } @@ -133,7 +135,13 @@ class MemoryCacheStore { } } - return new MemoryStoreWritableStream(storedValue) + return new MemoryStoreWritableStream( + storedValue, + this.#maxEntrySize, + () => { + values.filter(value => value !== storedValue) + } + ) } /** @@ -227,7 +235,7 @@ class MemoryStoreReadableStream extends Readable { /** * @type {Buffer[]} */ - #chunksToSend + #chunksToSend = [] /** * @param {MemoryStoreValue} value @@ -236,13 +244,15 @@ class MemoryStoreReadableStream extends Readable { super() this.#value = value - this.#chunksToSend = [...this.#value.value.body] if (value.complete) { - this.#chunksToSend.push(null) + this.#chunksToSend = [...this.#value.body, null] } else { - value.emitter.on('write', this.#chunksToSend.push) - value.emitter.on('final', () => { + value.emitter.on('body', () => { + this.#chunksToSend = [...this.#value.body, null] + }) + + value.emitter.on('error', () => { this.#chunksToSend.push(null) }) } @@ -270,19 +280,29 @@ class MemoryStoreReadableStream extends Readable { } } -// TODO enforce max entry size, ... +// TODO delete the value if it exceeds maxEntrySize or there's an error class MemoryStoreWritableStream extends Writable { /** * @type {MemoryStoreValue} */ #value + #currentSize = 0 + #maxEntrySize = 0 + /** + * @type {Buffer[]} + */ + #body = [] + #deleteValueCallback /** * @param {MemoryCacheStore} value + * @param {number} maxEntrySize */ - constructor (value) { + constructor (value, maxEntrySize, deleteValueCallback) { super() this.#value = value + this.#maxEntrySize = maxEntrySize + this.#deleteValueCallback = deleteValueCallback } get rawTrailers () { @@ -301,9 +321,16 @@ class MemoryStoreWritableStream extends Writable { * @param {*} _ * @param {() => void} callback */ - _write (chunk, _, callback) { - this.#value.value.body.push(chunk) - this.#value.emitter.emit('write', chunk) + _write (chunk, encoding, callback) { + if (typeof chunk === 'string') { + chunk = Buffer.from(chunk, encoding) + } + + this.#currentSize += chunk.byteLength + if (this.#currentSize < this.#maxEntrySize) { + this.#body.push(chunk) + } + callback() } @@ -311,8 +338,13 @@ class MemoryStoreWritableStream extends Writable { * @param {() => void} callback */ _final (callback) { - this.#value.complete = true - this.#value.emitter.emit('final') + if (this.#currentSize < this.#maxEntrySize) { + this.#value.complete = true + this.#value.body = this.#body + + this.#value.emitter.emit('body') + this.#value.emitter = undefined + } callback() } @@ -324,6 +356,7 @@ class MemoryStoreWritableStream extends Writable { _destroy (err, callback) { if (err) { this.#value.emitter.emit('error', err) + this.#deleteValueCallback() } callback(err) } diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 7694c690390..bd5fc916cc8 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -94,8 +94,6 @@ class CacheHandler extends DecoratorHandler { } const contentLength = Number(headers['content-length']) - const currentSize = - this.#getSizeOfBuffers(rawHeaders) + (statusMessage?.length ?? 0) + 64 if ( !Number.isInteger(contentLength) || this.#opts.store.isFull || @@ -139,18 +137,14 @@ class CacheHandler extends DecoratorHandler { statusCode, statusMessage, rawHeaders: strippedHeaders, - body: [], vary: varyDirectives, - size: currentSize, cachedAt: now, staleAt: now + staleAt, deleteAt: now + deleteAt }) this.#writeStream.on('drain', resume) - this.#writeStream.on('error', () => { - // TODO just pass to the #handler.onError maybe? - }) + this.#writeStream.on('error', this.#handler.onError) } if (typeof this.#handler.onHeaders === 'function') { @@ -219,13 +213,6 @@ class CacheHandler extends DecoratorHandler { } } - /** - * @returns {number} - */ - #getMaxEntrySize () { - return this.#opts.store.maxEntrySize ?? Infinity - } - /** * @param {string[] | Buffer[]} arr * @returns {number} diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index b86d3e751f2..d55e316bd2d 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -70,7 +70,6 @@ module.exports = globalOpts => { const signal = ac.signal try { - console.log(' on conn') if (typeof handler.onConnect === 'function') { handler.onConnect(ac.abort) signal.throwIfAborted() diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index 6708531ff23..a8402e6ee55 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -30,7 +30,6 @@ function cacheStoreTests (CacheStore) { statusCode: 200, statusMessage: '', rawHeaders: [1, 2, 3], - size: 16, cachedAt: Date.now(), staleAt: Date.now() + 10000, deleteAt: Date.now() + 20000 @@ -75,7 +74,6 @@ function cacheStoreTests (CacheStore) { statusCode: 200, statusMessage: '', rawHeaders: [1, 2, 3], - size: 16, cachedAt: Date.now(), staleAt: Date.now() + 10000, deleteAt: Date.now() + 20000 @@ -115,7 +113,6 @@ function cacheStoreTests (CacheStore) { statusCode: 200, statusMessage: '', rawHeaders: [1, 2, 3], - size: 16, cachedAt: Date.now() - 10000, staleAt: Date.now() - 1, deleteAt: Date.now() + 20000 @@ -151,10 +148,8 @@ function cacheStoreTests (CacheStore) { headers: {} } const requestValue = { - complete: true, statusCode: 200, statusMessage: '', - size: 16, cachedAt: Date.now() - 20000, staleAt: Date.now() - 10000, deleteAt: Date.now() - 5 @@ -187,14 +182,12 @@ function cacheStoreTests (CacheStore) { } } const requestValue = { - complete: true, statusCode: 200, statusMessage: '', rawHeaders: [1, 2, 3], vary: { 'some-header': 'hello world' }, - size: 16, cachedAt: Date.now(), staleAt: Date.now() + 10000, deleteAt: Date.now() + 20000 diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 6725b2a1e55..712136ef0d8 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -49,16 +49,11 @@ declare namespace CacheHandler { statusMessage: string; rawHeaders: Buffer[]; rawTrailers?: string[]; - body: Buffer[] // TODO should delete? /** * Headers defined by the Vary header and their respective values for * later comparison */ vary?: Record; - /** - * Actual size of the response (i.e. size of headers + body + trailers) - */ - size: number; /** * Time in millis that this value was cached */ From e065a8e51a8a44078c49f417434e8728d2eb05df Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 23 Sep 2024 20:51:27 -0700 Subject: [PATCH 21/58] Apply suggestions from code review Co-authored-by: Robert Nagy --- lib/cache/memory-cache-store.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 4e7b6fb81fa..e9fc364a003 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -329,6 +329,8 @@ class MemoryStoreWritableStream extends Writable { this.#currentSize += chunk.byteLength if (this.#currentSize < this.#maxEntrySize) { this.#body.push(chunk) + } else { + this.#body = null // release memory as early as possible } callback() @@ -343,7 +345,6 @@ class MemoryStoreWritableStream extends Writable { this.#value.body = this.#body this.#value.emitter.emit('body') - this.#value.emitter = undefined } callback() From 230533a42eda98198ffb08f8c06e9f0c497675fd Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 23 Sep 2024 21:56:14 -0700 Subject: [PATCH 22/58] add some comments on createWriteStream Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 37 +++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index e9fc364a003..c02fe32ae47 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -91,8 +91,18 @@ class MemoryCacheStore { } const values = this.#getValuesForRequest(req, true) + let storedValue = this.#findValue(req, values) if (!storedValue) { + // The value doesn't already exist, meaning we haven't cached this + // response before. Let's assign it a value and insert it into our data + // property. + + if (this.#entryCount >= this.#maxEntries) { + // Or not, we don't have space to add another response + return undefined + } + this.#entryCount++ // TODO better name for this @@ -103,14 +113,21 @@ class MemoryCacheStore { emitter: new EventEmitter() } + // We want to sort our responses in decending order by their deleteAt + // timestamps so that deleting expired responses is faster if ( values.length === 0 || value.deleteAt < values[values.length - 1].deleteAt ) { + // Our value is either the only response for this path or our deleteAt + // time is sooner than all the other responses values.push(storedValue) } else if (value.deleteAt >= values[0].deleteAt) { + // Our deleteAt is later than everyone elses values.unshift(storedValue) } else { + // We're neither in the front or the end, let's just binary search to + // find our stop we need to be in let startIndex = 0 let endIndex = values.length while (true) { @@ -135,13 +152,15 @@ class MemoryCacheStore { } } - return new MemoryStoreWritableStream( + const writable = new MemoryStoreWritableStream( storedValue, - this.#maxEntrySize, - () => { - values.filter(value => value !== storedValue) - } + this.#maxEntrySize ) + + // Remove the value if there was some error + writable.on('error', () => { + values.filter(value => value !== storedValue) + }) } /** @@ -280,7 +299,6 @@ class MemoryStoreReadableStream extends Readable { } } -// TODO delete the value if it exceeds maxEntrySize or there's an error class MemoryStoreWritableStream extends Writable { /** * @type {MemoryStoreValue} @@ -292,17 +310,15 @@ class MemoryStoreWritableStream extends Writable { * @type {Buffer[]} */ #body = [] - #deleteValueCallback /** * @param {MemoryCacheStore} value * @param {number} maxEntrySize */ - constructor (value, maxEntrySize, deleteValueCallback) { + constructor (value, maxEntrySize) { super() this.#value = value this.#maxEntrySize = maxEntrySize - this.#deleteValueCallback = deleteValueCallback } get rawTrailers () { @@ -330,7 +346,7 @@ class MemoryStoreWritableStream extends Writable { if (this.#currentSize < this.#maxEntrySize) { this.#body.push(chunk) } else { - this.#body = null // release memory as early as possible + this.#body = null // release memory as early as possible } callback() @@ -357,7 +373,6 @@ class MemoryStoreWritableStream extends Writable { _destroy (err, callback) { if (err) { this.#value.emitter.emit('error', err) - this.#deleteValueCallback() } callback(err) } From bcd7fa1cac972bdbb8f6e496a8cec7f5c4f7efce Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 23 Sep 2024 22:15:31 -0700 Subject: [PATCH 23/58] fix type tests, make staleAt and deleteAt absolute Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/handler/cache-handler.js | 24 +++++++++++++----------- test/types/cache-interceptor.test-d.ts | 15 +++------------ 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index bd5fc916cc8..f04247e624c 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -120,12 +120,12 @@ class CacheHandler extends DecoratorHandler { } const now = Date.now() - const staleAt = determineStaleAt(headers, cacheControlDirectives) + const staleAt = determineStaleAt(now, headers, cacheControlDirectives) if (staleAt) { const varyDirectives = headers.vary ? parseVaryHeader(headers.vary, this.#req.headers) : undefined - const deleteAt = determineDeleteAt(cacheControlDirectives, staleAt) + const deleteAt = determineDeleteAt(now, cacheControlDirectives, staleAt) const strippedHeaders = stripNecessaryHeaders( rawHeaders, @@ -139,8 +139,8 @@ class CacheHandler extends DecoratorHandler { rawHeaders: strippedHeaders, vary: varyDirectives, cachedAt: now, - staleAt: now + staleAt, - deleteAt: now + deleteAt + staleAt, + deleteAt }) this.#writeStream.on('drain', resume) @@ -286,45 +286,47 @@ function canCacheResponse (statusCode, headers, cacheControlDirectives) { } /** + * @param {number} now * @param {Record} headers * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives * * @returns {number | undefined} time that the value is stale at or undefined if it shouldn't be cached */ -function determineStaleAt (headers, cacheControlDirectives) { +function determineStaleAt (now, headers, cacheControlDirectives) { // Prioritize s-maxage since we're a shared cache // s-maxage > max-age > Expire // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.2.2.10-3 const sMaxAge = cacheControlDirectives['s-maxage'] if (sMaxAge) { - return sMaxAge * 1000 + return now + (sMaxAge * 1000) } if (cacheControlDirectives.immutable) { // https://www.rfc-editor.org/rfc/rfc8246.html#section-2.2 - return 31536000 + return now + 31536000 } const maxAge = cacheControlDirectives['max-age'] if (maxAge) { - return maxAge * 1000 + return now + (maxAge * 1000) } if (headers.expire) { // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.3 - return Date.now() - new Date(headers.expire).getTime() + return now + (Date.now() - new Date(headers.expire).getTime()) } return undefined } /** + * @param {number} now * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives * @param {number} staleAt */ -function determineDeleteAt (cacheControlDirectives, staleAt) { +function determineDeleteAt (now, cacheControlDirectives, staleAt) { if (cacheControlDirectives['stale-while-revalidate']) { - return (cacheControlDirectives['stale-while-revalidate'] * 1000) + return now + (cacheControlDirectives['stale-while-revalidate'] * 1000) } return staleAt diff --git a/test/types/cache-interceptor.test-d.ts b/test/types/cache-interceptor.test-d.ts index c0c43f249bf..7f368679cb1 100644 --- a/test/types/cache-interceptor.test-d.ts +++ b/test/types/cache-interceptor.test-d.ts @@ -3,15 +3,13 @@ import CacheInterceptor from '../../types/cache-interceptor' import Dispatcher from '../../types/dispatcher' const store: CacheInterceptor.CacheStore = { - maxEntrySize: 0, - entryCount: 0, - maxEntries: 0, + isFull: false, - get (_: Dispatcher.RequestOptions): CacheInterceptor.CacheStoreValue | Promise { + createReadStream (_: Dispatcher.RequestOptions): CacheInterceptor.CacheStoreReadable | undefined { throw new Error('stub') }, - put (_: Dispatcher.RequestOptions, _2: CacheInterceptor.CacheStoreValue): void | Promise { + createWriteStream (_: Dispatcher.RequestOptions, _2: CacheInterceptor.CacheStoreValue): CacheInterceptor.CacheStoreWriteable | undefined { throw new Error('stub') }, @@ -26,26 +24,20 @@ expectAssignable({ methods: [] }) expectAssignable({ store, methods: ['GET'] }) expectAssignable({ - complete: true, statusCode: 200, statusMessage: 'OK', rawHeaders: [], - body: [], - size: 0, cachedAt: 0, staleAt: 0, deleteAt: 0 }) expectAssignable({ - complete: true, statusCode: 200, statusMessage: 'OK', rawHeaders: [], rawTrailers: [], - body: [], vary: {}, - size: 0, cachedAt: 0, staleAt: 0, deleteAt: 0 @@ -53,7 +45,6 @@ expectAssignable({ expectNotAssignable({}) expectNotAssignable({ - complete: '', statusCode: '123', statusMessage: 123, rawHeaders: '', From 9ef03efc9bfe97edebc6f0ddf93f3ca070fb1429 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 23 Sep 2024 22:18:56 -0700 Subject: [PATCH 24/58] empty the body when overwriting the response Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index c02fe32ae47..17ff89339c5 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -150,6 +150,9 @@ class MemoryCacheStore { } } } + } else { + // Empty it so we can overwrite it + storedValue.body = [] } const writable = new MemoryStoreWritableStream( From 58839eeb46a168d4fba6bde792a21f20074f90bf Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Tue, 24 Sep 2024 11:59:53 -0700 Subject: [PATCH 25/58] update onError calls Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/handler/cache-handler.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index f04247e624c..436c33016c6 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -67,7 +67,9 @@ class CacheHandler extends DecoratorHandler { typeof result.catch === 'function' && typeof this.#handler.onError === 'function' ) { - result.catch(this.#handler.onError) + result.catch(err => { + this.#handler.onError(err) + }) } return this.#handler.onHeaders( @@ -144,7 +146,9 @@ class CacheHandler extends DecoratorHandler { }) this.#writeStream.on('drain', resume) - this.#writeStream.on('error', this.#handler.onError) + this.#writeStream.on('error', (err) => { + this.#handler.onError(err) + }) } if (typeof this.#handler.onHeaders === 'function') { From e49a32cc152af487ddcb5d7a29f75a616d3cb729 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Tue, 24 Sep 2024 18:49:42 -0700 Subject: [PATCH 26/58] remove request deduplication for now Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 65 ++++++++++++++------------ lib/handler/cache-handler.js | 32 +++---------- test/cache-interceptor/cache-stores.js | 61 +++++++++++++++++------- test/cache-interceptor/interceptor.js | 4 -- 4 files changed, 87 insertions(+), 75 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 17ff89339c5..c680a13028a 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -1,6 +1,5 @@ 'use strict' -const EventEmitter = require('node:events') const { Writable, Readable } = require('node:stream') /** @@ -9,9 +8,11 @@ const { Writable, Readable } = require('node:stream') * * @typedef {{ * complete: boolean + * readers: number + * readLock: boolean + * writeLock: boolean * value: import('../../types/cache-interceptor.d.ts').default.CacheStoreValue * body: Buffer[] - * emitter: EventEmitter * }} MemoryStoreValue */ class MemoryCacheStore { @@ -75,7 +76,11 @@ class MemoryCacheStore { const value = this.#findValue(req, values) - return value ? new MemoryStoreReadableStream(value) : undefined + if (!value || value.readLock) { + return undefined + } + + return new MemoryStoreReadableStream(value) } createWriteStream (req, value) { @@ -107,10 +112,11 @@ class MemoryCacheStore { // TODO better name for this storedValue = { - complete: false, + readLock: false, + readers: 0, + writeLock: false, value, - body: [], - emitter: new EventEmitter() + body: [] } // We want to sort our responses in decending order by their deleteAt @@ -151,6 +157,12 @@ class MemoryCacheStore { } } } else { + // Check if there's already another request writing to the value or + // a request reading from it + if (storedValue.writeLock || storedValue.readLock) { + return undefined + } + // Empty it so we can overwrite it storedValue.body = [] } @@ -164,6 +176,8 @@ class MemoryCacheStore { writable.on('error', () => { values.filter(value => value !== storedValue) }) + + return writable } /** @@ -265,19 +279,23 @@ class MemoryStoreReadableStream extends Readable { constructor (value) { super() + if (value.readLock) { + throw new Error('can\'t read a locked value') + } + this.#value = value + this.#chunksToSend = [...this.#value.body, null] - if (value.complete) { - this.#chunksToSend = [...this.#value.body, null] - } else { - value.emitter.on('body', () => { - this.#chunksToSend = [...this.#value.body, null] - }) + this.#value.readers++ + this.#value.writeLock = true - value.emitter.on('error', () => { - this.#chunksToSend.push(null) - }) - } + this.on('close', () => { + this.#value.readers-- + + if (this.#value.readers === 0) { + this.#value.writeLock = false + } + }) } get value () { @@ -321,6 +339,7 @@ class MemoryStoreWritableStream extends Writable { constructor (value, maxEntrySize) { super() this.#value = value + this.#value.readLock = true this.#maxEntrySize = maxEntrySize } @@ -361,24 +380,12 @@ class MemoryStoreWritableStream extends Writable { _final (callback) { if (this.#currentSize < this.#maxEntrySize) { this.#value.complete = true + this.#value.readLock = false this.#value.body = this.#body - - this.#value.emitter.emit('body') } callback() } - - /** - * @param {Error} err - * @param {() => void} callback - */ - _destroy (err, callback) { - if (err) { - this.#value.emitter.emit('error', err) - } - callback(err) - } } module.exports = MemoryCacheStore diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 436c33016c6..c2b2577f3ba 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -145,10 +145,12 @@ class CacheHandler extends DecoratorHandler { deleteAt }) - this.#writeStream.on('drain', resume) - this.#writeStream.on('error', (err) => { - this.#handler.onError(err) - }) + if (this.#writeStream) { + this.#writeStream.on('drain', resume) + this.#writeStream.on('error', (err) => { + this.#handler.onError(err) + }) + } } if (typeof this.#handler.onHeaders === 'function') { @@ -216,28 +218,6 @@ class CacheHandler extends DecoratorHandler { this.#handler.onError(err) } } - - /** - * @param {string[] | Buffer[]} arr - * @returns {number} - */ - #getSizeOfBuffers (arr) { - let size = 0 - - if (arr.length > 0) { - if (typeof arr[0] === 'string') { - for (const buffer of arr) { - size += buffer.length - } - } else { - for (const buffer of arr) { - size += buffer.byteLength - } - } - } - - return size - } } /** diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index a8402e6ee55..6e20682f893 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -46,10 +46,7 @@ function cacheStoreTests (CacheStore) { equal(store.createReadStream(request), undefined) // Write the response to the store - let writeStream = store.createWriteStream(request, { - ...requestValue, - body: [] - }) + let writeStream = store.createWriteStream(request, requestValue) notEqual(writeStream, undefined) writeResponse(writeStream, requestBody, requestTrailers) @@ -125,10 +122,7 @@ function cacheStoreTests (CacheStore) { */ const store = new CacheStore() - const writeStream = store.createWriteStream(request, { - ...requestValue, - body: [] - }) + const writeStream = store.createWriteStream(request, requestValue) notEqual(writeStream, undefined) writeResponse(writeStream, requestBody, requestTrailers) @@ -162,10 +156,7 @@ function cacheStoreTests (CacheStore) { */ const store = new CacheStore() - const writeStream = store.createWriteStream(request, { - ...requestValue, - body: [] - }) + const writeStream = store.createWriteStream(request, requestValue) notEqual(writeStream, undefined) writeResponse(writeStream, requestBody, rawTrailers) @@ -203,10 +194,7 @@ function cacheStoreTests (CacheStore) { // Sanity check equal(store.createReadStream(request), undefined) - const writeStream = store.createWriteStream(request, { - ...requestValue, - body: [] - }) + const writeStream = store.createWriteStream(request, requestValue) notEqual(writeStream, undefined) writeResponse(writeStream, requestBody, requestTrailers) @@ -231,6 +219,47 @@ function cacheStoreTests (CacheStore) { }) } +test('MemoryCacheStore locks values properly', async () => { + const store = new MemoryCacheStore() + + const request = { + origin: 'localhost', + path: '/', + method: 'GET', + headers: {} + } + + const requestValue = { + statusCode: 200, + statusMessage: '', + rawHeaders: [1, 2, 3], + cachedAt: Date.now(), + staleAt: Date.now() + 10000, + deleteAt: Date.now() + 20000 + } + + const writable = store.createWriteStream(request, requestValue) + notEqual(writable, undefined) + + // Value should now be locked, we shouldn't be able to create a readable or + // another writable to it until the first one finishes + equal(store.createReadStream(request), undefined) + equal(store.createWriteStream(request, requestValue), undefined) + + // Close the writable, this should unlock it + writeResponse(writable, ['asd'], []) + + // Stream is now closed, let's lock any new write streams + const readable = store.createReadStream(request) + notEqual(readable, undefined) + equal(store.createWriteStream(request, requestValue), undefined) + + // Consume & close the readable, this should lift the write lock + await readResponse(readable) + + notEqual(store.createWriteStream(request, requestValue), undefined) +}) + /** * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreWriteable} stream * @param {string[]} body diff --git a/test/cache-interceptor/interceptor.js b/test/cache-interceptor/interceptor.js index 54d259ff360..40cad118bba 100644 --- a/test/cache-interceptor/interceptor.js +++ b/test/cache-interceptor/interceptor.js @@ -237,8 +237,4 @@ describe('Cache Interceptor', () => { } }) }) - - test('shares responses still in-flight to the same request', async () => { - // TODO - }) }) From 6469aab405bc2fabf5709b13c6242bd288cf967e Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Tue, 24 Sep 2024 18:55:22 -0700 Subject: [PATCH 27/58] rename value -> opts, storedValue -> value Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 58 +++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index c680a13028a..2ba8e6008f9 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -11,7 +11,7 @@ const { Writable, Readable } = require('node:stream') * readers: number * readLock: boolean * writeLock: boolean - * value: import('../../types/cache-interceptor.d.ts').default.CacheStoreValue + * opts: import('../../types/cache-interceptor.d.ts').default.CacheStoreValue * body: Buffer[] * }} MemoryStoreValue */ @@ -64,6 +64,10 @@ class MemoryCacheStore { return this.#entryCount >= this.#maxEntries } + /** + * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req + * @returns {import('../../types/cache-interceptor.d.ts').default.CacheStoreReadable | undefined} + */ createReadStream (req) { if (typeof req !== 'object') { throw new TypeError(`expected req to be object, got ${typeof req}`) @@ -83,12 +87,17 @@ class MemoryCacheStore { return new MemoryStoreReadableStream(value) } - createWriteStream (req, value) { + /** + * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req + * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue} opts + * @returns {import('../../types/cache-interceptor.d.ts').default.CacheStoreWriteable | undefined} + */ + createWriteStream (req, opts) { if (typeof req !== 'object') { throw new TypeError(`expected req to be object, got ${typeof req}`) } - if (typeof value !== 'object') { - throw new TypeError(`expected value to be object, got ${typeof value}`) + if (typeof opts !== 'object') { + throw new TypeError(`expected value to be object, got ${typeof opts}`) } if (this.isFull) { @@ -97,8 +106,8 @@ class MemoryCacheStore { const values = this.#getValuesForRequest(req, true) - let storedValue = this.#findValue(req, values) - if (!storedValue) { + let value = this.#findValue(req, values) + if (!value) { // The value doesn't already exist, meaning we haven't cached this // response before. Let's assign it a value and insert it into our data // property. @@ -110,12 +119,11 @@ class MemoryCacheStore { this.#entryCount++ - // TODO better name for this - storedValue = { + value = { readLock: false, readers: 0, writeLock: false, - value, + opts, body: [] } @@ -123,14 +131,14 @@ class MemoryCacheStore { // timestamps so that deleting expired responses is faster if ( values.length === 0 || - value.deleteAt < values[values.length - 1].deleteAt + opts.deleteAt < values[values.length - 1].deleteAt ) { // Our value is either the only response for this path or our deleteAt // time is sooner than all the other responses - values.push(storedValue) - } else if (value.deleteAt >= values[0].deleteAt) { + values.push(value) + } else if (opts.deleteAt >= values[0].deleteAt) { // Our deleteAt is later than everyone elses - values.unshift(storedValue) + values.unshift(value) } else { // We're neither in the front or the end, let's just binary search to // find our stop we need to be in @@ -138,16 +146,16 @@ class MemoryCacheStore { let endIndex = values.length while (true) { if (startIndex === endIndex) { - values.splice(startIndex, 0, storedValue) + values.splice(startIndex, 0, value) break } const middleIndex = Math.floor((startIndex + endIndex) / 2) const middleValue = values[middleIndex] - if (value.deleteAt === middleIndex) { - values.splice(middleIndex, 0, storedValue) + if (opts.deleteAt === middleIndex) { + values.splice(middleIndex, 0, value) break - } else if (value.deleteAt > middleValue.value.deleteAt) { + } else if (opts.deleteAt > middleValue.opts.deleteAt) { endIndex = middleIndex continue } else { @@ -159,22 +167,22 @@ class MemoryCacheStore { } else { // Check if there's already another request writing to the value or // a request reading from it - if (storedValue.writeLock || storedValue.readLock) { + if (value.writeLock || value.readLock) { return undefined } // Empty it so we can overwrite it - storedValue.body = [] + value.body = [] } const writable = new MemoryStoreWritableStream( - storedValue, + value, this.#maxEntrySize ) // Remove the value if there was some error writable.on('error', () => { - values.filter(value => value !== storedValue) + values.filter(current => value !== current) }) return writable @@ -229,7 +237,7 @@ class MemoryCacheStore { const now = Date.now() for (let i = values.length - 1; i >= 0; i--) { const current = values[i] - const currentCacheValue = current.value + const currentCacheValue = current.opts if (now >= currentCacheValue.deleteAt) { // We've reached expired values, let's delete them this.#entryCount -= values.length - i @@ -299,7 +307,7 @@ class MemoryStoreReadableStream extends Readable { } get value () { - return this.#value.value + return this.#value.opts } /** @@ -344,14 +352,14 @@ class MemoryStoreWritableStream extends Writable { } get rawTrailers () { - return this.#value.value.rawTrailers + return this.#value.opts.rawTrailers } /** * @param {Buffer[] | undefined} trailers */ set rawTrailers (trailers) { - this.#value.value.rawTrailers = trailers + this.#value.opts.rawTrailers = trailers } /** From 969deb2578b251ad4bf885ee516d98c9864ef696 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Tue, 24 Sep 2024 19:17:45 -0700 Subject: [PATCH 28/58] fix types Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- types/cache-interceptor.d.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 712136ef0d8..84dda2cb164 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -85,9 +85,9 @@ declare namespace CacheHandler { get isFull (): boolean - createReadStream (req: Dispatcher.RequestOptions): Readable | undefined + createReadStream (req: Dispatcher.RequestOptions): CacheStoreReadable | undefined - createWriteStream (req: Dispatcher.RequestOptions, value: CacheStoreValue): Writable + createWriteStream (req: Dispatcher.RequestOptions, value: CacheStoreValue): CacheStoreWriteable deleteByOrigin (origin: string): void } From 3370f665fe68c2272fb547d660af12c977b987ae Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 25 Sep 2024 16:49:51 -0700 Subject: [PATCH 29/58] Apply suggestions from code review Co-authored-by: Matteo Collina --- lib/util/cache.js | 2 ++ test/cache-interceptor/cache-stores.js | 2 ++ 2 files changed, 4 insertions(+) diff --git a/lib/util/cache.js b/lib/util/cache.js index 30bc592d787..765b1975344 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -1,3 +1,5 @@ +'use strict' + const UNSAFE_METHODS = [ 'POST', 'PUT', 'PATCH', 'DELETE' ] diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index 6e20682f893..783e0bde84f 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -1,3 +1,5 @@ +'use strict' + const { describe, test } = require('node:test') const { deepStrictEqual, notEqual, equal } = require('node:assert') const { once } = require('node:events') From 263718e1f9247f10bceb53f8a9b2063313f8f465 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 25 Sep 2024 17:51:46 -0700 Subject: [PATCH 30/58] simplify parsing for qualified no-cache and private Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/util/cache.js | 40 +++++++++++++++++---------------- test/cache-interceptor/utils.js | 24 +++++++++----------- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/lib/util/cache.js b/lib/util/cache.js index 765b1975344..0c0f63609d8 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -93,30 +93,32 @@ function parseCacheControlHeader (header) { // Add the first header on and cut off the leading quote const headers = [value.substring(1)] - let foundEndingQuote = false - for (let j = i; j < directives.length; j++) { - const nextPart = directives[j] - const nextPartLength = nextPart.length - if (nextPartLength !== 0 && nextPart[nextPartLength] === '"') { - foundEndingQuote = true - headers.push(...directives.splice(i + 1, j - 1).map(header => header.trim())) - - // Cut off ending quote - let lastHeader = header[headers.length - 1] + let foundEndingQuote = value[value.length - 1] === '"' + if (!foundEndingQuote) { + // Something like `no-cache="some-header, another-header"` + // This can still be something invalid, e.g. `no-cache="some-header, ...` + for (let j = i + 1; j < directives.length; j++) { + const nextPart = directives[j] + const nextPartLength = nextPart.length + + headers.push(nextPart.trim()) + + if (nextPartLength !== 0 && nextPart[nextPartLength - 1] === '"') { + foundEndingQuote = true + break + } + } + } + + if (foundEndingQuote) { + let lastHeader = headers[headers.length - 1] + if (lastHeader[lastHeader.length - 1] === '"') { lastHeader = lastHeader.substring(0, lastHeader.length - 1) headers[headers.length - 1] = lastHeader - - break } - } - if (foundEndingQuote === false) { - // Something like `no-cache="some-header` with no end quote, - // let's just ignore it - continue + output[key] = headers } - - output[key] = headers } else { // Something like `no-cache=some-header` output[key] = [value] diff --git a/test/cache-interceptor/utils.js b/test/cache-interceptor/utils.js index 9b4e247ddca..a676d62d16e 100644 --- a/test/cache-interceptor/utils.js +++ b/test/cache-interceptor/utils.js @@ -135,30 +135,28 @@ describe('parseCacheControlHeader', () => { describe('parseVaryHeader', () => { test('basic usage', () => { - const output = parseVaryHeader({ - vary: 'some-header, another-one', + const output = parseVaryHeader('some-header, another-one', { 'some-header': 'asd', 'another-one': '123', 'third-header': 'cool' }) - deepStrictEqual(output, new Map([ - ['some-header', 'asd'], - ['another-one', '123'] - ])) + deepStrictEqual(output, { + 'some-header': 'asd', + 'another-one': '123' + }) }) test('handles weird spacings', () => { - const output = parseVaryHeader({ - vary: 'some-header, another-one,something-else', + const output = parseVaryHeader('some-header, another-one,something-else', { 'some-header': 'asd', 'another-one': '123', 'something-else': 'asd123', 'third-header': 'cool' }) - deepStrictEqual(output, new Map([ - ['some-header', 'asd'], - ['another-one', '123'], - ['something-else', 'asd123'] - ])) + deepStrictEqual(output, { + 'some-header': 'asd', + 'another-one': '123', + 'something-else': 'asd123' + }) }) }) From b5e483a75509e0da4e42a572d8623529fe0eb40b Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 25 Sep 2024 19:24:44 -0700 Subject: [PATCH 31/58] fix header omission, some cleanup Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 15 +++++---- lib/core/util.js | 37 +++++++++++++++++++++ lib/handler/cache-handler.js | 58 ++++++++++++++++++--------------- lib/interceptor/cache.js | 4 +-- 4 files changed, 80 insertions(+), 34 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 2ba8e6008f9..f7cfe03e8f2 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -7,7 +7,6 @@ const { Writable, Readable } = require('node:stream') * @implements {CacheStore} * * @typedef {{ - * complete: boolean * readers: number * readLock: boolean * writeLock: boolean @@ -112,7 +111,7 @@ class MemoryCacheStore { // response before. Let's assign it a value and insert it into our data // property. - if (this.#entryCount >= this.#maxEntries) { + if (this.isFull) { // Or not, we don't have space to add another response return undefined } @@ -120,8 +119,8 @@ class MemoryCacheStore { this.#entryCount++ value = { - readLock: false, readers: 0, + readLock: false, writeLock: false, opts, body: [] @@ -185,6 +184,10 @@ class MemoryCacheStore { values.filter(current => value !== current) }) + writable.on('bodyOversized', () => { + values.filter(current => value !== current) + }) + return writable } @@ -315,7 +318,7 @@ class MemoryStoreReadableStream extends Readable { */ _read (size) { if (this.#chunksToSend.length === 0) { - return + throw new Error('no chunks left to read, stream should have closed') } if (size > this.#chunksToSend.length) { @@ -364,7 +367,7 @@ class MemoryStoreWritableStream extends Writable { /** * @param {Buffer} chunk - * @param {*} _ + * @param {string} encoding * @param {() => void} callback */ _write (chunk, encoding, callback) { @@ -377,6 +380,7 @@ class MemoryStoreWritableStream extends Writable { this.#body.push(chunk) } else { this.#body = null // release memory as early as possible + this.emit('bodyOversized') } callback() @@ -387,7 +391,6 @@ class MemoryStoreWritableStream extends Writable { */ _final (callback) { if (this.#currentSize < this.#maxEntrySize) { - this.#value.complete = true this.#value.readLock = false this.#value.body = this.#body } diff --git a/lib/core/util.js b/lib/core/util.js index 05dd11867d7..d2d4a02557e 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -436,6 +436,42 @@ function parseHeaders (headers, obj) { return obj } +/** + * @param {Record} headers + * @returns {Buffer[]} + */ +function encodeHeaders (headers) { + const headerNames = Object.keys(headers) + + /** + * @type {Buffer[]} + */ + const rawHeaders = new Array(headerNames.length * 2) + + let rawHeadersIndex = 0 + for (const header of headerNames) { + let rawValue + const value = headers[header] + if (Array.isArray(value)) { + rawValue = new Array(value.length) + + for (let i = 0; i < value.length; i++) { + rawValue[i] = Buffer.from(value[i]) + } + } else { + rawValue = Buffer.from(value) + } + + rawHeaders[rawHeadersIndex] = Buffer.from(header) + rawHeadersIndex++ + + rawHeaders[rawHeadersIndex] = rawValue + rawHeadersIndex++ + } + + return rawHeaders +} + /** * @param {Buffer[]} headers * @returns {string[]} @@ -864,6 +900,7 @@ module.exports = { errorRequest, parseRawHeaders, parseHeaders, + encodeHeaders, parseKeepAliveTimeout, destroy, bodyLength, diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index c2b2577f3ba..39d17030c00 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -84,7 +84,7 @@ class CacheHandler extends DecoratorHandler { const cacheControlHeader = headers['cache-control'] const contentLengthHeader = headers['content-length'] - if (!cacheControlHeader || !contentLengthHeader) { + if (!cacheControlHeader || !contentLengthHeader || this.#opts.store.isFull) { // Don't have the headers we need, can't cache return this.#handler.onHeaders( statusCode, @@ -95,12 +95,8 @@ class CacheHandler extends DecoratorHandler { ) } - const contentLength = Number(headers['content-length']) - if ( - !Number.isInteger(contentLength) || - this.#opts.store.isFull || - this.#opts.store.entryCount >= this.#opts.store.maxEntries - ) { + const contentLength = Number(contentLengthHeader) + if (!Number.isInteger(contentLength)) { return this.#handler.onHeaders( statusCode, rawHeaders, @@ -319,7 +315,7 @@ function determineDeleteAt (now, cacheControlDirectives, staleAt) { /** * Strips headers required to be removed in cached responses * @param {Buffer[]} rawHeaders - * @param {string[]} parsedHeaders + * @param {Record} parsedHeaders * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives * @returns {Buffer[]} */ @@ -334,29 +330,39 @@ function stripNecessaryHeaders (rawHeaders, parsedHeaders, cacheControlDirective headersToRemove.push(...cacheControlDirectives['private']) } - let strippedRawHeaders - for (let i = 0; i < parsedHeaders.length; i++) { - const header = parsedHeaders[i] - const kvDelimiterIndex = header.indexOf(':') - if (kvDelimiterIndex === -1) { - // We should never get here but just for safety - throw new Error('header missing kv delimiter') - } + /** + * These are the headers that are okay to cache. If this is assigned, we need + * to remake the buffer representation of the headers + * @type {Record | undefined} + */ + let strippedHeaders + + const headerNames = Object.keys(parsedHeaders) + for (let i = 0; i < headerNames.length; i++) { + const header = headerNames[i] + + if (headersToRemove.indexOf(header) !== -1) { + // We have a at least one header we want to remove + if (!strippedHeaders) { + // This is the first header we want to remove, let's create the object + // and backfill the previous headers into it + strippedHeaders = {} + + for (let j = 0; j < i; j++) { + strippedHeaders[headerNames[j]] = parsedHeaders[headerNames[j]] + } + } - const headerName = header.substring(0, kvDelimiterIndex) + continue + } - if (headerName in headersToRemove) { - if (!strippedRawHeaders) { - strippedRawHeaders = rawHeaders.slice(0, i - 1) - } else { - strippedRawHeaders.push(rawHeaders[i]) - } + // This header is fine. Let's add it to strippedHeaders if it exists. + if (strippedHeaders) { + strippedHeaders[header] = parsedHeaders[header] } } - strippedRawHeaders ??= rawHeaders - - return strippedRawHeaders + return strippedHeaders ? util.encodeHeaders(strippedHeaders) : rawHeaders } module.exports = CacheHandler diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index d55e316bd2d..cd19ece7e51 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -114,8 +114,8 @@ module.exports = globalOpts => { // Check if the response is stale const now = Date.now() - if (now > value.staleAt) { - if (now > value.deleteAt) { + if (now >= value.staleAt) { + if (now >= value.deleteAt) { // Safety check in case the store gave us a response that should've been // deleted already dispatch(opts, new CacheHandler(globalOpts, opts, handler)) From d983b9c773b7caae3a09dbb80ccaca7849961d18 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 30 Sep 2024 23:35:26 -0700 Subject: [PATCH 32/58] running the tests in ci is probably a good idea Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- package.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 12c47f46824..c03b684fbad 100644 --- a/package.json +++ b/package.json @@ -70,10 +70,11 @@ "lint:fix": "eslint --fix --cache", "test": "npm run test:javascript && cross-env NODE_V8_COVERAGE= npm run test:typescript", "test:javascript": "npm run test:javascript:no-jest && npm run test:jest", - "test:javascript:no-jest": "npm run generate-pem && npm run test:unit && npm run test:node-fetch && npm run test:cache && npm run test:interceptors && npm run test:fetch && npm run test:cookies && npm run test:eventsource && npm run test:wpt && npm run test:websocket && npm run test:node-test", + "test:javascript:no-jest": "npm run generate-pem && npm run test:unit && npm run test:node-fetch && npm run test:cache && && npm run test:cache-interceptor npm run test:interceptors && npm run test:fetch && npm run test:cookies && npm run test:eventsource && npm run test:wpt && npm run test:websocket && npm run test:node-test", "test:javascript:without-intl": "npm run test:javascript:no-jest", "test:busboy": "borp -p \"test/busboy/*.js\"", "test:cache": "borp -p \"test/cache/*.js\"", + "test:cache-interceptor": "borp -p \"test/cache-interceptor/*.js\"", "test:cookies": "borp -p \"test/cookie/*.js\"", "test:eventsource": "npm run build:node && borp --expose-gc -p \"test/eventsource/*.js\"", "test:fuzzing": "node test/fuzzing/fuzzing.test.js", From 6b1de1fb98e22f6331534a06bc9ade15e602a02e Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 30 Sep 2024 23:38:36 -0700 Subject: [PATCH 33/58] fix some testing values Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- test/cache-interceptor/cache-stores.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index 783e0bde84f..845055be124 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -31,7 +31,7 @@ function cacheStoreTests (CacheStore) { const requestValue = { statusCode: 200, statusMessage: '', - rawHeaders: [1, 2, 3], + rawHeaders: [Buffer.from('1'), Buffer.from('2'), Buffer.from('3')], cachedAt: Date.now(), staleAt: Date.now() + 10000, deleteAt: Date.now() + 20000 @@ -72,7 +72,7 @@ function cacheStoreTests (CacheStore) { const anotherValue = { statusCode: 200, statusMessage: '', - rawHeaders: [1, 2, 3], + rawHeaders: [Buffer.from('1'), Buffer.from('2'), Buffer.from('3')], cachedAt: Date.now(), staleAt: Date.now() + 10000, deleteAt: Date.now() + 20000 @@ -111,7 +111,7 @@ function cacheStoreTests (CacheStore) { const requestValue = { statusCode: 200, statusMessage: '', - rawHeaders: [1, 2, 3], + rawHeaders: [Buffer.from('1'), Buffer.from('2'), Buffer.from('3')], cachedAt: Date.now() - 10000, staleAt: Date.now() - 1, deleteAt: Date.now() + 20000 @@ -151,7 +151,7 @@ function cacheStoreTests (CacheStore) { deleteAt: Date.now() - 5 } const requestBody = ['part1', 'part2'] - const rawTrailers = [4, 5, 6] + const rawTrailers = ['4', '5', '6'] /** * @type {import('../../types/cache-interceptor.d.ts').default.CacheStore} @@ -177,7 +177,7 @@ function cacheStoreTests (CacheStore) { const requestValue = { statusCode: 200, statusMessage: '', - rawHeaders: [1, 2, 3], + rawHeaders: [Buffer.from('1'), Buffer.from('2'), Buffer.from('3')], vary: { 'some-header': 'hello world' }, @@ -186,7 +186,7 @@ function cacheStoreTests (CacheStore) { deleteAt: Date.now() + 20000 } const requestBody = ['part1', 'part2'] - const requestTrailers = [4, 5, 6] + const requestTrailers = ['4', '5', '6'] /** * @type {import('../../types/cache-interceptor.d.ts').default.CacheStore} @@ -234,7 +234,7 @@ test('MemoryCacheStore locks values properly', async () => { const requestValue = { statusCode: 200, statusMessage: '', - rawHeaders: [1, 2, 3], + rawHeaders: [Buffer.from('1'), Buffer.from('2'), Buffer.from('3')], cachedAt: Date.now(), staleAt: Date.now() + 10000, deleteAt: Date.now() + 20000 From 7e482e9f391a34edc288fc97378a9bfd350725a8 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 30 Sep 2024 23:51:52 -0700 Subject: [PATCH 34/58] fixup! running the tests in ci is probably a good idea Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index c03b684fbad..74c638fecf4 100644 --- a/package.json +++ b/package.json @@ -70,7 +70,7 @@ "lint:fix": "eslint --fix --cache", "test": "npm run test:javascript && cross-env NODE_V8_COVERAGE= npm run test:typescript", "test:javascript": "npm run test:javascript:no-jest && npm run test:jest", - "test:javascript:no-jest": "npm run generate-pem && npm run test:unit && npm run test:node-fetch && npm run test:cache && && npm run test:cache-interceptor npm run test:interceptors && npm run test:fetch && npm run test:cookies && npm run test:eventsource && npm run test:wpt && npm run test:websocket && npm run test:node-test", + "test:javascript:no-jest": "npm run generate-pem && npm run test:unit && npm run test:node-fetch && npm run test:cache && npm run test:cache-interceptor && npm run test:interceptors && npm run test:fetch && npm run test:cookies && npm run test:eventsource && npm run test:wpt && npm run test:websocket && npm run test:node-test", "test:javascript:without-intl": "npm run test:javascript:no-jest", "test:busboy": "borp -p \"test/busboy/*.js\"", "test:cache": "borp -p \"test/cache/*.js\"", From e433ec6c6145f46dc8c237bb2e44f923a6f915a3 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Tue, 1 Oct 2024 12:34:24 -0700 Subject: [PATCH 35/58] Update lib/interceptor/cache.js Co-authored-by: Robert Nagy --- lib/interceptor/cache.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index cd19ece7e51..dea1cbba342 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -93,8 +93,8 @@ module.exports = globalOpts => { } else { if (typeof handler.onData === 'function') { stream.on('data', chunk => { - while (!handler.onData(chunk)) { - signal.throwIfAborted() + if (!handler.onData(chunk)) { + stream.pause() } }) } From d8c73683e73628e716ec7e17508e8868cdc51f63 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Tue, 1 Oct 2024 20:31:40 -0700 Subject: [PATCH 36/58] Update lib/util/cache.js Co-authored-by: Aras Abbasi --- lib/util/cache.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/util/cache.js b/lib/util/cache.js index 0c0f63609d8..e75c93e28d7 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -1,8 +1,8 @@ 'use strict' -const UNSAFE_METHODS = [ +const UNSAFE_METHODS = /** @type {const} */ ([ 'POST', 'PUT', 'PATCH', 'DELETE' -] +]) /** * @see https://www.rfc-editor.org/rfc/rfc9111.html#name-cache-control From 0a7a99ed6772d244a6347b8dc90cec57c56077a3 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Tue, 1 Oct 2024 22:17:45 -0700 Subject: [PATCH 37/58] update from reviews Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/core/constants.js | 10 +++++++++- lib/core/util.js | 9 +++++++-- lib/util/cache.js | 6 ++++++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/lib/core/constants.js b/lib/core/constants.js index 000c0194909..ba681135a59 100644 --- a/lib/core/constants.js +++ b/lib/core/constants.js @@ -104,6 +104,11 @@ const wellknownHeaderNames = /** @type {const} */ ([ /** @type {Record, string>} */ const headerNameLowerCasedRecord = {} +/** + * @type {Record, Buffer>} + */ +const wellknownHeaderNameBuffers = {} + // Note: object prototypes should not be able to be referenced. e.g. `Object#hasOwnProperty`. Object.setPrototypeOf(headerNameLowerCasedRecord, null) @@ -112,9 +117,12 @@ for (let i = 0; i < wellknownHeaderNames.length; ++i) { const lowerCasedKey = key.toLowerCase() headerNameLowerCasedRecord[key] = headerNameLowerCasedRecord[lowerCasedKey] = lowerCasedKey + + wellknownHeaderNameBuffers[lowerCasedKey] = Buffer.from(lowerCasedKey) } module.exports = { wellknownHeaderNames, - headerNameLowerCasedRecord + headerNameLowerCasedRecord, + wellknownHeaderNameBuffers } diff --git a/lib/core/util.js b/lib/core/util.js index d2d4a02557e..8790cd69d68 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -10,7 +10,7 @@ const nodeUtil = require('node:util') const { stringify } = require('node:querystring') const { EventEmitter: EE } = require('node:events') const { InvalidArgumentError } = require('./errors') -const { headerNameLowerCasedRecord } = require('./constants') +const { headerNameLowerCasedRecord, wellknownHeaderNameBuffers } = require('./constants') const { tree } = require('./tree') const [nodeMajor, nodeMinor] = process.versions.node.split('.').map(v => Number(v)) @@ -462,7 +462,12 @@ function encodeHeaders (headers) { rawValue = Buffer.from(value) } - rawHeaders[rawHeadersIndex] = Buffer.from(header) + let headerBuffer = wellknownHeaderNameBuffers[header] + if (!headerBuffer) { + headerBuffer = Buffer.from(header) + } + + rawHeaders[rawHeadersIndex] = headerBuffer rawHeadersIndex++ rawHeaders[rawHeadersIndex] = rawValue diff --git a/lib/util/cache.js b/lib/util/cache.js index e75c93e28d7..7e5f31d5cf6 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -137,6 +137,8 @@ function parseCacheControlHeader (header) { case 'must-understand': case 'only-if-cached': if (value) { + // These are qualified (something like `public=...`) when they aren't + // allowed to be, skip continue } @@ -157,6 +159,10 @@ function parseCacheControlHeader (header) { * @returns {Record} */ function parseVaryHeader (varyHeader, headers) { + if (varyHeader === '*') { + return headers + } + const output = /** @type {Record} */ ({}) const varyingHeaders = varyHeader.toLowerCase().split(',') From 43efed2a6bab5d75e2a4d9dbece7495dd5ed977c Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 2 Oct 2024 15:27:43 -0700 Subject: [PATCH 38/58] Update lib/interceptor/cache.js Co-authored-by: Aras Abbasi --- lib/interceptor/cache.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index dea1cbba342..06477298ae9 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -94,7 +94,7 @@ module.exports = globalOpts => { if (typeof handler.onData === 'function') { stream.on('data', chunk => { if (!handler.onData(chunk)) { - stream.pause() + stream.pause() } }) } From e76597616039cf216ad8d580bc82ebbab931e3a2 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 2 Oct 2024 15:35:55 -0700 Subject: [PATCH 39/58] Apply suggestions from code review Co-authored-by: Robert Nagy --- lib/interceptor/cache.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 06477298ae9..dd4f258bc5f 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -61,7 +61,7 @@ module.exports = globalOpts => { const { value } = stream // Dump body on error - if (typeof opts.body === 'object' && opts.body.constructor.name === 'Readable') { + if (util.isStream(opts.body)) { opts.body?.on('error', () => {}).resume() } From 019f9b47662eca87339dbd5215f4cfb6ff684de8 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 2 Oct 2024 16:16:59 -0700 Subject: [PATCH 40/58] change from reviews Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/handler/cache-handler.js | 52 +++++++++++------------------------- lib/interceptor/cache.js | 10 +++++++ 2 files changed, 25 insertions(+), 37 deletions(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 39d17030c00..79be2994448 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -55,6 +55,14 @@ class CacheHandler extends DecoratorHandler { statusMessage, headers = util.parseHeaders(rawHeaders) ) { + const downstreamOnHeaders = () => this.#handler.onHeaders( + statusCode, + rawHeaders, + resume, + statusMessage, + headers + ) + if ( this.#req.method in UNSAFE_METHODS && statusCode >= 200 && @@ -68,17 +76,11 @@ class CacheHandler extends DecoratorHandler { typeof this.#handler.onError === 'function' ) { result.catch(err => { - this.#handler.onError(err) + process.emitWarning(`deleteByOrigin failed: ${err}`) }) } - return this.#handler.onHeaders( - statusCode, - rawHeaders, - resume, - statusMessage, - headers - ) + return downstreamOnHeaders() } const cacheControlHeader = headers['cache-control'] @@ -86,35 +88,17 @@ class CacheHandler extends DecoratorHandler { if (!cacheControlHeader || !contentLengthHeader || this.#opts.store.isFull) { // Don't have the headers we need, can't cache - return this.#handler.onHeaders( - statusCode, - rawHeaders, - resume, - statusMessage, - headers - ) + return downstreamOnHeaders() } const contentLength = Number(contentLengthHeader) if (!Number.isInteger(contentLength)) { - return this.#handler.onHeaders( - statusCode, - rawHeaders, - resume, - statusMessage, - headers - ) + return downstreamOnHeaders() } const cacheControlDirectives = parseCacheControlHeader(cacheControlHeader) if (!canCacheResponse(statusCode, headers, cacheControlDirectives)) { - return this.#handler.onHeaders( - statusCode, - rawHeaders, - resume, - statusMessage, - headers - ) + return downstreamOnHeaders() } const now = Date.now() @@ -144,19 +128,13 @@ class CacheHandler extends DecoratorHandler { if (this.#writeStream) { this.#writeStream.on('drain', resume) this.#writeStream.on('error', (err) => { - this.#handler.onError(err) + process.emitWarning(`cache write stream failed: ${err}`) }) } } if (typeof this.#handler.onHeaders === 'function') { - return this.#handler.onHeaders( - statusCode, - rawHeaders, - resume, - statusMessage, - headers - ) + return downstreamOnHeaders() } } diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index dd4f258bc5f..16be277d175 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -69,6 +69,15 @@ module.exports = globalOpts => { const ac = new AbortController() const signal = ac.signal + signal.onabort = (_, err) => { + stream.destroy() + handler.onError(err) + } + + stream.on('error', (err) => { + handler.onError(err) + }) + try { if (typeof handler.onConnect === 'function') { handler.onConnect(ac.abort) @@ -89,6 +98,7 @@ module.exports = globalOpts => { if (opts.method === 'HEAD') { if (typeof handler.onComplete === 'function') { handler.onComplete(null) + stream.destroy() } } else { if (typeof handler.onData === 'function') { From 1db72bb1ef72623b2f7f4d892dd47aee30f5dabe Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 2 Oct 2024 16:28:30 -0700 Subject: [PATCH 41/58] add promise support back for createReadStream Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/interceptor/cache.js | 82 +++++++++++++++++++++++------------- types/cache-interceptor.d.ts | 4 +- 2 files changed, 54 insertions(+), 32 deletions(-) diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 16be277d175..c2b54467005 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -1,5 +1,6 @@ 'use strict' +const util = require('../core/util') const CacheHandler = require('../handler/cache-handler') const MemoryCacheStore = require('../cache/memory-cache-store') const CacheRevalidationHandler = require('../handler/cache-revalidation-handler') @@ -58,14 +59,11 @@ module.exports = globalOpts => { return dispatch(opts, new CacheHandler(globalOpts, opts, handler)) } - const { value } = stream - - // Dump body on error - if (util.isStream(opts.body)) { - opts.body?.on('error', () => {}).resume() - } - - const respondWithCachedValue = () => { + /** + * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreReadable | undefined} stream + * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue} value + */ + const respondWithCachedValue = (stream, value) => { const ac = new AbortController() const signal = ac.signal @@ -85,8 +83,8 @@ module.exports = globalOpts => { } if (typeof handler.onHeaders === 'function') { - // Add the age header - // https://www.rfc-editor.org/rfc/rfc9111.html#name-age + // Add the age header + // https://www.rfc-editor.org/rfc/rfc9111.html#name-age const age = Math.round((Date.now() - value.cachedAt) / 1000) value.rawHeaders.push(AGE_HEADER, Buffer.from(`${age}`)) @@ -122,35 +120,59 @@ module.exports = globalOpts => { } } - // Check if the response is stale - const now = Date.now() - if (now >= value.staleAt) { - if (now >= value.deleteAt) { - // Safety check in case the store gave us a response that should've been - // deleted already - dispatch(opts, new CacheHandler(globalOpts, opts, handler)) - return + /** + * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreReadable | undefined} stream + */ + const handleStream = (stream) => { + if (!stream) { + // Request isn't cached + return dispatch(opts, new CacheHandler(globalOpts, opts, handler)) } - if (!opts.headers) { - opts.headers = {} + const { value } = stream + + // Dump body on error + if (util.isStream(opts.body)) { + opts.body?.on('error', () => {}).resume() } - opts.headers['if-modified-since'] = new Date(value.cachedAt).toUTCString() + // Check if the response is stale + const now = Date.now() + if (now >= value.staleAt) { + if (now >= value.deleteAt) { + // Safety check in case the store gave us a response that should've been + // deleted already + dispatch(opts, new CacheHandler(globalOpts, opts, handler)) + return + } + + if (!opts.headers) { + opts.headers = {} + } + + opts.headers['if-modified-since'] = new Date(value.cachedAt).toUTCString() - // Need to revalidate the response - dispatch( - opts, - new CacheRevalidationHandler( - respondWithCachedValue, - new CacheHandler(globalOpts, opts, handler) + // Need to revalidate the response + dispatch( + opts, + new CacheRevalidationHandler( + () => respondWithCachedValue(stream, value), + new CacheHandler(globalOpts, opts, handler) + ) ) - ) - return + return + } + + respondWithCachedValue(stream, value) } - respondWithCachedValue() + if (stream.constructor.name === 'Promise') { + stream.then(handleStream) + stream.catch(handler.onError) + } else { + handleStream(stream) + } return true } diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 84dda2cb164..89671fa644a 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -26,7 +26,7 @@ declare namespace CacheHandler { */ get isFull(): boolean - createReadStream(req: Dispatcher.RequestOptions): CacheStoreReadable | undefined + createReadStream(req: Dispatcher.RequestOptions): CacheStoreReadable | Promise | undefined createWriteStream(req: Dispatcher.RequestOptions, value: CacheStoreValue): CacheStoreWriteable | undefined @@ -37,7 +37,7 @@ declare namespace CacheHandler { } export interface CacheStoreReadable extends Readable { - get value(): Omit + get value(): CacheStoreValue } export interface CacheStoreWriteable extends Writable { From ab72d14977c19ba71fcd947b61f7de0504937317 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Fri, 4 Oct 2024 19:22:03 -0700 Subject: [PATCH 42/58] Apply suggestions from code review Co-authored-by: Robert Nagy --- lib/handler/cache-handler.js | 2 ++ lib/interceptor/cache.js | 8 ++------ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 79be2994448..1bf648efd09 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -129,6 +129,8 @@ class CacheHandler extends DecoratorHandler { this.#writeStream.on('drain', resume) this.#writeStream.on('error', (err) => { process.emitWarning(`cache write stream failed: ${err}`) + this.#writeStream = null + resume() }) } } diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index c2b54467005..e2f50e3086f 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -114,6 +114,7 @@ module.exports = globalOpts => { } } } catch (err) { + stream.destroy(err) if (typeof handler.onError === 'function') { handler.onError(err) } @@ -167,12 +168,7 @@ module.exports = globalOpts => { respondWithCachedValue(stream, value) } - if (stream.constructor.name === 'Promise') { - stream.then(handleStream) - stream.catch(handler.onError) - } else { - handleStream(stream) - } + Promise.resolve(stream).then(handleStream).catch(err => handler.onError(err)) return true } From 917b530532e2da0bbf09e2a60d40230ea39f083d Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Fri, 4 Oct 2024 19:29:47 -0700 Subject: [PATCH 43/58] check if onError was called Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/interceptor/cache.js | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index e2f50e3086f..6b07e71a4fe 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -59,6 +59,8 @@ module.exports = globalOpts => { return dispatch(opts, new CacheHandler(globalOpts, opts, handler)) } + let onErrorCalled = false + /** * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreReadable | undefined} stream * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue} value @@ -69,11 +71,17 @@ module.exports = globalOpts => { signal.onabort = (_, err) => { stream.destroy() - handler.onError(err) + if (!onErrorCalled) { + handler.onError(err) + onErrorCalled = true + } } stream.on('error', (err) => { - handler.onError(err) + if (!onErrorCalled) { + handler.onError(err) + onErrorCalled = true + } }) try { @@ -83,8 +91,8 @@ module.exports = globalOpts => { } if (typeof handler.onHeaders === 'function') { - // Add the age header - // https://www.rfc-editor.org/rfc/rfc9111.html#name-age + // Add the age header + // https://www.rfc-editor.org/rfc/rfc9111.html#name-age const age = Math.round((Date.now() - value.cachedAt) / 1000) value.rawHeaders.push(AGE_HEADER, Buffer.from(`${age}`)) @@ -115,8 +123,9 @@ module.exports = globalOpts => { } } catch (err) { stream.destroy(err) - if (typeof handler.onError === 'function') { + if (!onErrorCalled && typeof handler.onError === 'function') { handler.onError(err) + onErrorCalled = true } } } @@ -141,8 +150,8 @@ module.exports = globalOpts => { const now = Date.now() if (now >= value.staleAt) { if (now >= value.deleteAt) { - // Safety check in case the store gave us a response that should've been - // deleted already + // Safety check in case the store gave us a response that should've been + // deleted already dispatch(opts, new CacheHandler(globalOpts, opts, handler)) return } From 2adae0994e9f2821c52cb84e777f6678c7965fc3 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 7 Oct 2024 17:22:28 -0700 Subject: [PATCH 44/58] add docs Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- docs/docs/api/CacheStore.md | 116 +++++++++++++++++++++++++++++++++++ docs/docs/api/Dispatcher.md | 10 +++ types/cache-interceptor.d.ts | 6 +- 3 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 docs/docs/api/CacheStore.md diff --git a/docs/docs/api/CacheStore.md b/docs/docs/api/CacheStore.md new file mode 100644 index 00000000000..f4dcbb927c1 --- /dev/null +++ b/docs/docs/api/CacheStore.md @@ -0,0 +1,116 @@ +# Cache Store + +A Cache Store is responsible for storing and retrieving cached responses. +It is also responsible for deciding which specific response to use based off of +a response's `Vary` header (if present). + +## Pre-built Cache Stores + +### `MemoryCacheStore` + +The `MemoryCacheStore` stores the responses in-memory. + +**Options** + +- `maxEntries` - The maximum amount of responses to store. Default `Infinity`. +- `maxEntrySize` - The maximum size in bytes that a response's body can be. If a response's body is greater than or equal to this, the response will not be cached. + +## Defining a Custom Cache Store + +The store must implement the following functions: + +### Getter: `isFull` + +This tells the cache interceptor if the store is full or not. If this is true, +the cache interceptor will not attempt to cache the response. + +### Function: `createReadStream` + +Parameters: + +* **req** `Dispatcher.RequestOptions` - Incoming request + +Returns: `CacheStoreReadable | Promise | undefined` - If the request is cached, a readable for the body is returned. Otherwise, `undefined` is returned. + +### Function: `createWriteStream` + +Parameters: + +* **req** `Dispatcher.RequestOptions` - Incoming request +* **value** `CacheStoreValue` - Response to store + +Returns: `CacheStoreWriteable | undefined` - If the store is full, return `undefined`. Otherwise, return a writable so that the cache interceptor can stream the body and trailers to the store. + +## `CacheStoreValue` + +This is an interface containing the majority of a response's data (minus the body). + +### Property `statusCode` + +`number` - The response's HTTP status code. + +### Property `statusMessage` + +`string` - The response's HTTP status message. + +### Property `rawHeaders` + +`(Buffer | Buffer[])[]` - The response's headers. + +### Property `rawTrailers` + +`string[] | undefined` - The response's trailers. + +### Property `vary` + +`Record | undefined` - The headers defined by the response's `Vary` header +and their respective values for later comparison + +For example, for a response like +``` +Vary: content-encoding, accepts +content-encoding: utf8 +accepts: application/json +``` + +This would be +```js +{ + 'content-encoding': 'utf8', + accepts: 'application/json' +} +``` + +### Property `cachedAt` + +`number` - Time in millis that this value was cached. + +### Property `staleAt` + +`number` - Time in millis that this value is considered stale. + +### Property `deleteAt` + +`number` - Time in millis that this value is to be deleted from the cache. This +is either the same sa staleAt or the `max-stale` caching directive. + +The store must not return a response after the time defined in this property. + +## `CacheStoreReadable` + +This extends Node's [`Readable`](https://nodejs.org/api/stream.html#class-streamreadable) +and defines extra properties relevant to the cache interceptor. + +### Getter: `value` + +The response's [`CacheStoreValue`](#cachestorevalue) + +## `CacheStoreWriteable` + +This extends Node's [`Writable`](https://nodejs.org/api/stream.html#class-streamwritable) +and defines extra properties relevant to the cache interceptor. + +### Setter: `rawTrailers` + +If the response has trailers, the cache interceptor will pass them to the cache +interceptor through this method. diff --git a/docs/docs/api/Dispatcher.md b/docs/docs/api/Dispatcher.md index 3a0dfb8e400..3903eca3d3c 100644 --- a/docs/docs/api/Dispatcher.md +++ b/docs/docs/api/Dispatcher.md @@ -1233,6 +1233,16 @@ test('should not error if request status code is not in the specified error code The Response Error Interceptor provides a robust mechanism for handling HTTP response errors by capturing detailed error information and propagating it through a structured `ResponseError` class. This enhancement improves error handling and debugging capabilities in applications using the interceptor. +##### `Cache Interceptor` + +The `cache` interceptor implements client-side response caching as described in +[RFC9111](https://www.rfc-editor.org/rfc/rfc9111.html). + +**Options** + +- `store` - The [`CacheStore`](./CacheStore.md) to store and retrieve responses from. Default is [`MemoryCacheStore`](./CacheStore.md#memorycachestore). +- `methods` - The [**safe** HTTP methods](https://www.rfc-editor.org/rfc/rfc9110#section-9.2.1) to cache the response of. + ## Instance Events ### Event: `'connect'` diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 89671fa644a..ac61d87f059 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -28,7 +28,7 @@ declare namespace CacheHandler { createReadStream(req: Dispatcher.RequestOptions): CacheStoreReadable | Promise | undefined - createWriteStream(req: Dispatcher.RequestOptions, value: CacheStoreValue): CacheStoreWriteable | undefined + createWriteStream(req: Dispatcher.RequestOptions, value: Omit): CacheStoreWriteable | undefined /** * Delete all of the cached responses from a certain origin (host) @@ -41,13 +41,13 @@ declare namespace CacheHandler { } export interface CacheStoreWriteable extends Writable { - set rawTrailers(rawTrailers: Buffer[] | undefined) + set rawTrailers(rawTrailers: string[] | undefined) } export interface CacheStoreValue { statusCode: number; statusMessage: string; - rawHeaders: Buffer[]; + rawHeaders: (Buffer | Buffer[])[]; rawTrailers?: string[]; /** * Headers defined by the Vary header and their respective values for From eda939d0ccab4f65d1e79b10fc038d1f91a85897 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Tue, 8 Oct 2024 23:55:04 -0700 Subject: [PATCH 45/58] add errorCallback Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 17 ++- lib/handler/cache-handler.js | 25 ++-- test/interceptors/cache.js | 240 ++++++++++++++++++++++++++++++++ types/cache-interceptor.d.ts | 1 + 4 files changed, 271 insertions(+), 12 deletions(-) create mode 100644 test/interceptors/cache.js diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index f7cfe03e8f2..64f96176f9f 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -19,6 +19,11 @@ class MemoryCacheStore { #maxEntrySize = Infinity + /** + * @type {((err) => void) | undefined} + */ + #errorCallback = undefined + #entryCount = 0 /** @@ -56,6 +61,13 @@ class MemoryCacheStore { } this.#maxEntrySize = opts.maxEntrySize } + + if (opts.errorCallback !== undefined) { + if (typeof opts.errorCallback !== 'function') { + throw new TypeError('MemoryCacheStore options.errorCallback must be a function') + } + this.#errorCallback = opts.errorCallback + } } } @@ -180,8 +192,11 @@ class MemoryCacheStore { ) // Remove the value if there was some error - writable.on('error', () => { + writable.on('error', (err) => { values.filter(current => value !== current) + if (this.#errorCallback) { + this.#errorCallback(err) + } }) writable.on('bodyOversized', () => { diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 1bf648efd09..ead13e341a6 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -69,15 +69,19 @@ class CacheHandler extends DecoratorHandler { statusCode <= 399 ) { // https://www.rfc-editor.org/rfc/rfc9111.html#name-invalidating-stored-respons - const result = this.#opts.store.deleteByOrigin(this.#req.origin) - if ( - result && - typeof result.catch === 'function' && - typeof this.#handler.onError === 'function' - ) { - result.catch(err => { - process.emitWarning(`deleteByOrigin failed: ${err}`) - }) + // Try/catch for if it's synchronous + try { + const result = this.#opts.store.deleteByOrigin(this.#req.origin) + if ( + result && + typeof result.catch === 'function' && + typeof this.#handler.onError === 'function' + ) { + // Fail silently + result.catch(_ => {}) + } + } catch (err) { + // Fail silently } return downstreamOnHeaders() @@ -127,8 +131,7 @@ class CacheHandler extends DecoratorHandler { if (this.#writeStream) { this.#writeStream.on('drain', resume) - this.#writeStream.on('error', (err) => { - process.emitWarning(`cache write stream failed: ${err}`) + this.#writeStream.on('error', () => { this.#writeStream = null resume() }) diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js new file mode 100644 index 00000000000..40cad118bba --- /dev/null +++ b/test/interceptors/cache.js @@ -0,0 +1,240 @@ +'use strict' + +const { describe, test, after } = require('node:test') +const { strictEqual, notEqual, fail } = require('node:assert') +const { createServer } = require('node:http') +const { once } = require('node:events') +const { Client, interceptors, cacheStores } = require('../../index') + +describe('Cache Interceptor', () => { + test('doesn\'t cache request w/ no cache-control header', async () => { + let requestsToOrigin = 0 + + const server = createServer((_, res) => { + requestsToOrigin++ + res.end('asd') + }).listen(0) + + after(() => server.close()) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + strictEqual(requestsToOrigin, 0) + + // Send initial request. This should reach the origin + let response = await client.request({ + origin: 'localhost', + method: 'GET', + path: '/' + }) + strictEqual(requestsToOrigin, 1) + strictEqual(await response.body.text(), 'asd') + + // Send second request that should be handled by cache + response = await client.request({ + origin: 'localhost', + method: 'GET', + path: '/' + }) + strictEqual(requestsToOrigin, 2) + strictEqual(await response.body.text(), 'asd') + }) + + test('caches request successfully', async () => { + let requestsToOrigin = 0 + + const server = createServer((_, res) => { + requestsToOrigin++ + res.setHeader('cache-control', 'public, s-maxage=10') + res.end('asd') + }).listen(0) + + after(() => server.close()) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + strictEqual(requestsToOrigin, 0) + + // Send initial request. This should reach the origin + let response = await client.request({ + origin: 'localhost', + method: 'GET', + path: '/' + }) + strictEqual(requestsToOrigin, 1) + strictEqual(await response.body.text(), 'asd') + + // Send second request that should be handled by cache + response = await client.request({ + origin: 'localhost', + method: 'GET', + path: '/' + }) + strictEqual(requestsToOrigin, 1) + strictEqual(await response.body.text(), 'asd') + strictEqual(response.headers.age, '0') + }) + + test('respects vary header', async () => { + let requestsToOrigin = 0 + + const server = createServer((req, res) => { + requestsToOrigin++ + res.setHeader('cache-control', 'public, s-maxage=10') + res.setHeader('vary', 'some-header, another-header') + + if (req.headers['some-header'] === 'abc123') { + res.end('asd') + } else { + res.end('dsa') + } + }).listen(0) + + after(() => server.close()) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + strictEqual(requestsToOrigin, 0) + + // Send initial request. This should reach the origin + let response = await client.request({ + origin: 'localhost', + method: 'GET', + path: '/', + headers: { + 'some-header': 'abc123', + 'another-header': '123abc' + } + }) + strictEqual(requestsToOrigin, 1) + strictEqual(await response.body.text(), 'asd') + + // Make another request with changed headers, this should miss + const secondResponse = await client.request({ + method: 'GET', + path: '/', + headers: { + 'some-header': 'qwerty', + 'another-header': 'asdfg' + } + }) + strictEqual(requestsToOrigin, 2) + strictEqual(await secondResponse.body.text(), 'dsa') + + // Resend the first request again which should still be cahced + response = await client.request({ + origin: 'localhost', + method: 'GET', + path: '/', + headers: { + 'some-header': 'abc123', + 'another-header': '123abc' + } + }) + strictEqual(requestsToOrigin, 2) + strictEqual(await response.body.text(), 'asd') + }) + + test('revalidates request when needed', async () => { + let requestsToOrigin = 0 + + const server = createServer((req, res) => { + res.setHeader('cache-control', 'public, s-maxage=1, stale-while-revalidate=10') + + requestsToOrigin++ + + if (requestsToOrigin > 1) { + notEqual(req.headers['if-modified-since'], undefined) + + if (requestsToOrigin === 3) { + res.end('asd123') + } else { + res.statusCode = 304 + res.end() + } + } else { + res.end('asd') + } + }).listen(0) + + after(() => server.close()) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + strictEqual(requestsToOrigin, 0) + + const request = { + origin: 'localhost', + method: 'GET', + path: '/' + } + + // Send initial request. This should reach the origin + let response = await client.request(request) + strictEqual(requestsToOrigin, 1) + strictEqual(await response.body.text(), 'asd') + + // Now we send two more requests. Both of these should reach the origin, + // but now with a conditional header asking if the resource has been + // updated. These need to be ran after the response is stale. + const completed = new Promise((resolve, reject) => { + setTimeout(async () => { + try { + // No update for the second request + response = await client.request(request) + strictEqual(requestsToOrigin, 2) + strictEqual(await response.body.text(), 'asd') + + // This should be updated, even though the value isn't expired. + response = await client.request(request) + strictEqual(requestsToOrigin, 3) + strictEqual(await response.body.text(), 'asd123') + + resolve() + } catch (e) { + reject(e) + } + }, 1500) + }) + await completed + }) + + test('respects cache store\'s isFull property', async () => { + const server = createServer((_, res) => { + res.end('asd') + }).listen(0) + + after(() => server.close()) + await once(server, 'listening') + + const store = new cacheStores.MemoryCacheStore() + Object.defineProperty(store, 'isFull', { + value: true + }) + + store.createWriteStream = (...args) => { + fail('shouln\'t have reached this') + } + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache({ store })) + + await client.request({ + origin: 'localhost', + method: 'GET', + path: '/', + headers: { + 'some-header': 'abc123', + 'another-header': '123abc' + } + }) + }) +}) diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index ac61d87f059..d36e2077581 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -78,6 +78,7 @@ declare namespace CacheHandler { * @default Infinity */ maxEntrySize?: number + errorCallback?: (err: unknown) => void } export class MemoryCacheStore implements CacheStore { From a43112abd63d2ee069d0aa441b8341fac939ac16 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 9 Oct 2024 09:10:39 -0700 Subject: [PATCH 46/58] Update types/cache-interceptor.d.ts Co-authored-by: Carlos Fuentes --- types/cache-interceptor.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index d36e2077581..45af1a8a5f5 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -78,7 +78,7 @@ declare namespace CacheHandler { * @default Infinity */ maxEntrySize?: number - errorCallback?: (err: unknown) => void + errorCallback?: (err: Error) => void } export class MemoryCacheStore implements CacheStore { From 684d044b517162304533793c42a34aeac1003455 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Wed, 9 Oct 2024 23:56:06 -0700 Subject: [PATCH 47/58] use fake timers and cleanup client Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- test/interceptors/cache.js | 77 ++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 32 deletions(-) diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js index 40cad118bba..302fc734b4b 100644 --- a/test/interceptors/cache.js +++ b/test/interceptors/cache.js @@ -4,6 +4,7 @@ const { describe, test, after } = require('node:test') const { strictEqual, notEqual, fail } = require('node:assert') const { createServer } = require('node:http') const { once } = require('node:events') +const FakeTimers = require('@sinonjs/fake-timers') const { Client, interceptors, cacheStores } = require('../../index') describe('Cache Interceptor', () => { @@ -15,12 +16,16 @@ describe('Cache Interceptor', () => { res.end('asd') }).listen(0) - after(() => server.close()) - await once(server, 'listening') - const client = new Client(`http://localhost:${server.address().port}`) .compose(interceptors.cache()) + after(async () => { + server.close() + await client.close() + }) + + await once(server, 'listening') + strictEqual(requestsToOrigin, 0) // Send initial request. This should reach the origin @@ -51,12 +56,16 @@ describe('Cache Interceptor', () => { res.end('asd') }).listen(0) - after(() => server.close()) - await once(server, 'listening') - const client = new Client(`http://localhost:${server.address().port}`) .compose(interceptors.cache()) + after(async () => { + server.close() + await client.close() + }) + + await once(server, 'listening') + strictEqual(requestsToOrigin, 0) // Send initial request. This should reach the origin @@ -94,12 +103,16 @@ describe('Cache Interceptor', () => { } }).listen(0) - after(() => server.close()) - await once(server, 'listening') - const client = new Client(`http://localhost:${server.address().port}`) .compose(interceptors.cache()) + after(async () => { + server.close() + await client.close() + }) + + await once(server, 'listening') + strictEqual(requestsToOrigin, 0) // Send initial request. This should reach the origin @@ -144,6 +157,10 @@ describe('Cache Interceptor', () => { test('revalidates request when needed', async () => { let requestsToOrigin = 0 + const clock = FakeTimers.install({ + shouldClearNativeTimers: true + }) + const server = createServer((req, res) => { res.setHeader('cache-control', 'public, s-maxage=1, stale-while-revalidate=10') @@ -163,12 +180,17 @@ describe('Cache Interceptor', () => { } }).listen(0) - after(() => server.close()) - await once(server, 'listening') - const client = new Client(`http://localhost:${server.address().port}`) .compose(interceptors.cache()) + after(async () => { + server.close() + await client.close() + clock.uninstall() + }) + + await once(server, 'listening') + strictEqual(requestsToOrigin, 0) const request = { @@ -182,29 +204,20 @@ describe('Cache Interceptor', () => { strictEqual(requestsToOrigin, 1) strictEqual(await response.body.text(), 'asd') + clock.tick(1500) + // Now we send two more requests. Both of these should reach the origin, // but now with a conditional header asking if the resource has been // updated. These need to be ran after the response is stale. - const completed = new Promise((resolve, reject) => { - setTimeout(async () => { - try { - // No update for the second request - response = await client.request(request) - strictEqual(requestsToOrigin, 2) - strictEqual(await response.body.text(), 'asd') - - // This should be updated, even though the value isn't expired. - response = await client.request(request) - strictEqual(requestsToOrigin, 3) - strictEqual(await response.body.text(), 'asd123') - - resolve() - } catch (e) { - reject(e) - } - }, 1500) - }) - await completed + // No update for the second request + response = await client.request(request) + strictEqual(requestsToOrigin, 2) + strictEqual(await response.body.text(), 'asd') + + // This should be updated, even though the value isn't expired. + response = await client.request(request) + strictEqual(requestsToOrigin, 3) + strictEqual(await response.body.text(), 'asd123') }) test('respects cache store\'s isFull property', async () => { From 12c2ded95711241d01c5bba17f6463bfbe08b619 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Fri, 11 Oct 2024 17:11:11 -0700 Subject: [PATCH 48/58] lazy cache wellknown headers Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/core/constants.js | 23 ++++++++++++++++++----- lib/core/util.js | 11 +++++------ 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/lib/core/constants.js b/lib/core/constants.js index ba681135a59..e450fc6087e 100644 --- a/lib/core/constants.js +++ b/lib/core/constants.js @@ -104,25 +104,38 @@ const wellknownHeaderNames = /** @type {const} */ ([ /** @type {Record, string>} */ const headerNameLowerCasedRecord = {} +// Note: object prototypes should not be able to be referenced. e.g. `Object#hasOwnProperty`. +Object.setPrototypeOf(headerNameLowerCasedRecord, null) + /** * @type {Record, Buffer>} */ const wellknownHeaderNameBuffers = {} -// Note: object prototypes should not be able to be referenced. e.g. `Object#hasOwnProperty`. -Object.setPrototypeOf(headerNameLowerCasedRecord, null) +/** + * @param {string} header Lowercased header + * @returns {Buffer} + */ +function getWellknownHeaderNameBuffer (header) { + let buffer = wellknownHeaderNameBuffers[header] + + if (buffer === undefined) { + buffer = Buffer.from(header) + } + + return buffer +} for (let i = 0; i < wellknownHeaderNames.length; ++i) { const key = wellknownHeaderNames[i] const lowerCasedKey = key.toLowerCase() headerNameLowerCasedRecord[key] = headerNameLowerCasedRecord[lowerCasedKey] = lowerCasedKey - - wellknownHeaderNameBuffers[lowerCasedKey] = Buffer.from(lowerCasedKey) } module.exports = { wellknownHeaderNames, headerNameLowerCasedRecord, - wellknownHeaderNameBuffers + wellknownHeaderNameBuffers, + getWellknownHeaderNameBuffer } diff --git a/lib/core/util.js b/lib/core/util.js index 8790cd69d68..8f02116924c 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -10,7 +10,7 @@ const nodeUtil = require('node:util') const { stringify } = require('node:querystring') const { EventEmitter: EE } = require('node:events') const { InvalidArgumentError } = require('./errors') -const { headerNameLowerCasedRecord, wellknownHeaderNameBuffers } = require('./constants') +const { headerNameLowerCasedRecord, getWellknownHeaderNameBuffer } = require('./constants') const { tree } = require('./tree') const [nodeMajor, nodeMinor] = process.versions.node.split('.').map(v => Number(v)) @@ -438,7 +438,7 @@ function parseHeaders (headers, obj) { /** * @param {Record} headers - * @returns {Buffer[]} + * @returns {(Buffer | Buffer[])[]} */ function encodeHeaders (headers) { const headerNames = Object.keys(headers) @@ -462,10 +462,9 @@ function encodeHeaders (headers) { rawValue = Buffer.from(value) } - let headerBuffer = wellknownHeaderNameBuffers[header] - if (!headerBuffer) { - headerBuffer = Buffer.from(header) - } + const headerBuffer = header in headerNameLowerCasedRecord + ? getWellknownHeaderNameBuffer(header) + : Buffer.from(header) rawHeaders[rawHeadersIndex] = headerBuffer rawHeadersIndex++ From 29b13624ea52721d594b0b3d36a9185423a30b2f Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Sun, 13 Oct 2024 23:19:11 -0700 Subject: [PATCH 49/58] Apply suggestions from code review Co-authored-by: Aras Abbasi --- lib/core/util.js | 6 ++---- lib/handler/cache-handler.js | 6 +++--- lib/handler/cache-revalidation-handler.js | 4 +++- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/core/util.js b/lib/core/util.js index 8f02116924c..f8d6b38c430 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -445,7 +445,7 @@ function encodeHeaders (headers) { /** * @type {Buffer[]} - */ + * @type {Buffer[]|Buffer[][]} const rawHeaders = new Array(headerNames.length * 2) let rawHeadersIndex = 0 @@ -462,9 +462,7 @@ function encodeHeaders (headers) { rawValue = Buffer.from(value) } - const headerBuffer = header in headerNameLowerCasedRecord - ? getWellknownHeaderNameBuffer(header) - : Buffer.from(header) + const headerBuffer = getWellknownHeaderNameBuffer(header) rawHeaders[rawHeadersIndex] = headerBuffer rawHeadersIndex++ diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index ead13e341a6..db8fb3993cf 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -10,7 +10,7 @@ const { parseCacheControlHeader, parseVaryHeader, UNSAFE_METHODS } = require('.. class CacheHandler extends DecoratorHandler { /** * @type {import('../../types/cache-interceptor.d.ts').default.CacheOptions} | null - */ + * @type {import('../../types/cache-interceptor.d.ts').default.CacheOptions | null} #opts = null /** * @type {import('../../types/dispatcher.d.ts').default.RequestOptions | null} @@ -64,7 +64,7 @@ class CacheHandler extends DecoratorHandler { ) if ( - this.#req.method in UNSAFE_METHODS && + UNSAFE_METHODS.includes(this.#req.method) && statusCode >= 200 && statusCode <= 399 ) { @@ -301,7 +301,7 @@ function determineDeleteAt (now, cacheControlDirectives, staleAt) { * @param {Record} parsedHeaders * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives * @returns {Buffer[]} - */ + * @returns {(Buffer|Buffer[])[]} function stripNecessaryHeaders (rawHeaders, parsedHeaders, cacheControlDirectives) { const headersToRemove = ['connection'] diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index f262a925543..9519026d32a 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -89,8 +89,10 @@ class CacheRevalidationHandler extends DecoratorHandler { } if (typeof this.#handler.onData === 'function') { - this.#handler.onData(chunk) + return this.#handler.onData(chunk) } + + return false } /** From 9e4f15bc7ebed71983f337900cd1fc247970a6f9 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Sun, 13 Oct 2024 23:20:06 -0700 Subject: [PATCH 50/58] Update lib/cache/memory-cache-store.js Co-authored-by: Aras Abbasi --- lib/cache/memory-cache-store.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 64f96176f9f..58723487c9d 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -33,7 +33,7 @@ class MemoryCacheStore { /** * @param {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts | undefined} opts - */ + * @param {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts | undefined} [opts] constructor (opts) { if (opts) { if (typeof opts !== 'object') { From a5a3cf39507920f83c34bc5e17eb86713d1ff481 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 14 Oct 2024 00:19:29 -0700 Subject: [PATCH 51/58] code review Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 4 ++-- lib/core/util.js | 2 +- lib/handler/cache-handler.js | 22 ++++++++++++++++++---- lib/interceptor/cache.js | 14 +++----------- lib/util/cache.js | 22 +++++++++++++++++++++- 5 files changed, 45 insertions(+), 19 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 58723487c9d..364833f21d7 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -32,8 +32,8 @@ class MemoryCacheStore { #data = new Map() /** - * @param {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts | undefined} opts * @param {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts | undefined} [opts] + */ constructor (opts) { if (opts) { if (typeof opts !== 'object') { @@ -359,7 +359,7 @@ class MemoryStoreWritableStream extends Writable { #body = [] /** - * @param {MemoryCacheStore} value + * @param {MemoryStoreValue} value * @param {number} maxEntrySize */ constructor (value, maxEntrySize) { diff --git a/lib/core/util.js b/lib/core/util.js index f8d6b38c430..07a198f1fab 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -444,8 +444,8 @@ function encodeHeaders (headers) { const headerNames = Object.keys(headers) /** - * @type {Buffer[]} * @type {Buffer[]|Buffer[][]} + */ const rawHeaders = new Array(headerNames.length * 2) let rawHeadersIndex = 0 diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index db8fb3993cf..91b2d65ef5e 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -2,15 +2,15 @@ const util = require('../core/util') const DecoratorHandler = require('../handler/decorator-handler') -const { parseCacheControlHeader, parseVaryHeader, UNSAFE_METHODS } = require('../util/cache') +const { parseCacheControlHeader, parseVaryHeader, UNSAFE_METHODS, assertCacheStoreType } = require('../util/cache') /** * Writes a response to a CacheStore and then passes it on to the next handler */ class CacheHandler extends DecoratorHandler { /** - * @type {import('../../types/cache-interceptor.d.ts').default.CacheOptions} | null * @type {import('../../types/cache-interceptor.d.ts').default.CacheOptions | null} + */ #opts = null /** * @type {import('../../types/dispatcher.d.ts').default.RequestOptions | null} @@ -33,6 +33,20 @@ class CacheHandler extends DecoratorHandler { constructor (opts, req, handler) { super(handler) + if (typeof opts !== 'object') { + throw new TypeError(`expected opts to be an object, got type ${typeof opts}`) + } + + assertCacheStoreType(opts.store) + + if (typeof req !== 'object') { + throw new TypeError(`expected req to be an object, got type ${typeof opts}`) + } + + if (typeof handler !== 'object') { + throw new TypeError(`expected handler to be an object, got type ${typeof opts}`) + } + this.#opts = opts this.#req = req this.#handler = handler @@ -132,7 +146,7 @@ class CacheHandler extends DecoratorHandler { if (this.#writeStream) { this.#writeStream.on('drain', resume) this.#writeStream.on('error', () => { - this.#writeStream = null + this.#writeStream = undefined resume() }) } @@ -300,8 +314,8 @@ function determineDeleteAt (now, cacheControlDirectives, staleAt) { * @param {Buffer[]} rawHeaders * @param {Record} parsedHeaders * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives - * @returns {Buffer[]} * @returns {(Buffer|Buffer[])[]} + */ function stripNecessaryHeaders (rawHeaders, parsedHeaders, cacheControlDirectives) { const headersToRemove = ['connection'] diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 6b07e71a4fe..e726ec2ba47 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -4,7 +4,7 @@ const util = require('../core/util') const CacheHandler = require('../handler/cache-handler') const MemoryCacheStore = require('../cache/memory-cache-store') const CacheRevalidationHandler = require('../handler/cache-revalidation-handler') -const { UNSAFE_METHODS } = require('../util/cache.js') +const { UNSAFE_METHODS, assertCacheStoreType } = require('../util/cache.js') const AGE_HEADER = Buffer.from('age') @@ -18,15 +18,7 @@ module.exports = globalOpts => { } if (globalOpts.store) { - for (const fn of ['createReadStream', 'createWriteStream', 'deleteByOrigin']) { - if (typeof globalOpts.store[fn] !== 'function') { - throw new TypeError(`CacheStore needs a \`${fn}()\` function`) - } - } - - if (typeof globalOpts.store.isFull !== 'boolean') { - throw new TypeError(`CacheStore needs a isFull getter with type boolean, current type: ${typeof globalOpts.store.isFull}`) - } + assertCacheStoreType(globalOpts.store) } else { globalOpts.store = new MemoryCacheStore() } @@ -62,7 +54,7 @@ module.exports = globalOpts => { let onErrorCalled = false /** - * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreReadable | undefined} stream + * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreReadable} stream * @param {import('../../types/cache-interceptor.d.ts').default.CacheStoreValue} value */ const respondWithCachedValue = (stream, value) => { diff --git a/lib/util/cache.js b/lib/util/cache.js index 7e5f31d5cf6..f134ef2c37a 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -177,8 +177,28 @@ function parseVaryHeader (varyHeader, headers) { return output } +/** + * @param {unknown} store + */ +function assertCacheStoreType (store) { + if (typeof store !== 'object') { + throw new TypeError(`expected type to be an store, got ${typeof object}`) + } + + for (const fn of ['createReadStream', 'createWriteStream', 'deleteByOrigin']) { + if (typeof store[fn] !== 'function') { + throw new TypeError(`CacheStore needs a \`${fn}()\` function`) + } + } + + if (typeof store.isFull !== 'boolean') { + throw new TypeError(`CacheStore needs a isFull getter with type boolean, current type: ${typeof store.isFull}`) + } +} + module.exports = { UNSAFE_METHODS, parseCacheControlHeader, - parseVaryHeader + parseVaryHeader, + assertCacheStoreType } From 8bf98645f2032e27bc77601e1880ce3cf3a70876 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 14 Oct 2024 00:22:22 -0700 Subject: [PATCH 52/58] Apply suggestions from code review Co-authored-by: Aras Abbasi --- lib/cache/memory-cache-store.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 364833f21d7..fd9927ba169 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -217,7 +217,7 @@ class MemoryCacheStore { * Gets all of the requests of the same origin, path, and method. Does not * take the `vary` property into account. * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req - * @returns {MemoryStoreValue[] | undefined} + * @param {boolean} [makeIfDoesntExist=false] */ #getValuesForRequest (req, makeIfDoesntExist) { // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 @@ -310,7 +310,7 @@ class MemoryStoreReadableStream extends Readable { } this.#value = value - this.#chunksToSend = [...this.#value.body, null] + this.#chunksToSend = value?.body ? [...value.body, null] : [null] this.#value.readers++ this.#value.writeLock = true @@ -354,7 +354,7 @@ class MemoryStoreWritableStream extends Writable { #currentSize = 0 #maxEntrySize = 0 /** - * @type {Buffer[]} + * @type {Buffer[]|null} */ #body = [] @@ -383,7 +383,7 @@ class MemoryStoreWritableStream extends Writable { /** * @param {Buffer} chunk * @param {string} encoding - * @param {() => void} callback + * @param {BufferEncoding} encoding */ _write (chunk, encoding, callback) { if (typeof chunk === 'string') { From baaa91499e13c1d0228d8a224bacc3da4cb3bf84 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 14 Oct 2024 00:31:07 -0700 Subject: [PATCH 53/58] Apply suggestions from code review Co-authored-by: Aras Abbasi --- lib/handler/cache-revalidation-handler.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index 9519026d32a..cb335d5bb31 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -19,13 +19,13 @@ const DecoratorHandler = require('../handler/decorator-handler') class CacheRevalidationHandler extends DecoratorHandler { #successful = false /** - * @type {(() => void)|null} + * @type {(() => void)} */ - #successCallback = null + #successCallback /** - * @type {(import('../../types/dispatcher.d.ts').default.DispatchHandlers)|null} + * @type {(import('../../types/dispatcher.d.ts').default.DispatchHandlers)} */ - #handler = null + #handler /** * @param {() => void} successCallback Function to call if the cached value is valid From 8bf6b4258b2f557dd9d1df840d42c546f52ae9d9 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 14 Oct 2024 00:30:34 -0700 Subject: [PATCH 54/58] code review pt2 Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 4 ++-- lib/core/constants.js | 8 +++++--- lib/core/util.js | 4 ++-- lib/handler/cache-handler.js | 9 ++++----- lib/handler/cache-revalidation-handler.js | 7 ++----- 5 files changed, 15 insertions(+), 17 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index fd9927ba169..ae6d6614d82 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -366,7 +366,7 @@ class MemoryStoreWritableStream extends Writable { super() this.#value = value this.#value.readLock = true - this.#maxEntrySize = maxEntrySize + this.#maxEntrySize = maxEntrySize ?? Infinity } get rawTrailers () { @@ -374,7 +374,7 @@ class MemoryStoreWritableStream extends Writable { } /** - * @param {Buffer[] | undefined} trailers + * @param {string[] | undefined} trailers */ set rawTrailers (trailers) { this.#value.opts.rawTrailers = trailers diff --git a/lib/core/constants.js b/lib/core/constants.js index e450fc6087e..088cf47d80f 100644 --- a/lib/core/constants.js +++ b/lib/core/constants.js @@ -112,11 +112,14 @@ Object.setPrototypeOf(headerNameLowerCasedRecord, null) */ const wellknownHeaderNameBuffers = {} +// Note: object prototypes should not be able to be referenced. e.g. `Object#hasOwnProperty`. +Object.setPrototypeOf(wellknownHeaderNameBuffers, null) + /** * @param {string} header Lowercased header * @returns {Buffer} */ -function getWellknownHeaderNameBuffer (header) { +function getHeaderNameAsBuffer (header) { let buffer = wellknownHeaderNameBuffers[header] if (buffer === undefined) { @@ -136,6 +139,5 @@ for (let i = 0; i < wellknownHeaderNames.length; ++i) { module.exports = { wellknownHeaderNames, headerNameLowerCasedRecord, - wellknownHeaderNameBuffers, - getWellknownHeaderNameBuffer + getHeaderNameAsBuffer } diff --git a/lib/core/util.js b/lib/core/util.js index 07a198f1fab..c37f213349b 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -10,7 +10,7 @@ const nodeUtil = require('node:util') const { stringify } = require('node:querystring') const { EventEmitter: EE } = require('node:events') const { InvalidArgumentError } = require('./errors') -const { headerNameLowerCasedRecord, getWellknownHeaderNameBuffer } = require('./constants') +const { headerNameLowerCasedRecord, getHeaderNameAsBuffer } = require('./constants') const { tree } = require('./tree') const [nodeMajor, nodeMinor] = process.versions.node.split('.').map(v => Number(v)) @@ -462,7 +462,7 @@ function encodeHeaders (headers) { rawValue = Buffer.from(value) } - const headerBuffer = getWellknownHeaderNameBuffer(header) + const headerBuffer = getHeaderNameAsBuffer(header) rawHeaders[rawHeadersIndex] = headerBuffer rawHeadersIndex++ diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 91b2d65ef5e..ddf8a1171bc 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -59,22 +59,19 @@ class CacheHandler extends DecoratorHandler { * @param {Buffer[]} rawHeaders * @param {() => void} resume * @param {string} statusMessage - * @param {Record | undefined} headers * @returns {boolean} */ onHeaders ( statusCode, rawHeaders, resume, - statusMessage, - headers = util.parseHeaders(rawHeaders) + statusMessage ) { const downstreamOnHeaders = () => this.#handler.onHeaders( statusCode, rawHeaders, resume, - statusMessage, - headers + statusMessage ) if ( @@ -101,6 +98,8 @@ class CacheHandler extends DecoratorHandler { return downstreamOnHeaders() } + const headers = util.parseHeaders(rawHeaders) + const cacheControlHeader = headers['cache-control'] const contentLengthHeader = headers['content-length'] diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index cb335d5bb31..4be61477a51 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -49,15 +49,13 @@ class CacheRevalidationHandler extends DecoratorHandler { * @param {Buffer[]} rawHeaders * @param {() => void} resume * @param {string} statusMessage - * @param {string[] | undefined} headers * @returns {boolean} */ onHeaders ( statusCode, rawHeaders, resume, - statusMessage, - headers = undefined + statusMessage ) { // https://www.rfc-editor.org/rfc/rfc9111.html#name-handling-a-validation-respo if (statusCode === 304) { @@ -71,8 +69,7 @@ class CacheRevalidationHandler extends DecoratorHandler { statusCode, rawHeaders, resume, - statusMessage, - headers + statusMessage ) } } From 05109b105b279678d0be48adba287041f0bf5a85 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 14 Oct 2024 16:18:57 +0200 Subject: [PATCH 55/58] Update lib/handler/cache-revalidation-handler.js Co-authored-by: Aras Abbasi --- lib/handler/cache-revalidation-handler.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index 4be61477a51..d5293cdb166 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -72,6 +72,7 @@ class CacheRevalidationHandler extends DecoratorHandler { statusMessage ) } + return false } /** From 092d9d4821a9562f9d73618eaed06ca483e7f3d1 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 14 Oct 2024 16:20:57 +0200 Subject: [PATCH 56/58] Update lib/handler/cache-handler.js Co-authored-by: Aras Abbasi --- lib/handler/cache-handler.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index ddf8a1171bc..2eaf0fa2417 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -154,6 +154,7 @@ class CacheHandler extends DecoratorHandler { if (typeof this.#handler.onHeaders === 'function') { return downstreamOnHeaders() } + return false } /** From 1b9147583718be86c04ddf7e3df85e1bd0a9c8ca Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Mon, 14 Oct 2024 16:32:02 +0200 Subject: [PATCH 57/58] Apply suggestions from code review --- lib/util/cache.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/util/cache.js b/lib/util/cache.js index f134ef2c37a..22dea206965 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -179,10 +179,10 @@ function parseVaryHeader (varyHeader, headers) { /** * @param {unknown} store - */ + * @returns {asserts store is import('../../types/cache-interceptor.d.ts').default.CacheStore} function assertCacheStoreType (store) { - if (typeof store !== 'object') { - throw new TypeError(`expected type to be an store, got ${typeof object}`) + if (typeof store !== 'object' || store === null) { + throw new TypeError(`expected type to be an store, got ${typeof store}`) } for (const fn of ['createReadStream', 'createWriteStream', 'deleteByOrigin']) { From e6aeda9676305afd6c28857e268b1fc59ed14a11 Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Mon, 14 Oct 2024 16:34:54 +0200 Subject: [PATCH 58/58] fix --- lib/util/cache.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/util/cache.js b/lib/util/cache.js index 22dea206965..17ba5a31dae 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -180,6 +180,7 @@ function parseVaryHeader (varyHeader, headers) { /** * @param {unknown} store * @returns {asserts store is import('../../types/cache-interceptor.d.ts').default.CacheStore} + */ function assertCacheStoreType (store) { if (typeof store !== 'object' || store === null) { throw new TypeError(`expected type to be an store, got ${typeof store}`)