Skip to content

Commit 72f8f7e

Browse files
ronagronag
committed
fix: cache fixes
1 parent 51836bb commit 72f8f7e

File tree

8 files changed

+82
-100
lines changed

8 files changed

+82
-100
lines changed

‎lib/cache/memory-cache-store.js‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use strict'
22

3-
const { Writable, Readable } = require('node:stream')
3+
const { Writable } = require('node:stream')
44

55
/**
66
* @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore
@@ -81,23 +81,9 @@ class MemoryCacheStore {
8181
return undefined
8282
}
8383

84-
/**
85-
* @type {Readable | undefined}
86-
*/
87-
let readable
88-
if (value.body) {
89-
readable = new Readable()
90-
91-
for (const chunk of value.body) {
92-
readable.push(chunk)
93-
}
94-
95-
readable.push(null)
96-
}
97-
9884
return {
9985
response: value.opts,
100-
body: readable
86+
body: value.body
10187
}
10288
}
10389

@@ -242,7 +228,7 @@ class MemoryCacheStore {
242228
/**
243229
* @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} key
244230
*/
245-
deleteByKey (key) {
231+
delete (key) {
246232
this.#data.delete(`${key.origin}:${key.path}`)
247233
}
248234

‎lib/handler/cache-handler.js‎

Lines changed: 25 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ class CacheHandler extends DecoratorHandler {
9494
) {
9595
// https://www.rfc-editor.org/rfc/rfc9111.html#name-invalidating-stored-response
9696
try {
97-
this.#store.deleteByKey(this.#cacheKey).catch?.(noop)
97+
this.#store.delete(this.#cacheKey).catch?.(noop)
9898
} catch {
9999
// Fail silently
100100
}
@@ -135,43 +135,31 @@ class CacheHandler extends DecoratorHandler {
135135
cacheControlDirectives
136136
)
137137

138-
if (this.#cacheKey.method === 'HEAD') {
139-
this.#store.createWriteStream(this.#cacheKey, {
140-
statusCode,
141-
statusMessage,
142-
rawHeaders: strippedHeaders,
143-
vary: varyDirectives,
144-
cachedAt: now,
145-
staleAt,
146-
deleteAt
147-
})
148-
} else {
149-
this.#writeStream = this.#store.createWriteStream(this.#cacheKey, {
150-
statusCode,
151-
statusMessage,
152-
rawHeaders: strippedHeaders,
153-
vary: varyDirectives,
154-
cachedAt: now,
155-
staleAt,
156-
deleteAt
157-
})
158-
159-
if (this.#writeStream) {
160-
const handler = this
161-
this.#writeStream
162-
.on('drain', resume)
163-
.on('error', function () {
138+
this.#writeStream = this.#store.createWriteStream(this.#cacheKey, {
139+
statusCode,
140+
statusMessage,
141+
rawHeaders: strippedHeaders,
142+
vary: varyDirectives,
143+
cachedAt: now,
144+
staleAt,
145+
deleteAt
146+
})
147+
148+
if (this.#writeStream) {
149+
const handler = this
150+
this.#writeStream
151+
.on('drain', resume)
152+
.on('error', function () {
164153
// TODO (fix): Make error somehow observable?
165-
})
166-
.on('close', function () {
167-
if (handler.#writeStream === this) {
168-
handler.#writeStream = undefined
169-
}
170-
171-
// TODO (fix): Should we resume even if was paused downstream?
172-
resume()
173-
})
174-
}
154+
})
155+
.on('close', function () {
156+
if (handler.#writeStream === this) {
157+
handler.#writeStream = undefined
158+
}
159+
160+
// TODO (fix): Should we resume even if was paused downstream?
161+
resume()
162+
})
175163
}
176164
}
177165

‎lib/interceptor/cache.js‎

Lines changed: 28 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict'
22

