diff --git a/packages/node-cache/src/index.ts b/packages/node-cache/src/index.ts index 19259295..47154ebe 100644 --- a/packages/node-cache/src/index.ts +++ b/packages/node-cache/src/index.ts @@ -141,6 +141,7 @@ export class NodeCache extends Hookified { } const keyValue = this.formatKey(key); + const existing = this.store.get(keyValue); let expirationTimestamp = 0; // 0 = never delete if (this.isNegativeTtl(ttl)) { @@ -170,7 +171,7 @@ export class NodeCache extends Hookified { /* v8 ignore next -- @preserve */ if (this.options.maxKeys) { const { maxKeys } = this.options; - if (maxKeys > -1 && this.store.size >= maxKeys) { + if (maxKeys > -1 && this.store.size >= maxKeys && !existing) { throw this.createError(NodeCacheErrors.ECACHEFULL, this.formatKey(key)); } } @@ -184,6 +185,11 @@ export class NodeCache extends Hookified { // Event this.emit("set", keyValue, value, expirationTimestamp); + if (existing) { + this._stats.decreaseKSize(keyValue); + this._stats.decreaseVSize(existing.value); + } + // Add the bytes to the stats this._stats.incrementKSize(keyValue); this._stats.incrementVSize(value); @@ -285,9 +291,12 @@ export class NodeCache extends Hookified { * @returns {T | undefined} the value or undefined */ public take(key: string | number): V | undefined { + const keyValue = this.formatKey(key); + const item = this.store.get(keyValue); + const exists = item ? !(item.ttl > 0 && item.ttl < Date.now()) : false; const result = this.get(key); - if (result) { + if (exists) { this.del(key); if (this.options.useClones) { return this.clone(result) as V; @@ -296,7 +305,7 @@ export class NodeCache extends Hookified { return result as V; } - return undefined; + return result as V; } /** diff --git a/packages/node-cache/src/store.ts b/packages/node-cache/src/store.ts index 8681b55d..f74572a6 100644 --- a/packages/node-cache/src/store.ts +++ b/packages/node-cache/src/store.ts @@ -35,6 +35,7 @@ export class NodeCacheStore extends Hookified { private _useClones: boolean; private _deleteOnExpire: boolean; private _keys = new Set(); + private _values = new Map(); private _ttls = new Map(); private _intervalId: number | NodeJS.Timeout = 0; @@ -128,19 +129,17 @@ export class NodeCacheStore extends Hookified { const finalTtl = this.resolveTtl(ttl); const valueToStore = this._useClones ? this.clone(value) : value; - const isOverwrite = this._keys.has(keyValue); + const isOverwrite = this._values.has(keyValue); this._keys.add(keyValue); if (isOverwrite) { - const oldValue = await this._keyv.get(keyValue); + const oldValue = this._values.get(keyValue) as T; this._stats.decreaseKSize(keyValue); - if (oldValue !== undefined) { - this._stats.decreaseVSize(oldValue); - } - + this._stats.decreaseVSize(oldValue); this._stats.decreaseCount(); } await this._keyv.set(keyValue, valueToStore as T, finalTtl); + this._values.set(keyValue, value); this.trackExpiration(keyValue, finalTtl); @@ -221,13 +220,15 @@ export class NodeCacheStore extends Hookified { */ public async del(key: string | number): Promise { const keyValue = key.toString(); - const value = await this._keyv.get(keyValue); + const hadValue = this._values.has(keyValue); + const value = hadValue ? (this._values.get(keyValue) as T) : undefined; const result = await this._keyv.delete(keyValue); if (result) { this._keys.delete(keyValue); + this._values.delete(keyValue); this._ttls.delete(keyValue); this._stats.decreaseKSize(keyValue); - if (value !== undefined) { + if (hadValue) { this._stats.decreaseVSize(value); } @@ -256,6 +257,7 @@ export class NodeCacheStore extends Hookified { public async clear(): Promise { await this._keyv.clear(); this._keys.clear(); + this._values.clear(); this._ttls.clear(); this._stats.resetStoreValues(); } @@ -267,6 +269,7 @@ export class NodeCacheStore extends Hookified { public async flushAll(): Promise { await this._keyv.clear(); this._keys.clear(); + this._values.clear(); this._ttls.clear(); this._stats = new Stats({ enabled: true }); this.emit("flush"); @@ -283,8 +286,8 @@ export class NodeCacheStore extends Hookified { ttl?: number | string, ): Promise { const keyValue = key.toString(); - const item = await this._keyv.get(keyValue); - if (item !== undefined) { + if (this._values.has(keyValue)) { + const item = this._values.get(keyValue) as T; const finalTtl = this.resolveTtl(ttl); await this._keyv.set(keyValue, item, finalTtl); this.trackExpiration(keyValue, finalTtl); @@ -361,9 +364,10 @@ export class NodeCacheStore extends Hookified { } const result = await this._keyv.get(keyValue); - if (result !== undefined) { + if (this._values.has(keyValue)) { await this._keyv.delete(keyValue); this._keys.delete(keyValue); + this._values.delete(keyValue); this._ttls.delete(keyValue); this._stats.incrementHits(); this._stats.decreaseKSize(keyValue); @@ -490,7 +494,8 @@ export class NodeCacheStore extends Hookified { return; } - const value = await this._keyv.get(key); + const hadValue = this._values.has(key); + const value = hadValue ? this._values.get(key) : undefined; /* v8 ignore next 3 -- @preserve: race condition guard when key is refreshed during async get */ if (!this.isExpired(key)) { @@ -502,11 +507,11 @@ export class NodeCacheStore extends Hookified { if (this._deleteOnExpire) { await this._keyv.delete(key); this._keys.delete(key); + this._values.delete(key); this._ttls.delete(key); if (wasTracked) { this._stats.decreaseKSize(key); - /* v8 ignore next 3 -- @preserve: value is typically undefined when Keyv also expires the item */ - if (value !== undefined) { + if (hadValue) { this._stats.decreaseVSize(value); } diff --git a/packages/node-cache/test/index.test.ts b/packages/node-cache/test/index.test.ts index d7df09be..cbf7df15 100644 --- a/packages/node-cache/test/index.test.ts +++ b/packages/node-cache/test/index.test.ts @@ -124,6 +124,21 @@ describe("NodeCache", () => { expect(cache.take(faker.string.uuid())).toBe(undefined); }); + test("should take falsy values and clear stats", () => { + const cache = new NodeCache({ checkperiod: 0 }); + const key = faker.string.uuid(); + cache.set(key, 0); + + const taken = cache.take(key); + expect(taken).toBe(0); + expect(cache.has(key)).toBe(false); + + const stats = cache.getStats(); + expect(stats.keys).toBe(0); + expect(stats.ksize).toBe(0); + expect(stats.vsize).toBe(0); + }); + test("should delete a key", () => { const key = faker.string.uuid(); cache.set(key, faker.lorem.word()); @@ -330,6 +345,51 @@ describe("NodeCache", () => { expect(cache.getStats().keys).toBe(0); }); + test("should not inflate ksize/vsize when overwriting an existing key", () => { + const key = faker.string.uuid(); + const firstValue = faker.lorem.word(); + const secondValue = faker.lorem.words(6); + + const overwrittenCache = new NodeCache({ checkperiod: 0 }); + overwrittenCache.set(key, firstValue); + overwrittenCache.set(key, secondValue); + + const baselineCache = new NodeCache({ checkperiod: 0 }); + baselineCache.set(key, secondValue); + + const overwrittenStats = overwrittenCache.getStats(); + const baselineStats = baselineCache.getStats(); + + expect(overwrittenStats.keys).toBe(1); + expect(overwrittenStats.ksize).toBe(baselineStats.ksize); + expect(overwrittenStats.vsize).toBe(baselineStats.vsize); + + overwrittenCache.del(key); + const statsAfterDelete = overwrittenCache.getStats(); + expect(statsAfterDelete.keys).toBe(0); + expect(statsAfterDelete.ksize).toBe(0); + expect(statsAfterDelete.vsize).toBe(0); + }); + + test("should fully clear ksize/vsize after overwritten key expires", async () => { + const cache = new NodeCache({ + checkperiod: 0, + deleteOnExpire: true, + }); + const key = faker.string.uuid(); + + cache.set(key, faker.lorem.word()); + cache.set(key, faker.lorem.word(), 0.05); + + await sleep(100); + expect(cache.get(key)).toBe(undefined); + + const stats = cache.getStats(); + expect(stats.keys).toBe(0); + expect(stats.ksize).toBe(0); + expect(stats.vsize).toBe(0); + }); + test("should flush all the keys", () => { const cache = new NodeCache({ checkperiod: 0 }); const key1 = faker.string.uuid(); diff --git a/packages/node-cache/test/store.test.ts b/packages/node-cache/test/store.test.ts index 4740c3c1..d31d472f 100644 --- a/packages/node-cache/test/store.test.ts +++ b/packages/node-cache/test/store.test.ts @@ -906,6 +906,37 @@ describe("NodeCacheStore - overwrite stats", () => { const stats = store.getStats(); expect(stats.keys).toBe(1); }); + + test("should not leak vsize when overwriting undefined value", async () => { + const store = new NodeCacheStore(); + const key = faker.string.uuid(); + + await store.set(key, undefined as unknown); + await store.set(key, faker.lorem.word()); + + const overwriteStats = store.getStats(); + expect(overwriteStats.keys).toBe(1); + + await store.del(key); + const afterDeleteStats = store.getStats(); + expect(afterDeleteStats.keys).toBe(0); + expect(afterDeleteStats.ksize).toBe(0); + expect(afterDeleteStats.vsize).toBe(0); + }); + + test("should not leak vsize when undefined value expires", async () => { + const store = new NodeCacheStore({ checkperiod: 0, deleteOnExpire: true }); + const key = faker.string.uuid(); + + await store.set(key, undefined as unknown, 50); + await sleep(100); + + expect(await store.get(key)).toBeUndefined(); + const stats = store.getStats(); + expect(stats.keys).toBe(0); + expect(stats.ksize).toBe(0); + expect(stats.vsize).toBe(0); + }); }); describe("NodeCacheStore - setTtl with falsy values", () => {