Selki/ff3396 expiring bandit cache

src/cache/lru-cache.spec.ts

@@ -61,4 +61,19 @@ describe('LRUCache', () => {
     expect(oneCache.get('a')).toBeFalsy();
     expect(oneCache.get('b')).toBe('banana');
   });
+
+  /**
+   This test case might be an overkill but in case Map() changes,
+   or we want to ditch it completely this will remind us that insertion
+   order is crucial for this cache to work properly
+   **/
+  it('should preserve insertion order when inserting on capacity limit', () => {
+    cache.set('a', 'apple');
+    cache.set('b', 'banana');
+    cache.set('c', 'cherry');
+
+    const keys = Array.from(cache.keys());
+    expect(keys[0]).toBe('b');
+    expect(keys[1]).toBe('c');
+  });
 });

src/cache/tlru-cache.spec.ts

@@ -22,22 +22,65 @@ describe('TLRU Cache', () => {
     expect(cache.get('a')).toBeUndefined();
   });
 
-  it('should evict all expired entries', () => {
+  it('should not evict cache before expiration', () => {
+    jest.useFakeTimers();
+
+    cache.set('a', 'apple');
+    jest.advanceTimersByTime(expectedCacheTimeoutMs - 1);
+    expect(cache.get('a')).toBe('apple');
+  });
+
+  it('should evict all expired entries on .entries() call', () => {
     jest.useFakeTimers();
 
     cache.set('a', 'avocado');
     jest.advanceTimersByTime(expectedCacheTimeoutMs);
     cache.set('b', 'banana');
     jest.advanceTimersByTime(expectedCacheTimeoutMs);
 
-    expect(cache.get('b')).toBeUndefined();
-    expect(cache.get('a')).toBeUndefined();
+    const cacheEntries = [];
+
+    for (const entry of cache.entries()) {
+      cacheEntries.push(entry);
+    }
+
+    expect(cacheEntries.length).toBe(0);
+  });
+
+  it('should evict all expired entries on .keys() call', () => {
+    jest.useFakeTimers();
+
+    cache.set('a', 'avocado');
+    jest.advanceTimersByTime(expectedCacheTimeoutMs);
+    cache.set('b', 'banana');
+    jest.advanceTimersByTime(expectedCacheTimeoutMs);
+
+    const cacheKeys = [];
+
+    for (const key of cache.keys()) {
+      cacheKeys.push(key);
+    }
+
+    expect(cacheKeys.length).toBe(0);
+  });
+
+  it('should evict all expired entries on .values() call', () => {
+    jest.useFakeTimers();
+
+    cache.set('a', 'avocado');
+    jest.advanceTimersByTime(expectedCacheTimeoutMs);
+    cache.set('b', 'banana');
+    jest.advanceTimersByTime(expectedCacheTimeoutMs);
+
+    const cacheValues = [];
+
+    for (const value of cache.values()) {
+      cacheValues.push(value);
+    }
+
+    expect(cacheValues.length).toBe(0);
   });
 
-  /**
-   * This test assumes implementation which is not ideal, but that's
-   * the only way I know of how to go around timers in jest
-   **/
   it('should overwrite existing cache entry', () => {
     jest.useFakeTimers();
 
@@ -88,4 +131,29 @@ describe('TLRU Cache', () => {
     expect(oneCache.get('a')).toBeFalsy();
     expect(oneCache.get('b')).toBe('banana');
   });
+
+  it('should evict oldest entry when capacity limit is reached', () => {
+    cache.set('a', 'apple');
+    cache.set('b', 'banana');
+    cache.set('c', 'cherry');
+
+    expect(cache.get('a')).toBeUndefined();
+    expect(cache.has('b')).toBeTruthy();
+    expect(cache.has('c')).toBeTruthy();
+  });
+
+  /**
+    This test case might be an overkill but in case Map() changes,
+    or we want to ditch it completely this will remind us that insertion
+    order is crucial for this cache to work properly
+  **/
+  it('should preserve insertion order when inserting on capacity limit', () => {
+    cache.set('a', 'apple');
+    cache.set('b', 'banana');
+    cache.set('c', 'cherry');
+
+    const keys = Array.from(cache.keys());
+    expect(keys[0]).toBe('b');
+    expect(keys[1]).toBe('c');
+  });
 });

src/cache/tlru-cache.ts

@@ -22,6 +22,12 @@ export class TLRUCache extends LRUCache {
     }
   }
 
+  private isCacheEntryValid(key: string): boolean {
+    const now = new Date(Date.now());
+    const evictionDate = this.cacheEntriesTTLRegistry.get(key);
+    return evictionDate !== undefined ? now < evictionDate : false;
+  }
+
   private setCacheEntryEvictionTime(key: string): void {
     this.cacheEntriesTTLRegistry.set(key, this.getCacheEntryEvictionTime());
   }
@@ -32,32 +38,65 @@ export class TLRUCache extends LRUCache {
   }
 
   private evictExpiredCacheEntries() {
-    const now = new Date(Date.now());
     let cacheKey: string;
-    let evictionDate: Date;
 
     // Not using this.cache.forEach so we can break the loop once
     // we find the fist non-expired entry. Each entry after that
-    // is guaranteed to also be non-expired, because they are oldest->newest
-    for ([cacheKey, evictionDate] of this.cacheEntriesTTLRegistry.entries()) {
-      if (now >= evictionDate) {
+    // is guaranteed to also be non-expired, because iteration happens
+    // in insertion order
+    for (cacheKey of this.cache.keys()) {
+      if (!this.isCacheEntryValid(cacheKey)) {
         this.delete(cacheKey);
       } else {
         break;
       }
     }
   }
 
+  entries(): IterableIterator<[string, string]> {
+    this.evictExpiredCacheEntries();
+    return super.entries();
+  }
+
+  keys(): IterableIterator<string> {
+    this.evictExpiredCacheEntries();
+    return super.keys();
+  }
+
+  values(): IterableIterator<string> {
+    this.evictExpiredCacheEntries();
+    return super.values();
+  }
+
   delete(key: string): boolean {
     this.clearCacheEntryEvictionTimeIfExists(key);
     return super.delete(key);
   }
 
+  // has(key: string): boolean {
+  //   const hasValue = this.cache.has(key);
+  //
+  //   if (!this.isCacheEntryValid(key)) {
+  //     this.delete(key);
+  //     return false;
+  //   }
+  //
+  //   return hasValue;
+  // }
+
   get(key: string): string | undefined {
-    this.evictExpiredCacheEntries();
+    if (!this.cache.has(key)) {
+      return undefined;
+    }
+
+    const value = this.cache.get(key);
 
-    const value = super.get(key);
     if (value !== undefined) {
+      if (!this.isCacheEntryValid(key)) {
+        this.delete(key);
+        return undefined;
+      }
+
       // Whenever we get a cache hit, we need to reset the timer
       // for eviction, because it is now considered most recently
       // accessed thus the timer should start over. Not doing that
@@ -68,8 +107,6 @@ export class TLRUCache extends LRUCache {
   }
 
   set(key: string, value: string): this {
-    this.evictExpiredCacheEntries();
-
     const cache = super.set(key, value);
     this.resetCacheEntryEvictionTime(key);
     return cache;