33
const assert = require('node:assert')
4+
const { Readable } = require('node:stream')
45
const util = require('../core/util')
56
const CacheHandler = require('../handler/cache-handler')
67
const MemoryCacheStore = require('../cache/memory-cache-store')
@@ -57,27 +58,25 @@ module.exports = (opts = {}) => {
5758
// Where body can be a Buffer, string, stream or blob?
5859
const result = store.get(cacheKey)
5960
if (!result) {
60-
// Request isn't cached
6161
return dispatch(opts, new CacheHandler(globalOpts, cacheKey, handler))
6262
}
6363

6464
/**
65-
* @param {import('node:stream').Readable | undefined} stream
65+
* @param {import('node:stream').Readable} stream
6666
* @param {import('../../types/cache-interceptor.d.ts').default.CachedResponse} value
6767
*/
6868
const respondWithCachedValue = (stream, value) => {
69-
assert(!stream || !stream.destroyed, 'stream should not be destroyed')
70-
assert(!stream || !stream.readableDidRead, 'stream should not be readableDidRead')
69+
assert(!stream.destroyed, 'stream should not be destroyed')
70+
assert(!stream.readableDidRead, 'stream should not be readableDidRead')
71+
7172
try {
7273
stream
73-
?.on('error', function (err) {
74+
.on('error', function (err) {
7475
if (!this.readableEnded) {
7576
if (typeof handler.onError === 'function') {
7677
handler.onError(err)
7778
} else {
78-
process.nextTick(() => {
79-
throw err
80-
})
79+
throw err
8180
}
8281
}
8382
})
@@ -89,10 +88,10 @@ module.exports = (opts = {}) => {
8988

9089
if (typeof handler.onConnect === 'function') {
9190
handler.onConnect((err) => {
92-
stream?.destroy(err)
91+
stream.destroy(err)
9392
})
9493

95-
if (stream?.destroyed) {
94+
if (stream.destroyed) {
9695
return
9796
}
9897
}
@@ -106,16 +105,12 @@ module.exports = (opts = {}) => {
106105
const rawHeaders = [...value.rawHeaders, AGE_HEADER, Buffer.from(`${age}`)]
107106

108107
if (handler.onHeaders(value.statusCode, rawHeaders, () => stream?.resume(), value.statusMessage) === false) {
109-
stream?.pause()
108+
stream.pause()
110109
}
111110
}
112111

113112
if (opts.method === 'HEAD') {
114-
if (typeof handler.onComplete === 'function') {
115-
handler.onComplete([])
116-
}
117-
118-
stream?.destroy()
113+
stream.destroy()
119114
} else {
120115
stream.on('data', function (chunk) {
121116
if (typeof handler.onData === 'function' && !handler.onData(chunk)) {
@@ -124,15 +119,20 @@ module.exports = (opts = {}) => {
124119
})
125120
}
126121
} catch (err) {
127-
stream?.destroy(err)
122+
stream.destroy(err)
128123
}
129124
}
130125

131126
/**
132127
* @param {import('../../types/cache-interceptor.d.ts').default.GetResult} result
133128
*/
134129
const handleStream = (result) => {
135-
const { response: value, body: stream } = result
130+
const { response: value, body } = result
131+
132+
// TODO (perf): Readable.from path can be optimized...
133+
const stream = util.isStream(body)
134+
? body
135+
: Readable.from(body ?? [])
136136

137137
if (!stream && opts.method !== 'HEAD') {
138138
throw new Error('stream is undefined but method isn\'t HEAD')
@@ -177,12 +177,17 @@ module.exports = (opts = {}) => {
177177
if (typeof result.then === 'function') {
178178
result.then((result) => {
179179
if (!result) {
180-
// Request isn't cached
181-
return dispatch(opts, new CacheHandler(globalOpts, cacheKey, handler))
180+
dispatch(opts, new CacheHandler(globalOpts, cacheKey, handler))
181+
} else {
182+
handleStream(result)
182183
}
183-
184-
handleStream(result)
185-
}).catch(err => handler.onError(err))
184+
}, err => {
185+
if (typeof handler.onError === 'function') {
186+
handler.onError(err)
187+
} else {
188+
throw err
189+
}
190+
})
186191
} else {
187192
handleStream(result)
188193
}

‎lib/util/cache.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ function assertCacheStore (store, name = 'CacheStore') {
210210
throw new TypeError(`expected type of ${name} to be a CacheStore, got ${store === null ? 'null' : typeof store}`)
211211
}
212212

213-
for (const fn of ['get', 'createWriteStream', 'deleteByKey']) {
213+
for (const fn of ['get', 'createWriteStream', 'delete']) {
214214
if (typeof store[fn] !== 'function') {
215215
throw new TypeError(`${name} needs to have a \`${fn}()\` function`)
216216
}

‎test/cache-interceptor/cache-stores.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
const { describe, test } = require('node:test')
44
const { deepStrictEqual, notEqual, equal } = require('node:assert')
5+
const { Readable } = require('node:stream')
56
const { once } = require('node:events')
67
const MemoryCacheStore = require('../../lib/cache/memory-cache-store')
78

@@ -17,7 +18,7 @@ function cacheStoreTests (CacheStore) {
1718
equal(typeof store.isFull, 'boolean')
1819
equal(typeof store.get, 'function')
1920
equal(typeof store.createWriteStream, 'function')
20-
equal(typeof store.deleteByKey, 'function')
21+
equal(typeof store.delete, 'function')
2122
})
2223

2324
// Checks that it can store & fetch different responses
@@ -268,9 +269,11 @@ function writeResponse (stream, body) {
268269
* @param {import('../../types/cache-interceptor.d.ts').default.GetResult} result
269270
* @returns {Promise<import('../../types/cache-interceptor.d.ts').default.GetResult | { body: Buffer[] }>}
270271
*/
271-
async function readResponse ({ response, body: stream }) {
272+
async function readResponse ({ response, body: src }) {
272273
notEqual(response, undefined)
273-
notEqual(stream, undefined)
274+
notEqual(src, undefined)
275+
276+
const stream = Readable.from(src)
274277

275278
/**
276279
* @type {Buffer[]}

‎test/interceptors/cache.js‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -251,21 +251,21 @@ describe('Cache Interceptor', () => {
251251
})
252252
})
253253

254-
test('unsafe methods call the store\'s deleteByKey function', async () => {
254+
test('unsafe methods call the store\'s delete function', async () => {
255255
const server = createServer((_, res) => {
256256
res.end('asd')
257257
}).listen(0)
258258

259259
after(() => server.close())
260260
await once(server, 'listening')
261261

262-
let deleteByKeyCalled = false
262+
let deleteCalled = false
263263
const store = new cacheStores.MemoryCacheStore()
264264

265-
const originalDeleteByKey = store.deleteByKey.bind(store)
266-
store.deleteByKey = (key) => {
267-
deleteByKeyCalled = true
268-
originalDeleteByKey(key)
265+
const originaldelete = store.delete.bind(store)
266+
store.delete = (key) => {
267+
deleteCalled = true
268+
originaldelete(key)
269269
}
270270

271271
const client = new Client(`http://localhost:${server.address().port}`)
@@ -281,7 +281,7 @@ describe('Cache Interceptor', () => {
281281
path: '/'
282282
})
283283

284-
equal(deleteByKeyCalled, false)
284+
equal(deleteCalled, false)
285285

286286
// Make sure other safe methods that we don't want to cache don't cause a cache purge
287287
await client.request({
@@ -290,19 +290,19 @@ describe('Cache Interceptor', () => {
290290
path: '/'
291291
})
292292

293-
strictEqual(deleteByKeyCalled, false)
293+
strictEqual(deleteCalled, false)
294294

295295
// Make sure the common unsafe methods cause cache purges
296296
for (const method of ['POST', 'PUT', 'PATCH', 'DELETE']) {
297-
deleteByKeyCalled = false
297+
deleteCalled = false
298298

299299
await client.request({
300300
origin: 'localhost',
301301
method,
302302
path: '/'
303303
})
304304

305-
equal(deleteByKeyCalled, true, method)
305+
equal(deleteCalled, true, method)
306306
}
307307
})
308308

‎test/types/cache-interceptor.test-d.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ const store: CacheInterceptor.CacheStore = {
1313
throw new Error('stub')
1414
},
1515

16-
deleteByKey (_: CacheInterceptor.CacheKey): void | Promise<void> {
16+
delete (_: CacheInterceptor.CacheKey): void | Promise<void> {
1717
throw new Error('stub')
1818
}
1919
}

0 commit comments

Comments
 (0)