kibertoad / toad-cache

In-memory cache for Node.js and browser
MIT License
17 stars 5 forks source link

Bug when calling `get` on only entry in `LruObject` #32

Closed tobiasdcl closed 11 months ago

tobiasdcl commented 11 months ago

Hey folks, first of all thanks for building and maintaining this library ❤️


I stumbled across this issue when investigating failures coming from fastify-rate-limit. I created an issue there because thats where I noticed the impact first.

However based on my current understanding there is an issue in the LruObject implementation - more specifically in the get implementation. Here is a simple test case that I wrote:

  describe('get', () => {
    it('does not overwrite cache.first', async () => {
      cache = new LruObject(5, 500)

      const key = '10.0.0.1'
      const value = 100

      cache.set(key, value)
      expect(cache.first).not.toBeNull()

      cache.get(key)
      expect(cache.first).not.toBeNull()
    })
  })
 FAIL  test/LruObject.spec.js > LruObject > get > does not overwrite cache.first
AssertionError: expected null not to be null
 ❯ test/LruObject.spec.js:77:31
     75| 
     76|       cache.get(key)
     77|       expect(cache.first).not.toBeNull()
       |                               ^
     78|     })
     79| 

Can you confirm that this is indeed not intended?

tobiasdcl commented 11 months ago

this is especially problematic once the cache limit is reached as the evict() call results in a failure

  describe('get', () => {
    it('does not cause TypeError when reaching the cache limit', async () => {
      const maxCacheSize = 3
      cache = new LruObject(maxCacheSize, 500)

      const key = '10.0.0.1'
      const value = 100

      cache.set(key, value)
      cache.get(key)

      for (let i = 0; i < maxCacheSize; i++) {
        cache.set(i, i)
      }
    })
  })
 FAIL  test/LruObject.spec.js > LruObject > get > does not cause TypeError when reaching the cache limit
TypeError: Cannot read properties of null (reading 'key')
 ❯ LruObject.evict src/LruObject.js:85:30
     83|       const item = this.first
     84| 
     85|       delete this.items[item.key]
       |                              ^
     86| 
     87|       if (--this.size === 0) {
 ❯ LruObject.set src/LruObject.js:149:12
 ❯ test/LruObject.spec.js:91:15