Skip to content

Commit

Permalink
Call dispose() when fetch overwrites a value
Browse files Browse the repository at this point in the history
This corrects an oversight in the disposal flow on set overwrites.

We don't call dispose when creating the BackgroundFetch, because of
course, the thing hasn't been replaced yet.

And we don't call dispose when _replacing_ a BackgroundFetch, because
that isn't a value, it's just a potential value, so we abort the fetch
and forget about it (which is a no-op if the set() in question is the
resolution of that BackgroundFetch).

The missing piece is that we *do* have to dispose() the
`__staleWhileFetching` value on that BackgroundFetch promise, if there
is one, when overwriting a BackgroundFetch.

Fix: #309
  • Loading branch information
isaacs committed Aug 10, 2023
1 parent 0c63b0d commit fcb797e
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 2 deletions.
9 changes: 9 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1682,6 +1682,15 @@ export class LRUCache<K extends {}, V extends {}, FC = unknown> {
if (v !== oldVal) {
if (this.#hasFetchMethod && this.#isBackgroundFetch(oldVal)) {
oldVal.__abortController.abort(new Error('replaced'))
const { __staleWhileFetching: s } = oldVal
if (s !== undefined && !noDisposeOnSet) {
if (this.#hasDispose) {
this.#dispose?.(s as V, k, 'set')
}
if (this.#hasDisposeAfter) {
this.#disposed?.push([s as V, k, 'set'])
}
}
} else if (!noDisposeOnSet) {
if (this.#hasDispose) {
this.#dispose?.(oldVal as V, k, 'set')
Expand Down
26 changes: 24 additions & 2 deletions test/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ if (typeof performance === 'undefined') {
global.performance = require('perf_hooks').performance
}
import t from 'tap'
import { LRUCache, BackgroundFetch } from '../'
import { BackgroundFetch, LRUCache } from '../'
import { expose } from './fixtures/expose'

const fn: LRUCache.Fetcher<any, any> = async (_, v) =>
Expand Down Expand Up @@ -720,7 +720,10 @@ t.test('allowStaleOnFetchAbort', async t => {
const p = c.fetch(1, { signal: ac.signal })
ac.abort(new Error('gimme the stale value'))
t.equal(await p, 10)
t.equal(c.get(1, { allowStale: true, noDeleteOnStaleGet: true }), 10)
t.equal(
c.get(1, { allowStale: true, noDeleteOnStaleGet: true }),
10
)
const p2 = c.fetch(1)
c.set(1, 100)
t.equal(await p2, 10)
Expand Down Expand Up @@ -844,3 +847,22 @@ t.test('has false for pending fetch without stale val', async t => {
t.equal(c.has(1), true)
}
})

t.test('properly dispose when using fetch', async t => {
const disposes: [number, number, string][] = []
const disposeAfters: [number, number, string][] = []
let i = 0
const c = new LRUCache<number, number>({
max: 3,
ttl: 10,
dispose: (key, val, reason) => disposes.push([key, val, reason]),
disposeAfter: (key, val, reason) =>
disposeAfters.push([key, val, reason]),
fetchMethod: async () => Promise.resolve(i++),
})
t.equal(await c.fetch(1), 0)
clock.advance(20)
t.equal(await c.fetch(1), 1)
t.strictSame(disposes, [[0, 1, 'set']])
t.strictSame(disposeAfters, [[0, 1, 'set']])
})

0 comments on commit fcb797e

Please sign in to comment.