Skip to content

fix: stop iteration from revisiting entries moved by get() - #411

Open
chuanghiduoc wants to merge 1 commit into
isaacs:mainfrom
chuanghiduoc:fix/iteration-revisit-loop
Open

chuanghiduoc wants to merge 1 commit into
isaacs:mainfrom
chuanghiduoc:fix/iteration-revisit-loop

Conversation

@chuanghiduoc

Copy link
Copy Markdown

Iterating with forEach/keys()/entries()/values() while calling get() on the visited entry moves it to the MRU end via #moveToTail. The index walkers follow the re-linked prev/next pointers and land back on nodes they already yielded, so the loop revisits the same entries forever until something throws:

const c = new LRUCache({ max: 3 })
c.set('a', 1); c.set('b', 2); c.set('c', 3)

for (const k of c.keys()) {
  c.get(k)
}
// visits c,b,c,b,c,b,... forever
// eventually: RangeError: Invalid array length

Walk-through of the two-entry cycle (#indexes, walking tail → head via #prev):

  1. visit c (tail), callback get('c') — no-op, already tail. Next: prev[c] = b.
  2. visit b, callback get('b') — moveToTail(b) unlinks b from between a and c, relinks a↔c, appends b after c (tail = b). List is now a ↔ c ↔ b.
  3. walker does i = prev[b] = c → visits c again, whose get restores the old order… and the walk oscillates between steps 2–3 indefinitely.

Deletes during iteration were already handled by the #isValidIndex check (the entry's index no longer maps back to its key); this covers the move case.

Fix

Track yielded indexes in #indexes / #rindexes and skip them if the walk circles back. Each entry is now visited at most once per iteration, matching Map iteration semantics.

The Set is allocated once per iteration call; for the common case where nothing mutates mid-walk, behavior and results are unchanged.

Tests

test/get-during-iteration.ts asserts that iterating while calling get(k) terminates and visits each key exactly once — fails on master (infinite loop), passes with this change.

Full suite: 18430 total / 18364 pass / 66 fail on this branch vs 18326 / 18258 / 68 on unmodified master. The only failing file in both runs is the pre-existing flaky test/tracing.ts; the remaining delta is subtests of my new test file plus run-to-run noise in the same tracing/fetch tests.

Iterating with forEach/keys()/entries()/values() while calling get() on
the visited entry moves it to the MRU end via #moveToTail. The index
walkers follow the re-linked prev/next pointers and land back on nodes
they already yielded, looping c,b,c,b... forever until something crashes
with 'RangeError: Invalid array length'.

Track yielded indexes in the walkers so each entry is visited at most
once per iteration. Deletes during iteration were already safe (the
existing #isValidIndex check); this covers the move case.

Repro:

  const c = new LRUCache({ max: 3 })
  c.set('a', 1); c.set('b', 2); c.set('c', 3)
  for (const k of c.keys()) { c.get(k) }
  // loops forever on c/b before this change

Related discussion: isaacs#301

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant