diff --git a/src/index.ts b/src/index.ts index d7168d8f..cbe9f157 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1140,7 +1140,12 @@ export class GossipSub extends EventEmitter implements PubSub { return this.entries.size } - put(key: string | number, value: T): void { + /** Returns true if there was a key collision and the entry is dropped */ + put(key: string | number, value: T): boolean { + if (this.entries.has(key)) { + // Key collisions break insertion order in the entries cache, which break prune logic. + // prune relies on each iterated entry to have strictly ascending validUntilMs, else it + // won't prune expired entries and SimpleTimeCache will grow unexpectedly. + // As of Oct 2022 NodeJS v16, inserting the same key twice with different value does not + // change the key position in the iterator stream. A unit test asserts this behaviour. + return true + } + this.entries.set(key, { value, validUntilMs: Date.now() + this.validityMs }) + return false } prune(): void { @@ -38,7 +49,8 @@ export class SimpleTimeCache { if (v.validUntilMs < now) { this.entries.delete(k) } else { - // sort by insertion order + // Entries are inserted with strictly ascending validUntilMs. + // Stop early to save iterations break } } diff --git a/test/time-cache.spec.ts b/test/time-cache.spec.ts index aa673831..079d578f 100644 --- a/test/time-cache.spec.ts +++ b/test/time-cache.spec.ts @@ -40,4 +40,28 @@ describe('SimpleTimeCache', () => { expect(timeCache.has('bFirst')).to.be.false() expect(timeCache.has('cFirst')).to.be.false() }) + + it('Map insertion order', () => { + const key1 = 'key1' + const key2 = 'key2' + const key3 = 'key3' + + const map = new Map() + map.set(key1, Date.now()) + map.set(key2, Date.now()) + map.set(key3, Date.now()) + + expect(Array.from(map.keys())).deep.equals([key1, key2, key3], 'Map iterator order') + + // Does not change key position + map.set(key2, Date.now()) + + expect(Array.from(map.keys())).deep.equals([key1, key2, key3], 'Map iterator order after re-set') + + // Changes key position + map.delete(key2) + map.set(key2, Date.now()) + + expect(Array.from(map.keys())).deep.equals([key1, key3, key2], 'Map iterator order after delete set') + }) })