Skip to content

Commit ee73e5a

Browse files
jmjones88gemini-code-assist[bot]jaredwray
authored
fix(node-cache): Fixing issues with metrics reporting (#1650)
* Fixing issues with metrics reporting * Apply suggestions from code review Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Jared Wray <me@jaredwray.com>
1 parent c6c3e21 commit ee73e5a

4 files changed

Lines changed: 122 additions & 17 deletions

File tree

‎packages/node-cache/src/index.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ export class NodeCache<T> extends Hookified {
141141
}
142142

143143
const keyValue = this.formatKey(key);
144+
const existing = this.store.get(keyValue);
144145
let expirationTimestamp = 0; // 0 = never delete
145146

146147
if (this.isNegativeTtl(ttl)) {
@@ -170,7 +171,7 @@ export class NodeCache<T> extends Hookified {
170171
/* v8 ignore next -- @preserve */
171172
if (this.options.maxKeys) {
172173
const { maxKeys } = this.options;
173-
if (maxKeys > -1 && this.store.size >= maxKeys) {
174+
if (maxKeys > -1 && this.store.size >= maxKeys && !existing) {
174175
throw this.createError(NodeCacheErrors.ECACHEFULL, this.formatKey(key));
175176
}
176177
}
@@ -184,6 +185,11 @@ export class NodeCache<T> extends Hookified {
184185
// Event
185186
this.emit("set", keyValue, value, expirationTimestamp);
186187

188+
if (existing) {
189+
this._stats.decreaseKSize(keyValue);
190+
this._stats.decreaseVSize(existing.value);
191+
}
192+
187193
// Add the bytes to the stats
188194
this._stats.incrementKSize(keyValue);
189195
this._stats.incrementVSize(value);
@@ -285,9 +291,12 @@ export class NodeCache<T> extends Hookified {
285291
* @returns {T | undefined} the value or undefined
286292
*/
287293
public take<V = T>(key: string | number): V | undefined {
294+
const keyValue = this.formatKey(key);
295+
const item = this.store.get(keyValue);
296+
const exists = item ? !(item.ttl > 0 && item.ttl < Date.now()) : false;
288297
const result = this.get(key);
289298

290-
if (result) {
299+
if (exists) {
291300
this.del(key);
292301
if (this.options.useClones) {
293302
return this.clone(result) as V;
@@ -296,7 +305,7 @@ export class NodeCache<T> extends Hookified {
296305
return result as V;
297306
}
298307

299-
return undefined;
308+
return result as V;
300309
}
301310

302311
/**

‎packages/node-cache/src/store.ts‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ export class NodeCacheStore<T> extends Hookified {
3535
private _useClones: boolean;
3636
private _deleteOnExpire: boolean;
3737
private _keys = new Set<string>();
38+
private _values = new Map<string, T>();
3839
private _ttls = new Map<string, number>();
3940
private _intervalId: number | NodeJS.Timeout = 0;
4041

@@ -128,19 +129,17 @@ export class NodeCacheStore<T> extends Hookified {
128129
const finalTtl = this.resolveTtl(ttl);
129130
const valueToStore = this._useClones ? this.clone(value) : value;
130131

131-
const isOverwrite = this._keys.has(keyValue);
132+
const isOverwrite = this._values.has(keyValue);
132133
this._keys.add(keyValue);
133134
if (isOverwrite) {
134-
const oldValue = await this._keyv.get(keyValue);
135+
const oldValue = this._values.get(keyValue) as T;
135136
this._stats.decreaseKSize(keyValue);
136-
if (oldValue !== undefined) {
137-
this._stats.decreaseVSize(oldValue);
138-
}
139-
137+
this._stats.decreaseVSize(oldValue);
140138
this._stats.decreaseCount();
141139
}
142140

143141
await this._keyv.set(keyValue, valueToStore as T, finalTtl);
142+
this._values.set(keyValue, value);
144143

145144
this.trackExpiration(keyValue, finalTtl);
146145

@@ -221,13 +220,15 @@ export class NodeCacheStore<T> extends Hookified {
221220
*/
222221
public async del(key: string | number): Promise<boolean> {
223222
const keyValue = key.toString();
224-
const value = await this._keyv.get(keyValue);
223+
const hadValue = this._values.has(keyValue);
224+
const value = hadValue ? (this._values.get(keyValue) as T) : undefined;
225225
const result = await this._keyv.delete(keyValue);
226226
if (result) {
227227
this._keys.delete(keyValue);
228+
this._values.delete(keyValue);
228229
this._ttls.delete(keyValue);
229230
this._stats.decreaseKSize(keyValue);
230-
if (value !== undefined) {
231+
if (hadValue) {
231232
this._stats.decreaseVSize(value);
232233
}
233234

@@ -256,6 +257,7 @@ export class NodeCacheStore<T> extends Hookified {
256257
public async clear(): Promise<void> {
257258
await this._keyv.clear();
258259
this._keys.clear();
260+
this._values.clear();
259261
this._ttls.clear();
260262
this._stats.resetStoreValues();
261263
}
@@ -267,6 +269,7 @@ export class NodeCacheStore<T> extends Hookified {
267269
public async flushAll(): Promise<void> {
268270
await this._keyv.clear();
269271
this._keys.clear();
272+
this._values.clear();
270273
this._ttls.clear();
271274
this._stats = new Stats({ enabled: true });
272275
this.emit("flush");
@@ -283,8 +286,8 @@ export class NodeCacheStore<T> extends Hookified {
283286
ttl?: number | string,
284287
): Promise<boolean> {
285288
const keyValue = key.toString();
286-
const item = await this._keyv.get(keyValue);
287-
if (item !== undefined) {
289+
if (this._values.has(keyValue)) {
290+
const item = this._values.get(keyValue) as T;
288291
const finalTtl = this.resolveTtl(ttl);
289292
await this._keyv.set(keyValue, item, finalTtl);
290293
this.trackExpiration(keyValue, finalTtl);
@@ -361,9 +364,10 @@ export class NodeCacheStore<T> extends Hookified {
361364
}
362365

363366
const result = await this._keyv.get<V>(keyValue);
364-
if (result !== undefined) {
367+
if (this._values.has(keyValue)) {
365368
await this._keyv.delete(keyValue);
366369
this._keys.delete(keyValue);
370+
this._values.delete(keyValue);
367371
this._ttls.delete(keyValue);
368372
this._stats.incrementHits();
369373
this._stats.decreaseKSize(keyValue);
@@ -490,7 +494,8 @@ export class NodeCacheStore<T> extends Hookified {
490494
return;
491495
}
492496

493-
const value = await this._keyv.get(key);
497+
const hadValue = this._values.has(key);
498+
const value = hadValue ? this._values.get(key) : undefined;
494499

495500
/* v8 ignore next 3 -- @preserve: race condition guard when key is refreshed during async get */
496501
if (!this.isExpired(key)) {
@@ -502,11 +507,11 @@ export class NodeCacheStore<T> extends Hookified {
502507
if (this._deleteOnExpire) {
503508
await this._keyv.delete(key);
504509
this._keys.delete(key);
510+
this._values.delete(key);
505511
this._ttls.delete(key);
506512
if (wasTracked) {
507513
this._stats.decreaseKSize(key);
508-
/* v8 ignore next 3 -- @preserve: value is typically undefined when Keyv also expires the item */
509-
if (value !== undefined) {
514+
if (hadValue) {
510515
this._stats.decreaseVSize(value);
511516
}
512517

‎packages/node-cache/test/index.test.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,21 @@ describe("NodeCache", () => {
124124
expect(cache.take(faker.string.uuid())).toBe(undefined);
125125
});
126126

127+
test("should take falsy values and clear stats", () => {
128+
const cache = new NodeCache({ checkperiod: 0 });
129+
const key = faker.string.uuid();
130+
cache.set(key, 0);
131+
132+
const taken = cache.take(key);
133+
expect(taken).toBe(0);
134+
expect(cache.has(key)).toBe(false);
135+
136+
const stats = cache.getStats();
137+
expect(stats.keys).toBe(0);
138+
expect(stats.ksize).toBe(0);
139+
expect(stats.vsize).toBe(0);
140+
});
141+
127142
test("should delete a key", () => {
128143
const key = faker.string.uuid();
129144
cache.set(key, faker.lorem.word());
@@ -330,6 +345,51 @@ describe("NodeCache", () => {
330345
expect(cache.getStats().keys).toBe(0);
331346
});
332347

348+
test("should not inflate ksize/vsize when overwriting an existing key", () => {
349+
const key = faker.string.uuid();
350+
const firstValue = faker.lorem.word();
351+
const secondValue = faker.lorem.words(6);
352+
353+
const overwrittenCache = new NodeCache<string>({ checkperiod: 0 });
354+
overwrittenCache.set(key, firstValue);
355+
overwrittenCache.set(key, secondValue);
356+
357+
const baselineCache = new NodeCache<string>({ checkperiod: 0 });
358+
baselineCache.set(key, secondValue);
359+
360+
const overwrittenStats = overwrittenCache.getStats();
361+
const baselineStats = baselineCache.getStats();
362+
363+
expect(overwrittenStats.keys).toBe(1);
364+
expect(overwrittenStats.ksize).toBe(baselineStats.ksize);
365+
expect(overwrittenStats.vsize).toBe(baselineStats.vsize);
366+
367+
overwrittenCache.del(key);
368+
const statsAfterDelete = overwrittenCache.getStats();
369+
expect(statsAfterDelete.keys).toBe(0);
370+
expect(statsAfterDelete.ksize).toBe(0);
371+
expect(statsAfterDelete.vsize).toBe(0);
372+
});
373+
374+
test("should fully clear ksize/vsize after overwritten key expires", async () => {
375+
const cache = new NodeCache<string>({
376+
checkperiod: 0,
377+
deleteOnExpire: true,
378+
});
379+
const key = faker.string.uuid();
380+
381+
cache.set(key, faker.lorem.word());
382+
cache.set(key, faker.lorem.word(), 0.05);
383+
384+
await sleep(100);
385+
expect(cache.get(key)).toBe(undefined);
386+
387+
const stats = cache.getStats();
388+
expect(stats.keys).toBe(0);
389+
expect(stats.ksize).toBe(0);
390+
expect(stats.vsize).toBe(0);
391+
});
392+
333393
test("should flush all the keys", () => {
334394
const cache = new NodeCache({ checkperiod: 0 });
335395
const key1 = faker.string.uuid();

‎packages/node-cache/test/store.test.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -906,6 +906,37 @@ describe("NodeCacheStore - overwrite stats", () => {
906906
const stats = store.getStats();
907907
expect(stats.keys).toBe(1);
908908
});
909+
910+
test("should not leak vsize when overwriting undefined value", async () => {
911+
const store = new NodeCacheStore();
912+
const key = faker.string.uuid();
913+
914+
await store.set(key, undefined as unknown);
915+
await store.set(key, faker.lorem.word());
916+
917+
const overwriteStats = store.getStats();
918+
expect(overwriteStats.keys).toBe(1);
919+
920+
await store.del(key);
921+
const afterDeleteStats = store.getStats();
922+
expect(afterDeleteStats.keys).toBe(0);
923+
expect(afterDeleteStats.ksize).toBe(0);
924+
expect(afterDeleteStats.vsize).toBe(0);
925+
});
926+
927+
test("should not leak vsize when undefined value expires", async () => {
928+
const store = new NodeCacheStore({ checkperiod: 0, deleteOnExpire: true });
929+
const key = faker.string.uuid();
930+
931+
await store.set(key, undefined as unknown, 50);
932+
await sleep(100);
933+
934+
expect(await store.get(key)).toBeUndefined();
935+
const stats = store.getStats();
936+
expect(stats.keys).toBe(0);
937+
expect(stats.ksize).toBe(0);
938+
expect(stats.vsize).toBe(0);
939+
});
909940
});
910941

911942
describe("NodeCacheStore - setTtl with falsy values", () => {

0 commit comments

Comments
 (0)