overlarge items should not be added to cache

Fixes a bug where a large item being added to the cache would cause
everything to be evicted, including the item being added, but the
`sizes` array was not updated properly.

Author: isaacs

README.md

@@ -114,6 +114,10 @@ the cache, and automatically evict items in order to stay below
 this size.  Note that this may result in fewer than `max` items
 being stored.
 
+Attempting to add an item to the cache whose calculated size is
+greater that this amount will be a no-op.  The item will not be
+cached, and no other items will be evicted.
+
 Optional, must be a positive integer if provided.  Required if
 other size tracking features are used.
 
@@ -134,7 +138,9 @@ argument, and the key is passed as the second argument.
 This may be overridden by passing an options object to
 `cache.set()`.
 
-Requires `maxSize` to be set.
+Requires `maxSize` to be set.  If the resulting calculated size
+is greater than `maxSize`, then the item will not be added to the
+cache.
 
 Deprecated alias: `length`
 
@@ -437,6 +443,9 @@ the `sizeCalculation` function and just use the specified number
 if it is a positive integer, and `noDisposeOnSet` which will
 prevent calling a `dispose` function in the case of overwrites.
 
+If the `size` (or return value of `sizeCalculation`) is greater
+than `maxSize`, then the item will not be added to the cache.
+
 Will update the recency of the entry.
 
 Returns the cache object.

index.js

