isaacs / node-lru-cache

A fast cache that automatically deletes the least recently used items
http://isaacs.github.io/node-lru-cache/
ISC License
5.38k stars 353 forks source link

fix evict-while-fetch() causing data structure corruption #270

Closed nornagon closed 1 year ago

nornagon commented 1 year ago

Before this fix, this new test failed with:

 FAIL  test/fetch.ts
 ✖ should be equal

  t.equal(c.size, 2)
  t.equal([...c].length, 2)
----^
})

  test: test/fetch.ts placeholder promise is not removed when resolving
  found: 0
  wanted: 2
  compare: ===
  at:
    line: 590
    column: 5
    file: test/fetch.ts
  stack: |
    test/fetch.ts:590:5
    fulfilled (test/fetch.ts:5:58)
isaacs commented 1 year ago

Fascinating. I'll dig into this tonight, it's not immediately clear to me why moving the moveToTail to the start of the operation would have that effect, but your test looks valid, so that's likely a good bug, thanks! :)

nornagon commented 1 year ago

It’s so that the eviction that happens in addSize will remove the current value last (so hopefully won’t evict the current item, unless its size is larger than the max!)

On Thu, Feb 16, 2023 at 19:09 isaacs @.***> wrote:

Fascinating. I'll dig into this tonight, it's not immediately clear to me why moving the moveToTail to the start of the operation would have that effect, but your test looks valid, so that's likely a good bug, thanks! :)

— Reply to this email directly, view it on GitHub https://github.com/isaacs/node-lru-cache/pull/270#issuecomment-1434035023, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABKGADX3CWNM7SG4GWL5WTWX3TVZANCNFSM6AAAAAAU6VXCMI . You are receiving this because you authored the thread.Message ID: @.***>

--

j

isaacs commented 1 year ago

Ah! 🤦 yes. Good catch, thanks. I'll get this in soon.

isaacs commented 1 year ago

Published on v7.16.1.

Thanks!