From e46c54cadf207ea74456e76e57e6a12876221c43 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 13 Nov 2024 21:24:59 +0100 Subject: [PATCH 01/13] fix: memory store Simplify and fix memory leak --- lib/cache/memory-cache-store.js | 123 +++++++------------------------- 1 file changed, 24 insertions(+), 99 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 17ec428cb6d..b90987f85a4 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -103,11 +103,7 @@ class MemoryCacheStore { const values = this.#getValuesForRequest(key, true) - /** - * @type {(MemoryStoreValue & { index: number }) | undefined} - */ let value = this.#findValue(key, values) - let valueIndex = value?.index 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 @@ -118,53 +114,12 @@ class MemoryCacheStore { return undefined } - this.#entryCount++ - - value = { - locked: true, - opts + if (this.#entryCount++ > this.#maxEntries) { + this.#prune() } - // We want to sort our responses in decending order by their deleteAt - // timestamps so that deleting expired responses is faster - if ( - values.length === 0 || - 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(value) - valueIndex = values.length - 1 - } else if (opts.deleteAt >= values[0].deleteAt) { - // Our deleteAt is later than everyone elses - values.unshift(value) - valueIndex = 0 - } 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) { - if (startIndex === endIndex) { - values.splice(startIndex, 0, value) - break - } - - const middleIndex = Math.floor((startIndex + endIndex) / 2) - const middleValue = values[middleIndex] - if (opts.deleteAt === middleIndex) { - values.splice(middleIndex, 0, value) - valueIndex = middleIndex - break - } else if (opts.deleteAt > middleValue.opts.deleteAt) { - endIndex = middleIndex - continue - } else { - startIndex = middleIndex - continue - } - } - } + value = { locked: true, opts } + values.push(value) } else { // Check if there's already another request writing to the value or // a request reading from it @@ -180,7 +135,7 @@ class MemoryCacheStore { /** * @type {Buffer[] | null} */ - let body = key.method !== 'HEAD' ? [] : null + let body = [] const maxEntrySize = this.#maxEntrySize const writable = new Writable({ @@ -202,7 +157,6 @@ class MemoryCacheStore { if (currentSize >= maxEntrySize) { body = null this.end() - shiftAtIndex(values, valueIndex) return callback() } @@ -263,63 +217,34 @@ class MemoryCacheStore { * to respond with. * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} req * @param {MemoryStoreValue[]} values - * @returns {(MemoryStoreValue & { index: number }) | undefined} + * @returns {(MemoryStoreValue) | undefined} */ #findValue (req, values) { - /** - * @type {MemoryStoreValue | undefined} - */ - let value const now = Date.now() - for (let i = values.length - 1; i >= 0; i--) { - const current = values[i] - const currentCacheValue = current.opts - 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 - } + return values.find(({ opts: { deleteAt, vary }, body }) => ( + body != null && + deleteAt > now && + (!vary || Object.keys(vary).every(key => vary[key] === req.headers?.[key])) + )) + } - for (const key in currentCacheValue.vary) { - if (currentCacheValue.vary[key] !== req.headers[key]) { - matches = false - break + #prune () { + const now = Date.now() + for (const [key, cachedPaths] of this.#data) { + for (const [method, prev] of cachedPaths) { + const next = prev.filter(({ opts, body }) => body == null || opts.deleteAt > now) + if (next.length === 0) { + cachedPaths.delete(method) + if (cachedPaths.size === 0) { + this.#data.delete(key) } + } else if (next.length !== prev.length) { + this.#entryCount -= prev.length - next.length + cachedPaths.set(method, next) } } - - if (matches) { - value = { - ...current, - index: i - } - break - } } - - return value } } -/** - * @param {any[]} array Array to modify - * @param {number} idx Index to delete - */ -function shiftAtIndex (array, idx) { - for (let i = idx + 1; idx < array.length; i++) { - array[i - 1] = array[i] - } - - array.length-- -} - module.exports = MemoryCacheStore From cb4fa88df8221ea02d5c999b4721a17681e14101 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 10:49:06 +0100 Subject: [PATCH 02/13] fixup --- lib/cache/memory-cache-store.js | 162 +++++++++++--------------------- 1 file changed, 57 insertions(+), 105 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index b90987f85a4..ba02ee1c187 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -3,14 +3,11 @@ const { Writable } = require('node:stream') /** + * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheKey} CacheKey * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore + * @typedef {import('../../types/cache-interceptor.d.ts').default.CachedResponse} CachedResponse + * @typedef {import('../../types/cache-interceptor.d.ts').default.GetResult} GetResult * @implements {CacheStore} - * - * @typedef {{ - * locked: boolean - * opts: import('../../types/cache-interceptor.d.ts').default.CachedResponse - * body?: Buffer[] - * }} MemoryStoreValue */ class MemoryCacheStore { #maxCount = Infinity @@ -20,7 +17,7 @@ class MemoryCacheStore { #entryCount = 0 /** - * @type {Map>} + * @type {Map>} */ #data = new Map() @@ -66,22 +63,8 @@ class MemoryCacheStore { * @returns {import('../../types/cache-interceptor.d.ts').default.GetResult | undefined} */ get (key) { - if (typeof key !== 'object') { - throw new TypeError(`expected key to be object, got ${typeof key}`) - } - - const values = this.#getValuesForRequest(key, false) - if (!values) { - return undefined - } - - const value = this.#findValue(key, values) - - if (!value || value.locked) { - return undefined - } - - return { ...value.opts, body: value.body } + const values = this.#getValuesForRequest(key) + return findValue(key, values) } /** @@ -98,86 +81,52 @@ class MemoryCacheStore { } if (this.isFull) { - return undefined + this.#prune() } - const values = this.#getValuesForRequest(key, true) - - let value = this.#findValue(key, 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. - - if (this.isFull) { - // Or not, we don't have space to add another response - return undefined - } - - if (this.#entryCount++ > this.#maxEntries) { - this.#prune() - } - - value = { locked: true, opts } - values.push(value) - } else { - // Check if there's already another request writing to the value or - // a request reading from it - if (value.locked) { - return undefined - } - - // Empty it so we can overwrite it - value.body = [] + if (this.isFull) { + return undefined } let currentSize = 0 - /** - * @type {Buffer[] | null} - */ - let body = [] - const maxEntrySize = this.#maxEntrySize - const writable = new Writable({ - write (chunk, encoding, callback) { - if (key.method === 'HEAD') { - throw new Error('HEAD request shouldn\'t have a body') - } - - if (!body) { - return callback() - } + const store = this + const body = [] + return new Writable({ + write (chunk, encoding, callback) { if (typeof chunk === 'string') { chunk = Buffer.from(chunk, encoding) } currentSize += chunk.byteLength - if (currentSize >= maxEntrySize) { - body = null - this.end() - return callback() + if (currentSize >= store.#maxEntrySize) { + this.destroy() + } else { + body.push(chunk) } - body.push(chunk) - callback() + callback(null) }, final (callback) { - value.locked = false - if (body !== null) { - value.body = body + const values = store.#getValuesForRequest(key) + + let value = findValue(key, values) + if (!value) { + value = { ...opts, body } + values.push(value) + } else { + Object.assign(value, opts, { body }) } - callback() + callback(null) } }) - - return writable } /** - * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} key + * @param {CacheKey} key */ delete (key) { this.#data.delete(`${key.origin}:${key.path}`) @@ -186,25 +135,24 @@ 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/cache-interceptor.d.ts').default.CacheKey} key - * @param {boolean} [makeIfDoesntExist=false] - * @returns {MemoryStoreValue[] | undefined} + * @param {CacheKey} key + * @returns {GetResult[]} */ - #getValuesForRequest (key, makeIfDoesntExist) { + #getValuesForRequest (key) { + if (typeof key !== 'object') { + throw new TypeError(`expected key to be object, got ${typeof key}`) + } + // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 const topLevelKey = `${key.origin}:${key.path}` let cachedPaths = this.#data.get(topLevelKey) if (!cachedPaths) { - if (!makeIfDoesntExist) { - return undefined - } - cachedPaths = new Map() this.#data.set(topLevelKey, cachedPaths) } let value = cachedPaths.get(key.method) - if (!value && makeIfDoesntExist) { + if (!value) { value = [] cachedPaths.set(key.method, value) } @@ -212,27 +160,11 @@ class MemoryCacheStore { return value } - /** - * Given a list of values of a certain request, this decides the best value - * to respond with. - * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} req - * @param {MemoryStoreValue[]} values - * @returns {(MemoryStoreValue) | undefined} - */ - #findValue (req, values) { - const now = Date.now() - return values.find(({ opts: { deleteAt, vary }, body }) => ( - body != null && - deleteAt > now && - (!vary || Object.keys(vary).every(key => vary[key] === req.headers?.[key])) - )) - } - #prune () { const now = Date.now() for (const [key, cachedPaths] of this.#data) { for (const [method, prev] of cachedPaths) { - const next = prev.filter(({ opts, body }) => body == null || opts.deleteAt > now) + const next = prev.filter(({ deleteAt }) => deleteAt > now) if (next.length === 0) { cachedPaths.delete(method) if (cachedPaths.size === 0) { @@ -247,4 +179,24 @@ class MemoryCacheStore { } } +/** + * Given a list of values of a certain request, this decides the best value + * to respond with. + * @param {CacheKey} key + * @param {GetResult[] | undefined } values + * @returns {(GetResult) | undefined} + */ +function findValue (key, values) { + if (typeof key !== 'object') { + throw new TypeError(`expected key to be object, got ${typeof key}`) + } + + const now = Date.now() + return values?.find(({ deleteAt, vary, body }) => ( + body != null && + deleteAt > now && + (!vary || Object.keys(vary).every(headerName => vary[headerName] === key.headers?.[headerName])) + )) +} + module.exports = MemoryCacheStore From 4e1620551c121f281fa21f40a5b52694510f4880 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 11:33:14 +0100 Subject: [PATCH 03/13] fixup --- lib/cache/memory-cache-store.js | 33 ++++++++++++--------- test/cache-interceptor/cache-stores.js | 40 -------------------------- 2 files changed, 20 insertions(+), 53 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index ba02ee1c187..6d36a98cbc2 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -7,19 +7,20 @@ const { Writable } = require('node:stream') * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore * @typedef {import('../../types/cache-interceptor.d.ts').default.CachedResponse} CachedResponse * @typedef {import('../../types/cache-interceptor.d.ts').default.GetResult} GetResult + */ + +/** * @implements {CacheStore} */ class MemoryCacheStore { #maxCount = Infinity - #maxEntrySize = Infinity - #entryCount = 0 - /** * @type {Map>} */ - #data = new Map() + #map = new Map() + #arr = [] /** * @param {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts | undefined} [opts] @@ -55,7 +56,7 @@ class MemoryCacheStore { } get isFull () { - return this.#entryCount >= this.#maxCount + return this.#arr.length >= this.#maxCount } /** @@ -115,6 +116,7 @@ class MemoryCacheStore { let value = findValue(key, values) if (!value) { value = { ...opts, body } + store.#arr.push(value) values.push(value) } else { Object.assign(value, opts, { body }) @@ -129,7 +131,7 @@ class MemoryCacheStore { * @param {CacheKey} key */ delete (key) { - this.#data.delete(`${key.origin}:${key.path}`) + this.#map.delete(`${key.origin}:${key.path}`) } /** @@ -145,10 +147,10 @@ class MemoryCacheStore { // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 const topLevelKey = `${key.origin}:${key.path}` - let cachedPaths = this.#data.get(topLevelKey) + let cachedPaths = this.#map.get(topLevelKey) if (!cachedPaths) { cachedPaths = new Map() - this.#data.set(topLevelKey, cachedPaths) + this.#map.set(topLevelKey, cachedPaths) } let value = cachedPaths.get(key.method) @@ -161,17 +163,22 @@ class MemoryCacheStore { } #prune () { - const now = Date.now() - for (const [key, cachedPaths] of this.#data) { + // TODO (perf): This could be implemented more efficiently... + + const count = Math.max(0, this.#arr.length - this.#maxCount / 2) + for (const value of this.#arr.splice(0, count)) { + value.body = null + } + + for (const [key, cachedPaths] of this.#map) { for (const [method, prev] of cachedPaths) { - const next = prev.filter(({ deleteAt }) => deleteAt > now) + const next = prev.filter(({ body }) => body == null) if (next.length === 0) { cachedPaths.delete(method) if (cachedPaths.size === 0) { - this.#data.delete(key) + this.#map.delete(key) } } else if (next.length !== prev.length) { - this.#entryCount -= prev.length - next.length cachedPaths.set(method, next) } } diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index 3fd4d3cc0f3..129d353b473 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -213,46 +213,6 @@ 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: [Buffer.from('1'), Buffer.from('2'), Buffer.from('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.get(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 result = store.get(request) - notEqual(result, undefined) - - // Consume & close the result, this should lift the write lock - await readResponse(result) - - notEqual(store.createWriteStream(request, requestValue), undefined) -}) - /** * @param {import('node:stream').Writable} stream * @param {string[]} body From d5b0cc7c9184e4fd23c5f5d1b87f001a305c2872 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 13:23:01 +0100 Subject: [PATCH 04/13] fixup --- lib/cache/memory-cache-store.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 6d36a98cbc2..0ba4f42034c 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -147,19 +147,20 @@ class MemoryCacheStore { // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 const topLevelKey = `${key.origin}:${key.path}` + let cachedPaths = this.#map.get(topLevelKey) if (!cachedPaths) { cachedPaths = new Map() this.#map.set(topLevelKey, cachedPaths) } - let value = cachedPaths.get(key.method) - if (!value) { - value = [] - cachedPaths.set(key.method, value) + let values = cachedPaths.get(key.method) + if (!values) { + values = [] + cachedPaths.set(key.method, values) } - return value + return values } #prune () { From ab590842b8d5cbc0f86f4a8b233362aad2ba8e9b Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 13:26:00 +0100 Subject: [PATCH 05/13] fixup --- lib/cache/memory-cache-store.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 0ba4f42034c..a7a379a11bf 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -94,6 +94,10 @@ class MemoryCacheStore { const store = this const body = [] + // TODO (fix): Make sure user can't modify... + // key = structuredClone(key) + // opts = structuredClone(opts) + return new Writable({ write (chunk, encoding, callback) { if (typeof chunk === 'string') { From b114ce4877ae40ff826f38c8f22a9844160233c6 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 13:30:29 +0100 Subject: [PATCH 06/13] fixup --- lib/cache/memory-cache-store.js | 27 +++++++++++++++++++++++++-- types/cache-interceptor.d.ts | 5 +++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index a7a379a11bf..f6952b56af5 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -14,8 +14,11 @@ const { Writable } = require('node:stream') */ class MemoryCacheStore { #maxCount = Infinity + #maxSize = Infinity #maxEntrySize = Infinity + #size = 0 + /** * @type {Map>} */ @@ -42,6 +45,17 @@ class MemoryCacheStore { this.#maxCount = opts.maxCount } + if (opts.maxSize !== undefined) { + if ( + typeof opts.maxSize !== 'number' || + !Number.isInteger(opts.maxSize) || + opts.maxSize < 0 + ) { + throw new TypeError('MemoryCacheStore options.maxSize must be a non-negative integer') + } + this.#maxSize = opts.maxSize + } + if (opts.maxEntrySize !== undefined) { if ( typeof opts.maxEntrySize !== 'number' || @@ -56,7 +70,7 @@ class MemoryCacheStore { } get isFull () { - return this.#arr.length >= this.#maxCount + return this.#arr.length >= this.#maxCount || this.#size >= this.#maxSize } /** @@ -65,7 +79,8 @@ class MemoryCacheStore { */ get (key) { const values = this.#getValuesForRequest(key) - return findValue(key, values) + const value = findValue(key, values) + return value != null ? { ...value } : undefined } /** @@ -120,7 +135,12 @@ class MemoryCacheStore { let value = findValue(key, values) if (!value) { value = { ...opts, body } + store.#arr.push(value) + for (const buf of body) { + store.#size += buf.byteLength + } + values.push(value) } else { Object.assign(value, opts, { body }) @@ -172,6 +192,9 @@ class MemoryCacheStore { const count = Math.max(0, this.#arr.length - this.#maxCount / 2) for (const value of this.#arr.splice(0, count)) { + for (const buf of value.body) { + this.#size -= buf.byteLength + } value.body = null } diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 015c2025eba..c104a0d08cd 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -79,6 +79,11 @@ declare namespace CacheHandler { */ maxCount?: number + /** + * @default Infinity + */ + maxSize?: number + /** * @default Infinity */ From b28d3902733d09329c085985c9b32eded758828e Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 13:36:01 +0100 Subject: [PATCH 07/13] fixup --- lib/cache/memory-cache-store.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index f6952b56af5..367e5eeb0c2 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -80,6 +80,8 @@ class MemoryCacheStore { get (key) { const values = this.#getValuesForRequest(key) const value = findValue(key, values) + + // TODO (perf): Faster with explicit props... return value != null ? { ...value } : undefined } @@ -109,9 +111,10 @@ class MemoryCacheStore { const store = this const body = [] - // TODO (fix): Make sure user can't modify... - // key = structuredClone(key) - // opts = structuredClone(opts) + // TODO (fix): Deep clone... + // TODO (perf): Faster with explicit props... + key = { ...key } + opts = { ...opts } return new Writable({ write (chunk, encoding, callback) { @@ -136,7 +139,7 @@ class MemoryCacheStore { if (!value) { value = { ...opts, body } - store.#arr.push(value) + store.#arr.push(value.body) for (const buf of body) { store.#size += buf.byteLength } From a8eccb7f6ee7592f7bbf312662a75eb9faed9e58 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 13:39:41 +0100 Subject: [PATCH 08/13] fixup --- lib/cache/memory-cache-store.js | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 367e5eeb0c2..84853139523 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -20,7 +20,7 @@ class MemoryCacheStore { #size = 0 /** - * @type {Map>} + * @type {Map>} */ #map = new Map() #arr = [] @@ -158,6 +158,26 @@ class MemoryCacheStore { * @param {CacheKey} key */ delete (key) { + // TODO (perf): This could be implemented more efficiently... + + // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 + const topLevelKey = `${key.origin}:${key.path}` + + const cachedPaths = this.#map.get(topLevelKey) + if (cachedPaths) { + for (const values of cachedPaths.values()) { + for (const value of values) { + if (value.body) { + for (const buf of value.body) { + this.#size -= buf.byteLength + } + value.body = null + } + } + } + this.#arr = this.#arr.filter(value => value.body != null) + } + this.#map.delete(`${key.origin}:${key.path}`) } From 8ed6901963d046fe781f0c91382f41bc763ddfc3 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 14:17:07 +0100 Subject: [PATCH 09/13] fixup --- lib/cache/memory-cache-store.js | 176 ++++++++++---------------------- types/cache-interceptor.d.ts | 45 ++++---- 2 files changed, 74 insertions(+), 147 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 84853139523..f4687929df1 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -4,8 +4,8 @@ const { Writable } = require('node:stream') /** * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheKey} CacheKey + * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheValue} CacheValue * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore - * @typedef {import('../../types/cache-interceptor.d.ts').default.CachedResponse} CachedResponse * @typedef {import('../../types/cache-interceptor.d.ts').default.GetResult} GetResult */ @@ -18,11 +18,6 @@ class MemoryCacheStore { #maxEntrySize = Infinity #size = 0 - - /** - * @type {Map>} - */ - #map = new Map() #arr = [] /** @@ -74,25 +69,40 @@ class MemoryCacheStore { } /** - * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} key + * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} req * @returns {import('../../types/cache-interceptor.d.ts').default.GetResult | undefined} */ - get (key) { - const values = this.#getValuesForRequest(key) - const value = findValue(key, values) - - // TODO (perf): Faster with explicit props... - return value != null ? { ...value } : undefined + get ({ origin, path, method, headers }) { + const now = Date.now() + + const value = this.#arr.find((value) => ( + value.method === method && + value.origin === origin && + value.path === path && + value.deleteAt > now && + (!value.vary || Object.keys(value.vary).every(headerName => value.vary[headerName] === headers?.[headerName])) + )) + + return value != null + ? { + statusMessage: value.statusMessage, + statusCode: value.statusCode, + rawHeaders: value.rawHeaders, + cachedAt: value.cachedAt, + staleAt: value.staleAt, + body: value.body + } + : undefined } /** - * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} key - * @param {import('../../types/cache-interceptor.d.ts').default.CachedResponse} opts + * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} req + * @param {import('../../types/cache-interceptor.d.ts').default.CacheValue} opts * @returns {Writable | undefined} */ - createWriteStream (key, opts) { - if (typeof key !== 'object') { - throw new TypeError(`expected key to be object, got ${typeof key}`) + createWriteStream (req, opts) { + if (typeof req !== 'object') { + throw new TypeError(`expected key to be object, got ${typeof req}`) } if (typeof opts !== 'object') { throw new TypeError(`expected value to be object, got ${typeof opts}`) @@ -109,12 +119,23 @@ class MemoryCacheStore { let currentSize = 0 const store = this - const body = [] // TODO (fix): Deep clone... // TODO (perf): Faster with explicit props... - key = { ...key } - opts = { ...opts } + const val = { + statusCode: opts.statusCode, + statusMessage: opts.statusMessage, + rawHeaders: opts.rawHeaders, + vary: opts.vary, + cachedAt: opts.cachedAt, + staleAt: opts.staleAt, + deleteAt: opts.deleteAt, + method: req.method, + origin: req.origin, + path: req.path, + /** @type {Buffer[]} */ + body: [] + } return new Writable({ write (chunk, encoding, callback) { @@ -127,28 +148,16 @@ class MemoryCacheStore { if (currentSize >= store.#maxEntrySize) { this.destroy() } else { - body.push(chunk) + val.body.push(chunk) } callback(null) }, final (callback) { - const values = store.#getValuesForRequest(key) - - let value = findValue(key, values) - if (!value) { - value = { ...opts, body } - - store.#arr.push(value.body) - for (const buf of body) { - store.#size += buf.byteLength - } - - values.push(value) - } else { - Object.assign(value, opts, { body }) + store.#arr.push(val) + for (const buf of val.body) { + store.#size += buf.byteLength } - callback(null) } }) @@ -157,104 +166,29 @@ class MemoryCacheStore { /** * @param {CacheKey} key */ - delete (key) { - // TODO (perf): This could be implemented more efficiently... - + delete ({ origin, path }) { // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 - const topLevelKey = `${key.origin}:${key.path}` - - const cachedPaths = this.#map.get(topLevelKey) - if (cachedPaths) { - for (const values of cachedPaths.values()) { - for (const value of values) { - if (value.body) { - for (const buf of value.body) { - this.#size -= buf.byteLength - } - value.body = null - } + const arr = [] + for (const value of this.#arr) { + if (value.path === path && value.origin === origin) { + for (const buf of value.body) { + this.#size -= buf.byteLength } + } else { + arr.push(value) } - this.#arr = this.#arr.filter(value => value.body != null) } - - this.#map.delete(`${key.origin}:${key.path}`) - } - - /** - * Gets all of the requests of the same origin, path, and method. Does not - * take the `vary` property into account. - * @param {CacheKey} key - * @returns {GetResult[]} - */ - #getValuesForRequest (key) { - if (typeof key !== 'object') { - throw new TypeError(`expected key to be object, got ${typeof key}`) - } - - // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 - const topLevelKey = `${key.origin}:${key.path}` - - let cachedPaths = this.#map.get(topLevelKey) - if (!cachedPaths) { - cachedPaths = new Map() - this.#map.set(topLevelKey, cachedPaths) - } - - let values = cachedPaths.get(key.method) - if (!values) { - values = [] - cachedPaths.set(key.method, values) - } - - return values + this.#arr = arr } #prune () { - // TODO (perf): This could be implemented more efficiently... - const count = Math.max(0, this.#arr.length - this.#maxCount / 2) for (const value of this.#arr.splice(0, count)) { for (const buf of value.body) { this.#size -= buf.byteLength } - value.body = null - } - - for (const [key, cachedPaths] of this.#map) { - for (const [method, prev] of cachedPaths) { - const next = prev.filter(({ body }) => body == null) - if (next.length === 0) { - cachedPaths.delete(method) - if (cachedPaths.size === 0) { - this.#map.delete(key) - } - } else if (next.length !== prev.length) { - cachedPaths.set(method, next) - } - } } } } -/** - * Given a list of values of a certain request, this decides the best value - * to respond with. - * @param {CacheKey} key - * @param {GetResult[] | undefined } values - * @returns {(GetResult) | undefined} - */ -function findValue (key, values) { - if (typeof key !== 'object') { - throw new TypeError(`expected key to be object, got ${typeof key}`) - } - - const now = Date.now() - return values?.find(({ deleteAt, vary, body }) => ( - body != null && - deleteAt > now && - (!vary || Object.keys(vary).every(headerName => vary[headerName] === key.headers?.[headerName])) - )) -} - module.exports = MemoryCacheStore diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index c104a0d08cd..95d2450aaf7 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -25,13 +25,30 @@ declare namespace CacheHandler { headers?: Record } + export interface CacheValue { + statusCode: number + statusMessage: string + rawHeaders: Buffer[] + vary: Record + cachedAt: number + staleAt: number + deleteAt: number + } + export interface DeleteByUri { origin: string method: string path: string } - type GetResult = CachedResponse & { body: null | Readable | Iterable | Buffer | Iterable | string } + type GetResult = { + statusCode: number + statusMessage: string + rawHeaders: Buffer[] + body: null | Readable | Iterable | Buffer | Iterable | string + cachedAt: number + staleAt: number + } /** * Underlying storage provider for cached responses @@ -44,35 +61,11 @@ declare namespace CacheHandler { get(key: CacheKey): GetResult | Promise | undefined - createWriteStream(key: CacheKey, value: CachedResponse): Writable | undefined + createWriteStream(key: CacheKey, val: CacheValue): Writable | undefined delete(key: CacheKey): void | Promise } - export interface CachedResponse { - statusCode: number; - statusMessage: string; - rawHeaders: Buffer[]; - /** - * Headers defined by the Vary header and their respective values for - * later comparison - */ - vary?: Record - /** - * 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 From 53af5153a59e148100e2e76f086027aeaa7520ed Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 14:20:11 +0100 Subject: [PATCH 10/13] fixup --- lib/cache/memory-cache-store.js | 124 +++++++++++--------------------- types/cache-interceptor.d.ts | 9 +-- 2 files changed, 43 insertions(+), 90 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index f4687929df1..af5f14a9fbc 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -18,7 +18,7 @@ class MemoryCacheStore { #maxEntrySize = Infinity #size = 0 - #arr = [] + #entries = [] /** * @param {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts | undefined} [opts] @@ -64,78 +64,40 @@ class MemoryCacheStore { } } - get isFull () { - return this.#arr.length >= this.#maxCount || this.#size >= this.#maxSize - } - /** * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} req * @returns {import('../../types/cache-interceptor.d.ts').default.GetResult | undefined} */ - get ({ origin, path, method, headers }) { - const now = Date.now() + get (key) { + if (typeof key !== 'object') { + throw new TypeError(`expected key to be object, got ${typeof key}`) + } - const value = this.#arr.find((value) => ( - value.method === method && - value.origin === origin && - value.path === path && - value.deleteAt > now && - (!value.vary || Object.keys(value.vary).every(headerName => value.vary[headerName] === headers?.[headerName])) + const now = Date.now() + return this.#entries.find((entry) => ( + entry.deleteAt > now && + entry.method === key.method && + entry.origin === key.origin && + entry.path === key.path && + (entry.vary == null || Object.keys(entry.vary).every(headerName => entry.vary[headerName] === key.headers?.[headerName])) )) - - return value != null - ? { - statusMessage: value.statusMessage, - statusCode: value.statusCode, - rawHeaders: value.rawHeaders, - cachedAt: value.cachedAt, - staleAt: value.staleAt, - body: value.body - } - : undefined } /** - * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} req - * @param {import('../../types/cache-interceptor.d.ts').default.CacheValue} opts + * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} key + * @param {import('../../types/cache-interceptor.d.ts').default.CacheValue} val * @returns {Writable | undefined} */ - createWriteStream (req, opts) { - if (typeof req !== 'object') { - throw new TypeError(`expected key to be object, got ${typeof req}`) - } - if (typeof opts !== 'object') { - throw new TypeError(`expected value to be object, got ${typeof opts}`) + createWriteStream (key, val) { + if (typeof key !== 'object') { + throw new TypeError(`expected key to be object, got ${typeof key}`) } - - if (this.isFull) { - this.#prune() - } - - if (this.isFull) { - return undefined + if (typeof val !== 'object') { + throw new TypeError(`expected value to be object, got ${typeof val}`) } - let currentSize = 0 - const store = this - - // TODO (fix): Deep clone... - // TODO (perf): Faster with explicit props... - const val = { - statusCode: opts.statusCode, - statusMessage: opts.statusMessage, - rawHeaders: opts.rawHeaders, - vary: opts.vary, - cachedAt: opts.cachedAt, - staleAt: opts.staleAt, - deleteAt: opts.deleteAt, - method: req.method, - origin: req.origin, - path: req.path, - /** @type {Buffer[]} */ - body: [] - } + const entry = { ...key, ...val, body: [], size: 0 } return new Writable({ write (chunk, encoding, callback) { @@ -143,21 +105,27 @@ class MemoryCacheStore { chunk = Buffer.from(chunk, encoding) } - currentSize += chunk.byteLength + entry.size += chunk.byteLength - if (currentSize >= store.#maxEntrySize) { + if (entry.size >= store.#maxEntrySize) { this.destroy() } else { - val.body.push(chunk) + entry.body.push(chunk) } callback(null) }, final (callback) { - store.#arr.push(val) - for (const buf of val.body) { - store.#size += buf.byteLength + store.#entries.push(entry) + store.#size += entry.size + + while (store.#entries.length >= store.#maxCount || store.#size >= store.#maxSize) { + const count = Math.max(0, store.#entries.length - store.#maxCount / 2) + for (const entry of store.#entries.splice(0, count)) { + store.#size -= entry.size + } } + callback(null) } }) @@ -166,28 +134,20 @@ class MemoryCacheStore { /** * @param {CacheKey} key */ - delete ({ origin, path }) { - // https://www.rfc-editor.org/rfc/rfc9111.html#section-2-3 - const arr = [] - for (const value of this.#arr) { - if (value.path === path && value.origin === origin) { - for (const buf of value.body) { - this.#size -= buf.byteLength - } - } else { - arr.push(value) - } + delete (key) { + if (typeof key !== 'object') { + throw new TypeError(`expected key to be object, got ${typeof key}`) } - this.#arr = arr - } - #prune () { - const count = Math.max(0, this.#arr.length - this.#maxCount / 2) - for (const value of this.#arr.splice(0, count)) { - for (const buf of value.body) { - this.#size -= buf.byteLength + const arr = [] + for (const entry of this.#entries) { + if (entry.path === key.path && entry.origin === key.origin) { + this.#size -= entry.size + } else { + arr.push(entry) } } + this.#entries = arr } } diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 95d2450aaf7..5de18a81aae 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -45,7 +45,7 @@ declare namespace CacheHandler { statusCode: number statusMessage: string rawHeaders: Buffer[] - body: null | Readable | Iterable | Buffer | Iterable | string + body: null | Readable | Iterable | AsyncIterable | Buffer | Iterable | AsyncIterable | string cachedAt: number staleAt: number } @@ -54,11 +54,6 @@ declare namespace CacheHandler { * Underlying storage provider for cached responses */ export interface CacheStore { - /** - * Whether or not the cache is full and can not store any more responses - */ - get isFull(): boolean | undefined - get(key: CacheKey): GetResult | Promise | undefined createWriteStream(key: CacheKey, val: CacheValue): Writable | undefined @@ -88,8 +83,6 @@ declare namespace CacheHandler { export class MemoryCacheStore implements CacheStore { constructor (opts?: MemoryCacheStoreOpts) - get isFull (): boolean | undefined - get (key: CacheKey): GetResult | Promise | undefined createWriteStream (key: CacheKey, value: CachedResponse): Writable | undefined From eed110e070b0e37c3c99b8d92f973cdb0eddb464 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 14:47:06 +0100 Subject: [PATCH 11/13] fixup --- lib/cache/memory-cache-store.js | 49 +++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index af5f14a9fbc..ab8d470eae7 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -18,7 +18,8 @@ class MemoryCacheStore { #maxEntrySize = Infinity #size = 0 - #entries = [] + #count = 0 + #entries = new Map() /** * @param {import('../../types/cache-interceptor.d.ts').default.MemoryCacheStoreOpts | undefined} [opts] @@ -73,12 +74,12 @@ class MemoryCacheStore { throw new TypeError(`expected key to be object, got ${typeof key}`) } + const topLevelKey = `${key.origin}:${key.path}` + const now = Date.now() - return this.#entries.find((entry) => ( + return this.#entries.get(topLevelKey)?.find((entry) => ( entry.deleteAt > now && entry.method === key.method && - entry.origin === key.origin && - entry.path === key.path && (entry.vary == null || Object.keys(entry.vary).every(headerName => entry.vary[headerName] === key.headers?.[headerName])) )) } @@ -96,6 +97,8 @@ class MemoryCacheStore { throw new TypeError(`expected value to be object, got ${typeof val}`) } + const topLevelKey = `${key.origin}:${key.path}` + const store = this const entry = { ...key, ...val, body: [], size: 0 } @@ -116,13 +119,25 @@ class MemoryCacheStore { callback(null) }, final (callback) { - store.#entries.push(entry) - store.#size += entry.size + let entries = store.#entries.get(topLevelKey) + if (!entries) { + entries = [] + store.#entries.set(topLevelKey, entries) + } + entries.push(entry) - while (store.#entries.length >= store.#maxCount || store.#size >= store.#maxSize) { - const count = Math.max(0, store.#entries.length - store.#maxCount / 2) - for (const entry of store.#entries.splice(0, count)) { - store.#size -= entry.size + store.#size += entry.size + store.#count += 1 + + if (store.#size > store.#maxSize || store.#count > store.#maxCount) { + for (const [key, entries] of store.#entries) { + for (const entry of entries.splice(0, entries.length / 2)) { + store.#size -= entry.size + store.#count -= 1 + } + if (entries.length === 0) { + store.#entries.delete(key) + } } } @@ -139,15 +154,13 @@ class MemoryCacheStore { throw new TypeError(`expected key to be object, got ${typeof key}`) } - const arr = [] - for (const entry of this.#entries) { - if (entry.path === key.path && entry.origin === key.origin) { - this.#size -= entry.size - } else { - arr.push(entry) - } + const topLevelKey = `${key.origin}:${key.path}` + + for (const entry of this.#entries.get(topLevelKey) ?? []) { + this.#size -= entry.size + this.#count -= 1 } - this.#entries = arr + this.#entries.delete(topLevelKey) } } From 2f969ad711d6e87096cbb31fdede17dfad3bc7e4 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 15 Nov 2024 21:11:50 +0100 Subject: [PATCH 12/13] fixup --- lib/cache/memory-cache-store.js | 14 +++++++++++++- test/cache-interceptor/cache-stores.js | 4 ++-- types/cache-interceptor.d.ts | 1 + 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index ab8d470eae7..4181cbbf9ca 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -77,11 +77,23 @@ class MemoryCacheStore { const topLevelKey = `${key.origin}:${key.path}` const now = Date.now() - return this.#entries.get(topLevelKey)?.find((entry) => ( + const entry = this.#entries.get(topLevelKey)?.find((entry) => ( entry.deleteAt > now && entry.method === key.method && (entry.vary == null || Object.keys(entry.vary).every(headerName => entry.vary[headerName] === key.headers?.[headerName])) )) + + return entry == null + ? undefined + : { + statusMessage: entry.statusMessage, + statusCode: entry.statusCode, + rawHeaders: entry.rawHeaders, + body: entry.body, + cachedAt: entry.cachedAt, + staleAt: entry.staleAt, + deleteAt: entry.deleteAt + } } /** diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index 129d353b473..dfa521fa0d1 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -15,7 +15,6 @@ function cacheStoreTests (CacheStore) { describe(CacheStore.prototype.constructor.name, () => { test('matches interface', async () => { const store = new CacheStore() - equal(typeof store.isFull, 'boolean') equal(typeof store.get, 'function') equal(typeof store.createWriteStream, 'function') equal(typeof store.delete, 'function') @@ -195,8 +194,9 @@ function cacheStoreTests (CacheStore) { const readStream = store.get(structuredClone(request)) notEqual(readStream, undefined) + const { vary, ...responseValue } = requestValue deepStrictEqual(await readResponse(readStream), { - ...requestValue, + ...responseValue, body: requestBody }) diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 5de18a81aae..cb2cef312b3 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -48,6 +48,7 @@ declare namespace CacheHandler { body: null | Readable | Iterable | AsyncIterable | Buffer | Iterable | AsyncIterable | string cachedAt: number staleAt: number + deleteAt: number } /** From 2b366b5a26b8f8d9c67ccb48f94c1deaeb126cfe Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 18 Nov 2024 07:29:29 +0100 Subject: [PATCH 13/13] fixup --- test/types/cache-interceptor.test-d.ts | 12 +++++------- types/cache-interceptor.d.ts | 4 ++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/test/types/cache-interceptor.test-d.ts b/test/types/cache-interceptor.test-d.ts index 4d9ad4b99a2..cfa6a90b779 100644 --- a/test/types/cache-interceptor.test-d.ts +++ b/test/types/cache-interceptor.test-d.ts @@ -3,13 +3,11 @@ import { expectAssignable, expectNotAssignable } from 'tsd' import CacheInterceptor from '../../types/cache-interceptor' const store: CacheInterceptor.CacheStore = { - isFull: false, - get (_: CacheInterceptor.CacheKey): CacheInterceptor.GetResult | Promise | undefined { throw new Error('stub') }, - createWriteStream (_: CacheInterceptor.CacheKey, _2: CacheInterceptor.CachedResponse): Writable | undefined { + createWriteStream (_: CacheInterceptor.CacheKey, _2: CacheInterceptor.CacheValue): Writable | undefined { throw new Error('stub') }, @@ -23,7 +21,7 @@ expectAssignable({ store }) expectAssignable({ methods: [] }) expectAssignable({ store, methods: ['GET'] }) -expectAssignable({ +expectAssignable({ statusCode: 200, statusMessage: 'OK', rawHeaders: [], @@ -32,7 +30,7 @@ expectAssignable({ deleteAt: 0 }) -expectAssignable({ +expectAssignable({ statusCode: 200, statusMessage: 'OK', rawHeaders: [], @@ -42,8 +40,8 @@ expectAssignable({ deleteAt: 0 }) -expectNotAssignable({}) -expectNotAssignable({ +expectNotAssignable({}) +expectNotAssignable({ statusCode: '123', statusMessage: 123, rawHeaders: '', diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index cb2cef312b3..fd71e30f586 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -29,7 +29,7 @@ declare namespace CacheHandler { statusCode: number statusMessage: string rawHeaders: Buffer[] - vary: Record + vary?: Record cachedAt: number staleAt: number deleteAt: number @@ -86,7 +86,7 @@ declare namespace CacheHandler { get (key: CacheKey): GetResult | Promise | undefined - createWriteStream (key: CacheKey, value: CachedResponse): Writable | undefined + createWriteStream (key: CacheKey, value: CacheValue): Writable | undefined delete (key: CacheKey): void | Promise }