@@ -364,8 +364,10 @@ class LRUCache {
   initializeSizeTracking() {
     this.calculatedSize = 0
     this.sizes = new ZeroArray(this.max)
-    this.removeItemSize = index =>
-      (this.calculatedSize -= this.sizes[index])
+    this.removeItemSize = index => {
+      this.calculatedSize -= this.sizes[index]
+      this.sizes[index] = 0
+    }
     this.requireSize = (k, v, size, sizeCalculation) => {
       if (!isPosInt(size)) {
         if (sizeCalculation) {
@@ -386,7 +388,7 @@ class LRUCache {
       }
       return size
     }
-    this.addItemSize = (index, v, k, size) => {
+    this.addItemSize = (index, size) => {
       this.sizes[index] = size
       const maxSize = this.maxSize - this.sizes[index]
       while (this.calculatedSize > maxSize) {
@@ -396,7 +398,7 @@ class LRUCache {
     }
   }
   removeItemSize(index) {}
-  addItemSize(index, v, k, size) {}
+  addItemSize(index, size) {}
   requireSize(k, v, size, sizeCalculation) {
     if (size || sizeCalculation) {
       throw new TypeError(
@@ -569,6 +571,10 @@ class LRUCache {
     } = {}
   ) {
     size = this.requireSize(k, v, size, sizeCalculation)
+    // if the item doesn't fit, don't do anything
+    if (this.maxSize && size > this.maxSize) {
+      return this
+    }
     let index = this.size === 0 ? undefined : this.keyMap.get(k)
     if (index === undefined) {
       // addition
@@ -580,7 +586,7 @@ class LRUCache {
       this.prev[index] = this.tail
       this.tail = index
       this.size++
-      this.addItemSize(index, v, k, size)
+      this.addItemSize(index, size)
       noUpdateTTL = false
     } else {
       // update
@@ -598,7 +604,7 @@ class LRUCache {
         }
         this.removeItemSize(index)
         this.valList[index] = v
-        this.addItemSize(index, v, k, size)
+        this.addItemSize(index, size)
       }
       this.moveToTail(index)
     }

test/size-calculation.ts

@@ -1,50 +1,84 @@
 import t from 'tap'
 import LRU from '../'
 
+const checkSize = (c:LRU<any,any>) => {
+  const sizes = (c as unknown as { sizes: number[] }).sizes
+  const {calculatedSize, maxSize} = c
+  const sum = [...sizes].reduce((a, b) => a + b, 0)
+  if (sum !== calculatedSize) {
+    console.error({sum, calculatedSize, sizes})
+    throw new Error('calculatedSize does not equal sum of sizes')
+  }
+  if (calculatedSize > maxSize) {
+    throw new Error('max size exceeded')
+  }
+}
+
 t.test('store strings, size = length', t => {
   const c = new LRU<any, string>({
     max: 100,
     maxSize: 100,
     sizeCalculation: n => n.length,
   })
 
+  checkSize(c)
   c.set(5, 'x'.repeat(5))
+  checkSize(c)
   c.set(10, 'x'.repeat(10))
+  checkSize(c)
   c.set(20, 'x'.repeat(20))
+  checkSize(c)
   t.equal(c.calculatedSize, 35)
   c.delete(20)
+  checkSize(c)
   t.equal(c.calculatedSize, 15)
   c.delete(5)
+  checkSize(c)
   t.equal(c.calculatedSize, 10)
   c.clear()
+  checkSize(c)
   t.equal(c.calculatedSize, 0)
 
   const s = 'x'.repeat(10)
   for (let i = 0; i < 5; i++) {
     c.set(i, s)
+    checkSize(c)
   }
   t.equal(c.calculatedSize, 50)
 
   // the big item goes in, but triggers a prune
   // we don't preemptively prune until we *cross* the max
   c.set('big', 'x'.repeat(100))
+  checkSize(c)
   t.equal(c.calculatedSize, 100)
   // override the size on set
   c.set('big', 'y'.repeat(100), { sizeCalculation: () => 10 })
+  checkSize(c)
   t.equal(c.size, 1)
+  checkSize(c)
   t.equal(c.calculatedSize, 10)
+  checkSize(c)
   c.delete('big')
+  checkSize(c)
   t.equal(c.size, 0)
   t.equal(c.calculatedSize, 0)
 
   c.set('repeated', 'i'.repeat(10))
+  checkSize(c)
   c.set('repeated', 'j'.repeat(10))
+  checkSize(c)
   c.set('repeated', 'i'.repeat(10))
+  checkSize(c)
   c.set('repeated', 'j'.repeat(10))
+  checkSize(c)
   c.set('repeated', 'i'.repeat(10))
+  checkSize(c)
   c.set('repeated', 'j'.repeat(10))
+  checkSize(c)
   c.set('repeated', 'i'.repeat(10))
+  checkSize(c)
   c.set('repeated', 'j'.repeat(10))
+  checkSize(c)
   t.equal(c.size, 1)
   t.equal(c.calculatedSize, 10)
   t.equal(c.get('repeated'), 'j'.repeat(10))
@@ -81,29 +115,84 @@ t.test('bad size calculation fn throws on set()', t => {
 
 t.test('delete while empty, or missing key, is no-op', t => {
   const c = new LRU({ max: 5, maxSize: 10, sizeCalculation: () => 2 })
+  checkSize(c)
   c.set(1, 1)
+  checkSize(c)
   t.equal(c.size, 1)
   t.equal(c.calculatedSize, 2)
   c.clear()
+  checkSize(c)
   t.equal(c.size, 0)
   t.equal(c.calculatedSize, 0)
   c.delete(1)
+  checkSize(c)
   t.equal(c.size, 0)
   t.equal(c.calculatedSize, 0)
 
   c.set(1, 1)
+  checkSize(c)
   c.set(1, 1)
+  checkSize(c)
   c.set(1, 1)
+  checkSize(c)
   t.equal(c.size, 1)
   t.equal(c.calculatedSize, 2)
   c.delete(99)
+  checkSize(c)
   t.equal(c.size, 1)
   t.equal(c.calculatedSize, 2)
   c.delete(1)
+  checkSize(c)
   t.equal(c.size, 0)
   t.equal(c.calculatedSize, 0)
   c.delete(1)
+  checkSize(c)
   t.equal(c.size, 0)
   t.equal(c.calculatedSize, 0)
   t.end()
 })
+
+t.test('large item falls out of cache, sizes are kept correct', t => {
+  const c = new LRU<number, number>({
+    maxSize: 10,
+    sizeCalculation: () => 100,
+  })
+  const sizes:number[] = (c as unknown as { sizes: number[] }).sizes
+
+  checkSize(c)
+  t.equal(c.size, 0)
+  t.equal(c.calculatedSize, 0)
+  t.same(sizes, [])
+
+  c.set(2, 2, { size: 2 })
+  checkSize(c)
+  t.equal(c.size, 1)
+  t.equal(c.calculatedSize, 2)
+  t.same(sizes, [2])
+
+  c.delete(2)
+  checkSize(c)
+  t.equal(c.size, 0)
+  t.equal(c.calculatedSize, 0)
+  t.same(sizes, [0])
+
+  c.set(1, 1)
+  checkSize(c)
+  t.equal(c.size, 0)
+  t.equal(c.calculatedSize, 0)
+  t.same(sizes, [0])
+
+  c.set(3, 3, { size: 3 })
+  checkSize(c)
+  t.equal(c.size, 1)
+  t.equal(c.calculatedSize, 3)
+  t.same(sizes, [3])
+
+  c.set(4, 4)
+  checkSize(c)
+  t.equal(c.size, 1)
+  t.equal(c.calculatedSize, 3)
+  t.same(sizes, [3])
+
+  t.end()
+})