node:events: make on() O(1) again by appending in place - #35801
Conversation
|
Status: rebased onto main 59242d6 (2026-07-29), force-pushed c4a2654. Release re-measurement:
This PR fixes Reproduced with (release, before fix: 138/183/766/2558 ms; after: 0.3/0.5/0.9/1.7 ms): bun -e 'const {EventEmitter}=require("node:events");for(const n of[5e3,1e4,2e4,4e4]){const ee=new EventEmitter();ee.setMaxListeners(0);const t=performance.now();for(let i=0;i<n;i++)ee.on("x",()=>{});console.log(n,(performance.now()-t).toFixed(1),"ms")}' |
|
Warning Review limit reached
Next review available in: 12 minutes 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 4:05 AM PT - Jul 29th, 2026
❌ @robobun, your commit c4a2654 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 35801That installs a local version of the PR into your bun-35801 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I traced the in-place append against the reentrancy cases and they hold: remove-then-add during emit (remove installs a fresh array so the subsequent push lands there, emit's local handler stays the old object), removeAllListeners(type) LIFO walk (first removeListener reassigns events[type] before emitting removeListener, so a handler's on() pushes onto the copy, not the snapshot), and rawListeners()/listeners() still return copies via .slice()/.map() so callers can't observe later pushes.
Extended reasoning...
The only finding is a stale comment in removeListener — no runtime effect. The core invariant change (append mutates in place; only index-shifting ops copy) is sound because every iteration site latches length up front (applyHandlers, both emit variants). I checked the non-obvious reentrancy paths — remove→add mid-emit, once() wrapper self-removal, newListener handlers that add/remove, removeAllListeners with a removeListener handler that re-adds — and in each case the in-flight iterator holds a reference to an array whose [0..latched) slots are never overwritten. Deferring rather than approving only because node:events is the hottest path in the runtime and the invariant relaxation deserves a maintainer's eyes.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the stale removeListener comment (03f1fb5) and compressing the invariant comments (4514c21) — all seven sites now state the new invariant consistently. I couldn't find a hole in the append-in-place reasoning, but this relaxes the mutation invariant on node:events' hottest path, so I'd like a maintainer to confirm before merge.
What was reviewed
- Traced every iterator over a stored
_events[type]array (emitWithoutRejectionCapture,emitWithRejectionCapture,applyHandlers,removeAllListenersLIFO loop) — each either latcheslengthor only ever sees a fresh copy after the first reentrancy, so an in-place push is never visited mid-iteration. - Checked reentrancy combinations:
once→removeListenermid-emit followed byon(),on()thenprependListener()/removeListener()mid-emit,newListenerhandlers mutating the list — emit's capturedhandlerreference plus latchedlengthkeeps[0..length)stable in each. .warnedcarryover: preserved naturally on the in-place array, still copied bycopyWithPrependedandremoveListener;rawListeners()/listeners()still hand out fresh copies so userland can't observe later pushes.copyWithInsertedhad no other callers; rename tocopyWithPrependedis clean.
Extended reasoning...
Overview
This PR reverts the O(N²) cost of EventEmitter#on() introduced when #34519 made listener arrays copy-on-write. The functional change is one branch in _addListener (src/js/node/events.ts): when appending to an existing array, use $arrayPush(existing, fn) instead of copyWithInserted. copyWithInserted is simplified to copyWithPrepended (append branch deleted), and seven invariant comments across events.ts and streams/legacy.ts are updated from "arrays are never mutated in place" to the weaker-but-accurate "index-shifting mutations install a copy; append lands past emit()'s latched length". Two tests are added: a timing bound (12k adds < 500ms) and a behavioral test that on() during emit doesn't fire in that round.
Security risks
None. Pure JS, no untrusted input parsing, no auth/crypto.
Level of scrutiny
High. node:events underpins every stream, socket, HTTP server, child process, and FS watcher in the runtime, and on()/emit() are on essentially every hot path. The change deliberately relaxes a safety invariant (full COW → append-only mutation) whose whole point was to make emit()'s clone-free iteration safe against reentrancy. A subtle mistake here would surface as rare listener-skips or double-fires deep inside unrelated subsystems. That said, the diff is small and the argument is precise: every in-tree iterator over a stored array latches length at loop entry (for (let i = 0, { length } = handler; ...)), and $arrayPush only writes past that index; the two operations that shift existing indices (prepend, remove) still install a fresh array so an in-flight emit's captured handler reference stays byte-stable over [0..length).
I walked the reentrancy combinations that #34519's COW was defending against and could not construct a case where a pushed slot is visited or an existing slot is skipped: once wrappers call removeListener (fresh array) before invoking the target, so a subsequent on() in the same round pushes into the new array while emit iterates the old; removeAllListeners' LIFO loop captures listeners before its first removeListener call replaces events[type]; applyHandlers (errorMonitor path) latches length identically. .warned naturally persists on the same array object, and copyWithPrepended/removeListener still carry it over. rawListeners()/listeners() return .slice()/.map() copies, so userland snapshots remain immutable. No remaining callers of copyWithInserted.
Other factors
My prior review flagged one stale comment; that and the comment-cop feedback are both addressed and all threads resolved. Test coverage is solid — the PR reports the full test-event-emitter-*/test-events-* node-parallel suite (including test-event-emitter-modify-in-emit.js) and node-stream.test.js passing, plus the two new targeted tests. The "addListener in callback" test in the existing suite already exercises append-during-emit ordering. The 500ms perf assertion has a wide margin (author measured ~1.6s vs tens of ms under debug+ASAN), so it shouldn't be flaky.
I'm deferring rather than approving because relaxing a reentrancy invariant on node:events is exactly the kind of change where a maintainer's sign-off is worth more than a bot's — even one that couldn't find a counterexample.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
nobody is adding more than like 9 event listeners.
this seems dumb
There was a problem hiding this comment.
Beyond the inline test-threshold nit: I verified all three iterate-without-clone sites (emitWithoutRejectionCapture, emitWithRejectionCapture, applyHandlers) latch length before the loop, so the in-place push can't be visited mid-emit; rawListeners()/listeners() return copies so external holders don't observe the mutation; and removeAllListeners iterates a local snapshot that removeListener replaces rather than mutates, so an on() from a removeListener handler can't corrupt that walk. Node's own _addListener does existing.push(listener), so this restores parity rather than diverging.
Extended reasoning...
The inline nit (perf-test threshold only discriminates on debug+ASAN) stands on its own. This note just records the correctness concerns I checked around the weakened invariant — every no-clone iterator of the stored array either latches length up front or holds a reference that index-shifting mutators replace with a fresh array — so a human reviewer looking at the invariant change doesn't have to re-derive them.
|
Fair, 40k listeners is a synthetic benchmark. The reason I thought it was worth touching anyway: At the common case (2-9 listeners) the fix is also never worse. Before, every Node's Happy to close if you'd rather keep the simpler "stored arrays are immutable" invariant; the diff is small either way. |
#34519 switched the listener array to copy-on-write so emit() could iterate without cloning. That made every on() copy the whole array, so N adds on one event type cost O(N^2): 40k adds went from ~2ms to ~2.5s. Append in place instead: emit() already latches the array length before iterating, so a listener pushed mid-emit sits past the latched length and is never visited (matching Node's semantics). Only prepend and remove, which shift existing indices, still install a fresh array so an in-flight emit keeps a stable view.
…also discriminate
ab8e560 to
c4a2654
Compare
There was a problem hiding this comment.
No bugs found; both earlier nits (stale removeListener comment, unbranched perf threshold) are addressed. Deferring because this changes the mutation invariant for the stored listener array in node:events — foundational hot path — and the author notes #35823 supersedes it with a different scheme (kIterated), so a maintainer should pick which lands.
What was reviewed:
- Traced emit()'s latched-length loop against every mutator: append in place lands past the latch; prepend/remove/removeAllListeners install a fresh array so the
handlerlocal stays stable; combinations (append-then-remove mid-emit, once() self-removal then append) all keephandler[0..length)intact. - Checked
rawListeners()/listeners()still return copies (slice/map), so the now-mutable internal array doesn't leak. - Checked
.warnedpropagation — in-place append keeps it on the same array;copyWithPrependedcarries it over.
Extended reasoning...
Overview
Restores O(1) EventEmitter.prototype.on() by switching the append path in _addListener from copy-on-write (copyWithInserted) to in-place $arrayPush. The safety argument is that both emit() variants and applyHandlers latch length before iterating, so a slot pushed mid-emit sits at an index ≥ the latched length and is never visited. Only index-shifting mutations (prependListener, removeListener) still install a fresh array. Also renames copyWithInserted → copyWithPrepended (dropping the dead append branch), rewrites the invariant comments at all seven sites, updates the streams/legacy.ts prependListener comment to match, and adds two tests (perf regression guard branched on isDebug || isASAN; append-during-emit ordering).
Security risks
None. Pure JS, no untrusted-input parsing, no auth/crypto surface. The only user-observable change is performance and preserving Node's documented "listeners added during emit are not called in that emit" semantics — which the previous COW implementation also preserved.
Level of scrutiny
High. src/js/node/events.ts is one of the hottest, most foundational built-in modules — every stream, every server, every process is an EventEmitter. The correctness argument is subtle: it depends on the interaction between the length latch in three iteration sites and every mutation path (including mid-emit combinations like once-wrapper self-removal followed by an append, or append-then-remove where the append mutated the same array remove reads from). I walked each combination and believe the invariant holds, but a maintainer who owns this file should sign off on relaxing "stored arrays are never mutated" to "only index-shifting mutations install a fresh array".
Other factors
- The author's own status comment says #35823 covers both
on()and theremoveListenertail-drain O(N²) via a stickykIteratedmark and "supersedes this PR" — a maintainer should decide whether to land this narrower fix now or wait for the broader one. - Both of my earlier inline comments are resolved in the current diff (the removeListener comment now reads "Copy-remove so an in-flight emit() keeps a stable view"; the perf test threshold branches on
isDebug || isASAN). - Test coverage: the PR ran the 34 upstream
test-event-emitter-*/test-events-*parallel tests includingtest-event-emitter-modify-in-emit.js, plus the full event-emitter and node-stream suites. The new append-during-emit test directly asserts the load-bearing invariant.
2 participants
What does this PR do?
#34519 switched the listener array to copy-on-write so
emit()could skip cloning. That made everyon()copy the whole array, so registering N listeners on one event type cost O(N^2).Fix: append in place via
$arrayPush.emit()already latcheslengthbefore iterating, so a listener pushed mid-emit sits past the latched length and is never visited, which matches Node's "listeners added during emit are not called in that emit". OnlyprependListenerandremoveListener, which shift existing indices, still install a fresh array so an in-flight emit keeps a stablehandler[0..length)view.emit()stays clone-free.How did you verify your code works?
bun bd test test/js/node/events/event-emitter.test.ts(77 pass).test/js/node/test/parallel/test-event-emitter-*.js/test-events-*.jspass, includingtest-event-emitter-modify-in-emit.jswhich exercises add/remove during emit.test/js/node/stream/node-stream.test.js(98 pass).[review] gate passed · iteration 9 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 9
evidence per changed file
root cause · written by the author bot
The listener storage used copy-on-write semantics, so every
on()call allocated a fresh array and copied all existing listeners into it, making each registration O(N) and N registrations on the same event O(N²). The fix replaces the per-add array clone with an in-place push onto the existing listener array, matching Node's behavior and making each add O(1). The same change removes the shared copy-on-write root that was also inflatingremoveListener()cost.