node:events: mutate listener arrays in place; copy only when emit() has iterated them - #35823
node:events: mutate listener arrays in place; copy only when emit() has iterated them#35823robobun wants to merge 4 commits into
Conversation
…as iterated them Adding or removing a listener allocated a fresh N-element array per call so emit() could iterate without a defensive clone. That made N adds, or a tail-first drain of N listeners, O(N^2): 64k listeners took ~5.7s to drain and ~3.8s to add. Listener arrays are now mutated in place (push / unshift / spliceOne, as in Node). emit() marks the array it walks with a sticky symbol; a mutator that sees the mark installs a fresh copy instead, so the running loop still sees its original snapshot. The copy is unmarked, so the next mutation is in-place again. The mark is one property read (and a write only on the first emit of a given array), so the emit hot path is unchanged within noise. Fixes #3770, fixes #3734.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 11:34 PM PT - Jul 25th, 2026
❌ @robobun, your commit 019e53d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35823That installs a local version of the PR into your bun-35823 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #35801. That PR makes |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this reworks the copy-on-write strategy in node:events — the hottest path in the runtime — and the sticky-kIterated design is a trade-off (post-emit mutations always pay one copy) that's worth a maintainer's eyes.
What was reviewed:
- Traced
kIteratedthrough all three emit paths (emitWithoutRejectionCapture,emitWithRejectionCapture,applyHandlers) — mark is set before the loop in each, so mutators inside a listener always see it. - Verified the nested-emit case and the
removeAllListenersLIFO walk stay correct when the array shrinks in place under it (matches Node's uncloned loop). - Checked
once()wrappers: first removal COWs the marked array, subsequent removals splice the unmarked copy — outer emit's snapshot is untouched. legacy.tschange is comment-only; the fresh-array behavior there is preserved.
Extended reasoning...
Overview
This PR changes src/js/node/events.ts from unconditional copy-on-write (every on/off/prependListener allocates a fresh array) to in-place mutation gated by a sticky kIterated symbol that emit() stamps on arrays it iterates. The goal is to fix O(N²) behavior when adding or draining N listeners. Also touched: a comment-only update in src/js/internal/streams/legacy.ts and two new tests in event-emitter.test.ts.
Security risks
None. This is pure JS data-structure management with no I/O, auth, or untrusted-input parsing. The new spliceOne uses list.pop() (user-overridable), but the file already uses .map/.slice on the same arrays and Node's own spliceOne does the same, so this doesn't change the tamper-resistance posture.
Level of scrutiny
High. node:events underlies every stream, socket, HTTP request, and child process in the runtime. The correctness argument depends on the mark being set at every iteration site before user code runs, and on the interaction between in-place spliceOne and the removeAllListeners LIFO loop (which now walks an array that shrinks under it). I traced these and they look right — including the length-2 → bare-function transition mid-loop — and the behavior matches Node's uncloned loop. But the sticky-vs-counter design choice and the emit hot-path cost (one symbol read + conditional write per emit) are the kind of trade-off a maintainer should sign off on per the repo's Performance guidance.
Other factors
The new tests are deterministic (identity check on _events.x and exact call-order for nested emit), and the PR description references existing Node parallel tests (test-event-emitter-modify-in-emit.js, test-event-emitter-remove-listeners.js) for the modify-during-emit semantics. CI is still building. No prior human reviews on the thread.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… per-site repeats
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/events.ts:491-494—spliceOnecallslist.pop()directly, which dispatches through the runtime (user-overridable)Array.prototype.pop, while the add path in this same PR uses the tamper-safe$arrayPushintrinsic and the module-load-capturedArrayPrototypeUnshift.$call. For consistency with the hardening you applied on the add side, captureconst ArrayPrototypePop = Array.prototype.pop;at module load and call it via.$call(list)here.Extended reasoning...
What the issue is
The new
spliceOnehelper at src/js/node/events.ts:491-494 ends with a barelist.pop():function spliceOne(list, index) { for (; index + 1 < list.length; index++) list[index] = list[index + 1]; list.pop(); }
This looks up
poponlistat call time, which resolves throughArray.prototype.pop— a userland-overridable slot. That is asymmetric with the add path this same PR introduces at lines 341-342:if (prepend) ArrayPrototypeUnshift.$call(existing, fn); else $arrayPush(existing, fn);
There,
$arrayPushis a JSC intrinsic andArrayPrototypeUnshiftis captured at module load (line 42) and invoked via.$call, so neither can be intercepted by userland monkey-patching. The PR author clearly considered tamper-safety on the add side (choosing$arrayPush(existing, fn)overexisting.push(fn)), so leaving the symmetric remove-sidepop()bare is an inconsistency within the diff.Why this matters per the repo guidelines
REVIEW.md's section on built-in JS modules is explicit:
Built-in JS modules (
src/js/) are hot-path code in a hostile environment. Tamper-resistance: $-prefixed intrinsics and primordial-safe calls ($isJSArray,map.$get,$call), globals captured at module load … never route internal logic through user-overridable machineryremoveListeneris one of the hottest paths innode:events, andlist.pop()routes through user-overridable machinery.Step-by-step failure mode
Concrete walkthrough with a hostile override:
- Userland runs
Array.prototype.pop = () => {};(a no-op). - An emitter has three listeners on
"x":[f0, f1, f2], and the array carries nokIteratedmark (never emitted). ee.removeListener("x", f0)is called.position = 0,list.length === 3, so control reachesspliceOne(list, 0).- The shift loop copies
list[1]→list[0]andlist[2]→list[1], leaving[f1, f2, f2]. list.pop()dispatches to the userland no-op; length stays 3 and the trailingf2is not removed.- The stored listener array is now
[f1, f2, f2]. A subsequentee.emit("x")invokesf2twice.
A throwing or side-effecting override would similarly break
removeListenermid-operation. MeanwhileaddListeneron the same emitter is unaffected because it uses the intrinsic — that's the asymmetry.Why existing code doesn't prevent it
Nothing else in
removeListener's in-place branch touches the array length;spliceOneis the sole place the trailing slot is dropped, and it relies entirely on.pop(). ThekIteratedcopy-on-write branch and thelength === 2bare-function branch don't reach here, so any unmarked array of length ≥ 3 hits the overridable call.Fix
Mirror the pattern already used for
unshift:const ArrayPrototypePop = Array.prototype.pop; // … function spliceOne(list, index) { for (; index + 1 < list.length; index++) list[index] = list[index + 1]; ArrayPrototypePop.$call(list); }
Why nit, not blocking
Node's own
internal/utilspliceOnealso useslist.pop(), and this file already has pre-existing non-primordial calls (.slice(),.map(), another.pop()inlistenersController). So this is not a regression in Node compat or a new class of vulnerability — it's a consistency/hardening nit within this PR's own choices, worth the one-line fix but not worth blocking merge over. - Userland runs
There was a problem hiding this comment.
The earlier nit (raw .pop() in spliceOne) is addressed — thanks. I didn't find correctness issues, but this replaces the copy-on-write invariant in node:events with a novel sticky-mark scheme (kIterated) that governs reentrancy safety across emit/on/off/removeAllListeners, so it's worth a maintainer sign-off on the design and the hot-path perf claims.
What was reviewed:
- Traced
kIteratedthrough all three emit paths (emitWithoutRejectionCapture,emitWithRejectionCapture,applyHandlersfor error/errorMonitor) — each marks before iterating. - Checked reentrant
once()wrappers, nested emit, and add/remove-during-emit against the new snapshot rule; the fresh copy correctly drops the mark so the next mutation is in-place again. removeAllListenersLIFO walk over the live array: tail-shrink stays in step withi, and the length-2 branch replacesevents[type]without mutating the locallistenersreference, so indices stay valid.legacy.tsprependListenerfallback still allocates a fresh array (can't seekIterated), which is the safe choice.
Extended reasoning...
Overview
This PR changes src/js/node/events.ts from unconditional copy-on-write for listener arrays (introduced in #34519) to in-place mutation ($arrayPush / ArrayPrototypeUnshift.$call / spliceOne), gated by a sticky kIterated symbol that emit() stamps on the array before iterating. Mutators that see the mark fall back to installing a fresh copy so any in-flight emit loop keeps a stable snapshot; the copy is unmarked, so subsequent mutations are in-place again. It also updates the comment in src/js/internal/streams/legacy.ts and adds two tests (array-identity across on/off; nested-emit snapshot preservation) to test/js/node/events/event-emitter.test.ts.
Security risks
None identified. The change is internal listener-array bookkeeping in a JS builtin; no new user-controlled input reaches native code, no auth/crypto/permissions surface. Tamper-resistance was checked: the add path uses $arrayPush / captured ArrayPrototypeUnshift, and after the follow-up commit spliceOne uses captured ArrayPrototypePop.$call, so no new user-overridable prototype lookups are introduced on the hot path.
Level of scrutiny
High. node:events is one of the hottest and most widely-depended-on Node compat modules — every stream, HTTP request, and process signal flows through it. The PR replaces a simple invariant ("stored arrays are never mutated") with a more subtle one ("stored arrays may be mutated iff no emit has ever iterated them"). The design is sound as far as I can trace — the mark is sticky (never cleared) precisely so a nested emit can't unmark an outer emit's array, and every iteration site (emitWithoutRejectionCapture, emitWithRejectionCapture, applyHandlers) sets it — but this is exactly the kind of load-bearing invariant a maintainer should ratify rather than a bot.
Other factors
- My prior inline nit (raw
list.pop()inspliceOne) was addressed in 019e53d with a capturedArrayPrototypePop. - The PR carries benchmark numbers and a hot-path claim ("emit stays within noise, 6.4 → 6.6 ns"); per REVIEW.md's Performance guidance, perf claims on hot paths warrant maintainer verification.
removeAllListenersnow iterates the live array rather than an implicit snapshot. I traced the LIFO walk: tailspliceOneand tailpushpreserve indices[0, i-1], and thelength === 2branch swapsevents[type]to a bare function without touching the locallistenersarray, solisteners[0]is still correct on the final iteration. Node itself clones before this loop, so an exoticremoveListenerhandler that prepends duringremoveAllListenerscould in theory diverge — I judged this too obscure to block on but note it for the human reviewer.- Existing Node parallel tests (
test-event-emitter-modify-in-emit.js,test-event-emitter-remove-listeners.js,test-event-emitter-remove-all-listeners.js) cover the standard reentrancy cases; the new nested-emit test covers the specific hazard the sticky mark exists to prevent. - There is a competing PR (#35801) that only fixes the add path; the author has explained why this one supersedes it, which is another reason a human should pick between them.
|
CI status: the diff itself is green. Build 81993 ran 54 test lanes with no Ready for review. |
2 participants
Since #34519 the listener array is copy-on-write: every
on()/prependListener()/removeListener()allocates a fresh N±1 array soemit()can iterate the stored one with no defensive clone. That makes each call O(N), so N adds or a tail-first drain of N listeners is O(N²). Node'spush/spliceOneis O(1) from the tail, so the same drain is linear there.Release build, this machine:
Fix
Mutate in place (
$arrayPush/ArrayPrototypeUnshift/spliceOne) by default.emit()sets a stickykIteratedsymbol on the array before its loop; a mutator that sees the mark installs a fresh copy instead, so any running emit loop keeps a stable snapshot. The copy carries no mark, so the next mutation is in-place again. The mark is never cleared: that is one property read per emit (plus a single write the first time a given array is emitted), so the hotemit()path stays within noise of main (6.4 → 6.6 ns for 3 listeners over 5e6 iters), and it also sidesteps the nested-emit hazard a clear-after-loop boolean would have.This restores Node's observable shape:
_events[type]is the same array object acrosson/off(untilemit()has walked it), which is what the new test asserts deterministically. Semantics for listeners that add/remove during their own emit are unchanged and covered bytest-event-emitter-modify-in-emit.js/test-event-emitter-remove-listeners.js, plus a new nested-emit case.Fixes #3770.
Fixes #3734.
[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file