fix(runtime): Set/Map forEach — fused array-receiver reroute + delete-during-iteration#6215
Open
proggeramlug wants to merge 1 commit into
Open
fix(runtime): Set/Map forEach — fused array-receiver reroute + delete-during-iteration#6215proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
…-during-iteration Two forEach defects on native Sets/Maps, both hit by react-server-dom during Next.js App Router SSR: 1. `.forEach` on an unknown-typed receiver is statically fused to js_array_forEach; a Set/Map receiver (a collection stored on an object and read back as a property) was treated as an ArrayHeader, feeding hash-table internals to the callback and segfaulting on the first property read. react-server-dom sweeps `request.abortableTasks` (a Set) exactly this way. Detect registered Sets/Maps and route to the native forEach — the same pattern as the existing typed-array reroute; `forEach` is the only method name the fused array methods share with Set/Map. 2. Deleting during Set/Map forEach skipped the next entry: the backing vector compacts on delete, so naive index advancement jumped the shifted-in element. Only advance when the current slot still holds the just-visited entry (compared via a GC-rooted handle). Delete-current/earlier/later and delete-then-re-add now match ECMA-262; adds-during-iteration behavior is unchanged. Adds test_gap_set_map_foreach_fused_receiver.ts, byte-for-byte against node.
📝 WalkthroughWalkthroughThe runtime now redirects unknown-typed Set and Map receivers to native ChangesSet/Map
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test-files/test_gap_set_map_foreach_fused_receiver.ts (1)
26-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider verifying Map forEach third callback argument.
Test (3) verifies the Set
forEachthird argument (theSet) but there is no equivalent check for MapforEach. The upstream contract (map.rs:1918-1968) passesmap_valueas the third callback argument, and the subclass dispatch (collection_methods.rs) preserves the collection instance identity. Adding a third-argument assertion for Map would close this gap.➕ Proposed addition to test (2)
const holder: any = { cache: new Map() }; holder.cache.set("a", { n: 1 }); holder.cache.set("b", { n: 2 }); const pairs: string[] = []; -holder.cache.forEach((v: any, k: any) => pairs.push(`${k}=${v.n}`)); +holder.cache.forEach((v: any, k: any, m: any) => { + pairs.push(`${k}=${v.n}`); + console.log(m === holder.cache, m.get("a").n); + }); console.log(pairs.join(","));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-files/test_gap_set_map_foreach_fused_receiver.ts` around lines 26 - 32, Add a third-argument assertion to the Map forEach test in the callback used on holder.cache so it verifies the passed collection is the same Map instance, mirroring the Set test. Use the existing holder.cache.forEach path and check the third callback parameter identity to confirm Map forEach preserves the map instance as described by the map/collection dispatch behavior.crates/perry-runtime/src/map.rs (1)
1947-1971: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid growing the runtime-handle stack in this loop
RuntimeHandleScope::root_nanbox_f64pushes a new root that stays live until the scope drops, so this outer-scope loop retains one extra handle per entry. A per-iteration scope, or reusing a single handle slot, would keep the root set flat.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/map.rs` around lines 1947 - 1971, The loop in the Map iteration logic is creating a new long-lived root via RuntimeHandleScope::root_nanbox_f64 for every entry, which grows the runtime-handle stack across the whole traversal. Update the code around the key_handle usage in the Map callback loop so the root is only held for the duration of each iteration, either by introducing a per-iteration scope or by reusing a single handle slot, while preserving the slot comparison that guards the i++ advance.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-files/test_gap_set_map_foreach_fused_receiver.ts`:
- Around line 1-80: The test file covers Set/Map forEach deletion and
receiver-reroute cases, but it is missing the add-during-iteration regression
that the PR objectives require. Add a new regression block alongside the
existing forEach cases that exercises js_set_foreach_impl and
js_map_foreach_impl by appending entries during the callback and verifying the
newly added items are visited in iteration order. Keep the scenario in the same
style as the existing Set/Map tests so future changes that accidentally snapshot
size will be caught.
---
Nitpick comments:
In `@crates/perry-runtime/src/map.rs`:
- Around line 1947-1971: The loop in the Map iteration logic is creating a new
long-lived root via RuntimeHandleScope::root_nanbox_f64 for every entry, which
grows the runtime-handle stack across the whole traversal. Update the code
around the key_handle usage in the Map callback loop so the root is only held
for the duration of each iteration, either by introducing a per-iteration scope
or by reusing a single handle slot, while preserving the slot comparison that
guards the i++ advance.
In `@test-files/test_gap_set_map_foreach_fused_receiver.ts`:
- Around line 26-32: Add a third-argument assertion to the Map forEach test in
the callback used on holder.cache so it verifies the passed collection is the
same Map instance, mirroring the Set test. Use the existing holder.cache.forEach
path and check the third callback parameter identity to confirm Map forEach
preserves the map instance as described by the map/collection dispatch behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 608dbace-78b3-4803-86bf-e115655c4b7b
📒 Files selected for processing (4)
crates/perry-runtime/src/array/iter_methods.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/set.rstest-files/test_gap_set_map_foreach_fused_receiver.ts
Comment on lines
+1
to
+80
| // `.forEach` on an unknown-typed receiver is statically fused to the ARRAY | ||
| // forEach entry point. When the receiver is actually a native Set or Map — | ||
| // e.g. a collection stored on an object and read back as a property — the | ||
| // fused call treated the SetHeader as an ArrayHeader, feeding hash-table | ||
| // internals to the callback as elements and segfaulting on the first property | ||
| // read. react-server-dom hits exactly this: `request.abortableTasks` is a Set | ||
| // it iterates via `.forEach`, reading `.status` off each task — this crashed | ||
| // every Next.js App Router dynamic route once the RSC flight started flowing | ||
| // (#5989). | ||
| // | ||
| // `forEach` is the only method name the fused array methods share with | ||
| // Set/Map, so the runtime reroute (mirroring the existing typed-array reroute) | ||
| // covers the hazard class. | ||
| // | ||
| // Validated byte-for-byte against `node --experimental-strip-types`. | ||
|
|
||
| // (1) the flight shape: Set stored on an object, read back, forEach'd | ||
| const req: any = { abortableTasks: new Set() }; | ||
| req.abortableTasks.add({ status: 10 }); | ||
| req.abortableTasks.add({ status: 11 }); | ||
| req.abortableTasks.add({ status: 12 }); | ||
| const got: any[] = []; | ||
| req.abortableTasks.forEach((t: any) => got.push(t.status)); | ||
| console.log(JSON.stringify(got), req.abortableTasks.size); | ||
|
|
||
| // (2) Map stored on an object — callback receives (value, key, map) | ||
| const holder: any = { cache: new Map() }; | ||
| holder.cache.set("a", { n: 1 }); | ||
| holder.cache.set("b", { n: 2 }); | ||
| const pairs: string[] = []; | ||
| holder.cache.forEach((v: any, k: any) => pairs.push(`${k}=${v.n}`)); | ||
| console.log(pairs.join(",")); | ||
|
|
||
| // (3) Set forEach argument order: (value, valueAgain, set) | ||
| const s: any = { s: new Set(["x"]) }; | ||
| s.s.forEach((v: any, v2: any, theSet: any) => | ||
| console.log(v === v2, theSet.has("x"), theSet.size), | ||
| ); | ||
|
|
||
| // (4) plain arrays through the same fused path stay correct | ||
| const arrHolder: any = { list: [7, 8] }; | ||
| const items: string[] = []; | ||
| arrHolder.list.forEach((v: any, i: any, a: any) => items.push(`${i}:${v}:${a.length}`)); | ||
| console.log(items.join(",")); | ||
|
|
||
| // (5) delete during Set.forEach (React deletes tasks while sweeping): the | ||
| // backing vector compacts on delete, so naive index advancement skipped the | ||
| // shifted-in next entry. | ||
| const req2: any = { tasks: new Set() }; | ||
| const t1 = { id: 1 }; | ||
| const t2 = { id: 2 }; | ||
| req2.tasks.add(t1); | ||
| req2.tasks.add(t2); | ||
| const seen: number[] = []; | ||
| req2.tasks.forEach((t: any) => { | ||
| seen.push(t.id); | ||
| req2.tasks.delete(t); | ||
| }); | ||
| console.log(JSON.stringify(seen), req2.tasks.size); | ||
|
|
||
| // (6) delete during Map.forEach — same compaction hazard | ||
| const m6: any = { m: new Map() }; | ||
| m6.m.set("a", 1); | ||
| m6.m.set("b", 2); | ||
| m6.m.set("c", 3); | ||
| const seen6: string[] = []; | ||
| m6.m.forEach((v: any, k: any) => { | ||
| seen6.push(`${k}:${v}`); | ||
| m6.m.delete(k); | ||
| }); | ||
| console.log(JSON.stringify(seen6), m6.m.size); | ||
|
|
||
| // (7) delete an EARLIER entry during iteration (must not skip or re-visit) | ||
| const s7 = new Set(["p", "q", "r"]); | ||
| const seen7: string[] = []; | ||
| s7.forEach((v: any) => { | ||
| seen7.push(v); | ||
| if (v === "q") s7.delete("p"); | ||
| }); | ||
| console.log(JSON.stringify(seen7), s7.size); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚖️ Poor tradeoff
Missing add-during-iteration regression test claimed in PR objectives.
The PR objectives state the test file covers "add-during-iteration regression cases," but no such test exists. The upstream forEach implementations (both js_map_foreach_impl and js_set_foreach_impl) explicitly re-read (*map).size / (*set).size each iteration so that entries appended during the callback are visited per ECMA-262. Without a regression test, a future change that snapshots the initial size could silently break this contract.
Suggested addition:
➕ Proposed add-during-iteration tests
// (8) add during Set.forEach — entries appended in the callback MUST be visited
const s8 = new Set([1]);
const seen8: number[] = [];
s8.forEach((v: any) => {
seen8.push(v);
if (v < 3) s8.add(v + 1);
});
console.log(JSON.stringify(seen8), s8.size);
// Expected: [1,2,3] 3
// (9) add during Map.forEach — same contract
const m9 = new Map([["a", 1]]);
const seen9: string[] = [];
m9.forEach((v: any, k: any) => {
seen9.push(`${k}:${v}`);
if (v < 3) m9.set(String.fromCharCode(k.charCodeAt(0) + 1), v + 1);
});
console.log(JSON.stringify(seen9), m9.size);
// Expected: ["a:1","b:2","c:3"] 3📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // `.forEach` on an unknown-typed receiver is statically fused to the ARRAY | |
| // forEach entry point. When the receiver is actually a native Set or Map — | |
| // e.g. a collection stored on an object and read back as a property — the | |
| // fused call treated the SetHeader as an ArrayHeader, feeding hash-table | |
| // internals to the callback as elements and segfaulting on the first property | |
| // read. react-server-dom hits exactly this: `request.abortableTasks` is a Set | |
| // it iterates via `.forEach`, reading `.status` off each task — this crashed | |
| // every Next.js App Router dynamic route once the RSC flight started flowing | |
| // (#5989). | |
| // | |
| // `forEach` is the only method name the fused array methods share with | |
| // Set/Map, so the runtime reroute (mirroring the existing typed-array reroute) | |
| // covers the hazard class. | |
| // | |
| // Validated byte-for-byte against `node --experimental-strip-types`. | |
| // (1) the flight shape: Set stored on an object, read back, forEach'd | |
| const req: any = { abortableTasks: new Set() }; | |
| req.abortableTasks.add({ status: 10 }); | |
| req.abortableTasks.add({ status: 11 }); | |
| req.abortableTasks.add({ status: 12 }); | |
| const got: any[] = []; | |
| req.abortableTasks.forEach((t: any) => got.push(t.status)); | |
| console.log(JSON.stringify(got), req.abortableTasks.size); | |
| // (2) Map stored on an object — callback receives (value, key, map) | |
| const holder: any = { cache: new Map() }; | |
| holder.cache.set("a", { n: 1 }); | |
| holder.cache.set("b", { n: 2 }); | |
| const pairs: string[] = []; | |
| holder.cache.forEach((v: any, k: any) => pairs.push(`${k}=${v.n}`)); | |
| console.log(pairs.join(",")); | |
| // (3) Set forEach argument order: (value, valueAgain, set) | |
| const s: any = { s: new Set(["x"]) }; | |
| s.s.forEach((v: any, v2: any, theSet: any) => | |
| console.log(v === v2, theSet.has("x"), theSet.size), | |
| ); | |
| // (4) plain arrays through the same fused path stay correct | |
| const arrHolder: any = { list: [7, 8] }; | |
| const items: string[] = []; | |
| arrHolder.list.forEach((v: any, i: any, a: any) => items.push(`${i}:${v}:${a.length}`)); | |
| console.log(items.join(",")); | |
| // (5) delete during Set.forEach (React deletes tasks while sweeping): the | |
| // backing vector compacts on delete, so naive index advancement skipped the | |
| // shifted-in next entry. | |
| const req2: any = { tasks: new Set() }; | |
| const t1 = { id: 1 }; | |
| const t2 = { id: 2 }; | |
| req2.tasks.add(t1); | |
| req2.tasks.add(t2); | |
| const seen: number[] = []; | |
| req2.tasks.forEach((t: any) => { | |
| seen.push(t.id); | |
| req2.tasks.delete(t); | |
| }); | |
| console.log(JSON.stringify(seen), req2.tasks.size); | |
| // (6) delete during Map.forEach — same compaction hazard | |
| const m6: any = { m: new Map() }; | |
| m6.m.set("a", 1); | |
| m6.m.set("b", 2); | |
| m6.m.set("c", 3); | |
| const seen6: string[] = []; | |
| m6.m.forEach((v: any, k: any) => { | |
| seen6.push(`${k}:${v}`); | |
| m6.m.delete(k); | |
| }); | |
| console.log(JSON.stringify(seen6), m6.m.size); | |
| // (7) delete an EARLIER entry during iteration (must not skip or re-visit) | |
| const s7 = new Set(["p", "q", "r"]); | |
| const seen7: string[] = []; | |
| s7.forEach((v: any) => { | |
| seen7.push(v); | |
| if (v === "q") s7.delete("p"); | |
| }); | |
| console.log(JSON.stringify(seen7), s7.size); | |
| // `.forEach` on an unknown-typed receiver is statically fused to the ARRAY | |
| // forEach entry point. When the receiver is actually a native Set or Map — | |
| // e.g. a collection stored on an object and read back as a property — the | |
| // fused call treated the SetHeader as an ArrayHeader, feeding hash-table | |
| // internals to the callback as elements and segfaulting on the first property | |
| // read. react-server-dom hits exactly this: `request.abortableTasks` is a Set | |
| // it iterates via `.forEach`, reading `.status` off each task — this crashed | |
| // every Next.js App Router dynamic route once the RSC flight started flowing | |
| // (`#5989`). | |
| // | |
| // `forEach` is the only method name the fused array methods share with | |
| // Set/Map, so the runtime reroute (mirroring the existing typed-array reroute) | |
| // covers the hazard class. | |
| // | |
| // Validated byte-for-byte against `node --experimental-strip-types`. | |
| // (1) the flight shape: Set stored on an object, read back, forEach'd | |
| const req: any = { abortableTasks: new Set() }; | |
| req.abortableTasks.add({ status: 10 }); | |
| req.abortableTasks.add({ status: 11 }); | |
| req.abortableTasks.add({ status: 12 }); | |
| const got: any[] = []; | |
| req.abortableTasks.forEach((t: any) => got.push(t.status)); | |
| console.log(JSON.stringify(got), req.abortableTasks.size); | |
| // (2) Map stored on an object — callback receives (value, key, map) | |
| const holder: any = { cache: new Map() }; | |
| holder.cache.set("a", { n: 1 }); | |
| holder.cache.set("b", { n: 2 }); | |
| const pairs: string[] = []; | |
| holder.cache.forEach((v: any, k: any) => pairs.push(`${k}=${v.n}`)); | |
| console.log(pairs.join(",")); | |
| // (3) Set forEach argument order: (value, valueAgain, set) | |
| const s: any = { s: new Set(["x"]) }; | |
| s.s.forEach((v: any, v2: any, theSet: any) => | |
| console.log(v === v2, theSet.has("x"), theSet.size), | |
| ); | |
| // (4) plain arrays through the same fused path stay correct | |
| const arrHolder: any = { list: [7, 8] }; | |
| const items: string[] = []; | |
| arrHolder.list.forEach((v: any, i: any, a: any) => items.push(`${i}:${v}:${a.length}`)); | |
| console.log(items.join(",")); | |
| // (5) delete during Set.forEach (React deletes tasks while sweeping): the | |
| // backing vector compacts on delete, so naive index advancement skipped the | |
| // shifted-in next entry. | |
| const req2: any = { tasks: new Set() }; | |
| const t1 = { id: 1 }; | |
| const t2 = { id: 2 }; | |
| req2.tasks.add(t1); | |
| req2.tasks.add(t2); | |
| const seen: number[] = []; | |
| req2.tasks.forEach((t: any) => { | |
| seen.push(t.id); | |
| req2.tasks.delete(t); | |
| }); | |
| console.log(JSON.stringify(seen), req2.tasks.size); | |
| // (6) delete during Map.forEach — same compaction hazard | |
| const m6: any = { m: new Map() }; | |
| m6.m.set("a", 1); | |
| m6.m.set("b", 2); | |
| m6.m.set("c", 3); | |
| const seen6: string[] = []; | |
| m6.m.forEach((v: any, k: any) => { | |
| seen6.push(`${k}:${v}`); | |
| m6.m.delete(k); | |
| }); | |
| console.log(JSON.stringify(seen6), m6.m.size); | |
| // (7) delete an EARLIER entry during iteration (must not skip or re-visit) | |
| const s7 = new Set(["p", "q", "r"]); | |
| const seen7: string[] = []; | |
| s7.forEach((v: any) => { | |
| seen7.push(v); | |
| if (v === "q") s7.delete("p"); | |
| }); | |
| console.log(JSON.stringify(seen7), s7.size); | |
| // (8) add during Set.forEach — entries appended in the callback MUST be visited | |
| const s8 = new Set([1]); | |
| const seen8: number[] = []; | |
| s8.forEach((v: any) => { | |
| seen8.push(v); | |
| if (v < 3) s8.add(v + 1); | |
| }); | |
| console.log(JSON.stringify(seen8), s8.size); | |
| // (9) add during Map.forEach — same contract | |
| const m9 = new Map([["a", 1]]); | |
| const seen9: string[] = []; | |
| m9.forEach((v: any, k: any) => { | |
| seen9.push(`${k}:${v}`); | |
| if (v < 3) m9.set(String.fromCharCode(k.charCodeAt(0) + 1), v + 1); | |
| }); | |
| console.log(JSON.stringify(seen9), m9.size); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-files/test_gap_set_map_foreach_fused_receiver.ts` around lines 1 - 80,
The test file covers Set/Map forEach deletion and receiver-reroute cases, but it
is missing the add-during-iteration regression that the PR objectives require.
Add a new regression block alongside the existing forEach cases that exercises
js_set_foreach_impl and js_map_foreach_impl by appending entries during the
callback and verifying the newly added items are visited in iteration order.
Keep the scenario in the same style as the existing Set/Map tests so future
changes that accidentally snapshot size will be caught.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two related
forEachdefects on native Sets/Maps, both hit by react-server-domduring Next.js App Router SSR (#5989):
Fused array
forEachon a Set/Map receiver segfaults..forEachon anunknown-typed receiver is statically fused to the ARRAY forEach entry point.
When the receiver is actually a native Set or Map — e.g. a collection stored
on an object and read back as a property —
js_array_forEachtreated theSetHeaderas anArrayHeader, feeding hash-table internals to the callbackas "elements" and segfaulting on the first property read:
react-server-dom sweeps
request.abortableTasks(a Set) exactly this way,reading
.statusoff each task — this crashed the server on every dynamicroute once the RSC flight started flowing.
Deleting during
Set.prototype.forEach/Map.prototype.forEachskippedthe next entry. The backing vector compacts on delete (later entries shift
left), so naive index advancement jumped over the shifted-in element:
React's task sweeps delete entries synchronously while iterating.
Fix
array/iter_methods.rs: injs_array_forEach, detect a registered Set/Mapreceiver and route to the native
js_set_foreach/js_map_foreach— thesame pattern as the existing typed-array reroute.
forEachis the onlymethod name the fused array methods share with Set/Map, so this single
reroute covers the hazard class.
set.rs/map.rs: in the forEach loops, only advance the index when thecurrent slot still holds the just-visited entry (compare via a GC-rooted
handle so a move during the callback can't skew the comparison). Handles
delete-current, delete-earlier, delete-later, and delete-then-re-add per
ECMA-262 (re-added entries are appended and re-visited); entries added during
the callback are still visited (existing behavior, revalidated).
Validation
New gap test
test_gap_set_map_foreach_fused_receiver.ts, byte-for-byteagainst
node --experimental-strip-types: the fused property-read receivershape for Set and Map,
(value, value, set)argument order, plain arraysthrough the same fused path, delete-during-iteration for Set and Map, deleting
an earlier entry mid-iteration, and adds-during-iteration regression.
Summary by CodeRabbit
.forEachhandling for collections accessed through generic/unknown references soSetandMapiterate correctly instead of being treated like arrays.Set.forEachandMap.forEachso deletions during iteration no longer cause items to be skipped.Set,Map, and array.forEachbehavior, including deletion-while-iterating cases.