Repository navigation
node:events: validate emitter in static listenerCount like Node - #34605
Conversation
events.listenerCount(emitter, type) now throws ERR_INVALID_ARG_TYPE when the argument is neither an EventEmitter (something with a callable .listenerCount) nor an EventTarget, matching Node. Previously Bun fell through to reading _events and silently returned 0 (or the real count for a null-prototype pseudo-emitter).
|
Status: diff is green; CI red is unrelated flake. Ready for review/merge. Reproduced with: USE_SYSTEM_BUN=1 bun test test/js/node/events/event-emitter.test.ts -t "events.listenerCount validates"(fails: Passes with CI: PR: #34605 |
|
Warning Review limit reached
Next review available in: 46 seconds 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 5:44 AM PT - Jul 18th, 2026
✅ @robobun, your commit 52cdccac8709d44438c2426a3a55adbd93151aea passed in 🧪 To try this PR locally: bunx bun-pr 34605That installs a local version of the PR into your bun-34605 --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 finding, I grepped src/js/ for other internal callers of the static EE.listenerCount / EventEmitter.listenerCount — src/js/internal/streams/legacy.ts:58 is the only one, so no other sibling sites need the ?. update.
Extended reasoning...
Verified the inline bug is real: test/js/node/test/parallel/test-stream-pipe-without-listenerCount.js sets r.listenerCount = undefined (own property shadowing the prototype), and legacy.ts:58 still calls EE.listenerCount(this, "error"), which after this PR throws instead of falling through to the removed _events fallback. Grepped src/js/ for other callers of the static — legacy.ts is the only one, so the fix scope is just that file plus the events.ts change.
Match Node's lib/internal/streams/legacy.js: use this.listenerCount?.() instead of the static EE.listenerCount(), which now throws ERR_INVALID_ARG_TYPE when .listenerCount is shadowed with undefined (test-stream-pipe-without-listenerCount.js).
There was a problem hiding this comment.
LGTM — the legacy.ts caller is now updated to this.listenerCount?.("error"), which resolves the test-stream-pipe-without-listenerCount.js regression I flagged.
Grepped src/js/ and confirmed there are no remaining internal callers of the static listenerCount and no remaining references to listenerCountSlow. The EE import in legacy.ts is still live (used by EE.$call / $toClass).
Extended reasoning...
Overview
Three-file Node-compat change: src/js/node/events.ts replaces the listenerCountSlow fallback in the deprecated static events.listenerCount(emitter, type) with an ERR_INVALID_ARG_TYPE throw (matching Node's lib/events.js); src/js/internal/streams/legacy.ts updates the sole internal caller to this.listenerCount?.("error") === 0 (matching Node's lib/internal/streams/legacy.js); and a new test in event-emitter.test.ts covers both the happy paths (EventEmitter, EventTarget) and the throw paths ({}, 42, null-prototype object with _events but no .listenerCount).
Security risks
None. This tightens validation on a deprecated static helper to throw a typed error instead of silently returning 0. No auth, crypto, filesystem, or network surface.
Level of scrutiny
Low-to-moderate. node:events is core, but the change is a small, surgical alignment with Node's reference implementation. My previous review found the one internal call site (legacy.ts) that would regress; commit 21a8604 fixed it exactly as Node does. I re-grepped src/js/ for EE.listenerCount / events.listenerCount / EventEmitter.listenerCount and for listenerCountSlow — both empty, so no stragglers. The vendored test-stream-pipe-without-listenerCount.js now takes the optional-chaining path (undefined === 0 → false → no re-emit), preserving its behavior.
Other factors
The new test asserts a specific error class + code (not a bare toThrow()), covers both accepted emitter kinds and three rejected shapes, and lives in the existing module test file per repo convention. PR evidence shows the test fails on main / USE_SYSTEM_BUN=1 and passes on the branch in both debug-ASAN and release. The EE import in legacy.ts remains live via the Stream constructor and $toClass, so no dead import was introduced.
There was a problem hiding this comment.
LGTM — the earlier legacy.ts regression is fixed and no other internal callers remain.
What was reviewed:
events.listenerCountnow throwsERR_INVALID_ARG_TYPEfor non-EE/non-EventTarget, matching Node'slib/events.js;jsEventTargetGetEventListenersCountreturnsundefinedfor non-EventTargets so the fall-through is correct.legacy.tsonerrorswitched tothis.listenerCount?.("error") === 0(Node's exact form), sotest-stream-pipe-without-listenerCount.jskeeps passing.- Grepped
src/js/for other staticEE.listenerCount/events.listenerCountcallers and forlistenerCountSlow— none remain.
Extended reasoning...
Overview
Three files: src/js/node/events.ts replaces the listenerCountSlow _events-reading fallback in the deprecated static events.listenerCount with a $ERR_INVALID_ARG_TYPE("emitter", ["EventEmitter", "EventTarget"], emitter) throw and deletes the now-dead helper; src/js/internal/streams/legacy.ts updates the sole internal caller (Stream.prototype.pipe's onerror) to this.listenerCount?.("error") === 0, mirroring Node's lib/internal/streams/legacy.js; and test/js/node/events/event-emitter.test.ts adds a test covering EventEmitter, EventTarget, and three invalid emitters ({}, 42, and a null-prototype object with _events but no listenerCount method) asserting { name: "TypeError", code: "ERR_INVALID_ARG_TYPE" }.
Security risks
None. This tightens validation on a deprecated static helper — turning a silent 0 into a typed error for garbage input. No auth, crypto, native memory, or untrusted-parse surface involved.
Level of scrutiny
Low-to-moderate. It's a small Node-compat delta in a builtin JS module with a clear upstream reference. The one real risk — internal callers relying on the old lax fallback — was flagged in my previous review and has been addressed: legacy.ts now matches Node's optional-chaining form, and I re-grepped src/js/ for EE.listenerCount / events.listenerCount / EventEmitter.listenerCount and for listenerCountSlow — zero hits. I also checked jsEventTargetGetEventListenersCount in JSEventTarget.cpp: it dynamicDowncasts and returns jsUndefined() for non-EventTargets, so plain objects and primitives correctly fall through to the throw rather than getting stuck in the C++ path.
Other factors
The new test asserts specific error name + code (not a bare toThrow()), covers both happy paths (EE and EventTarget) plus three distinct invalid shapes, and lives in the existing event-emitter.test.ts alongside related coverage. The PR was gated fails-without-fix / passes-with-fix, and the author confirmed test-stream-pipe-without-listenerCount.js still passes after the legacy.ts update. The EE import in legacy.ts remains live (EE.$call, $toClass). Net diff is ~16 lines removed, ~1 line changed, plus tests — well within auto-approve territory.
There was a problem hiding this comment.
LGTM — the legacy.ts caller update addresses the earlier regression concern.
What was reviewed:
listenerCountfallback replaced withERR_INVALID_ARG_TYPE, matching Node'slib/events.js;listenerCountSlowfully removed (no remaining refs).- Grepped
src/js/for other staticlistenerCount(emitter, ...)callers — none;EEimport inlegacy.tsstill used for$toClass/EE.$call. null/undefinedemitter still throws a plain TypeError on the.listenerCountproperty read — same as Node, pre-existing.- New test covers EventEmitter, EventTarget, and three invalid shapes with exact
code/nameassertions.
Extended reasoning...
Overview
Tightens the deprecated static events.listenerCount(emitter, type) to throw ERR_INVALID_ARG_TYPE when emitter has neither a callable .listenerCount nor is an EventTarget, replacing Bun's non-standard _events-reading fallback (listenerCountSlow). Also updates the sole internal caller in src/js/internal/streams/legacy.ts to Node's this.listenerCount?.("error") === 0 form so test-stream-pipe-without-listenerCount.js keeps passing. Adds a test in event-emitter.test.ts covering the valid EventEmitter/EventTarget paths plus three invalid inputs ({}, 42, and a null-proto object with _events but no prototype method).
Security risks
None. This is a validation-tightening change in a deprecated Node compat helper; it converts a silent 0/bogus-count return into an explicit typed error. No untrusted input parsing, no auth/crypto/fs surface.
Level of scrutiny
Low-to-moderate. The change is a direct port of Node's lib/events.js behavior (duck-type check → EventTarget check → throw) and Node's lib/internal/streams/legacy.js onerror shape. The only real risk was breaking internal callers that relied on the old lenient fallback — my earlier review flagged exactly that for legacy.ts:58, and it's now fixed and verified against the vendored Node test. I re-grepped src/js/ and confirmed no other static listenerCount call sites remain.
Other factors
- The dead
listenerCountSlowhelper is removed in the same PR (per REVIEW.md's "delete dead code in the same PR that makes it dead"). - The
EEimport inlegacy.tsis not dead after this change — still used forEE.$calland$toClass. - The ruled-out candidate (
null/undefinedemitter throwing generic TypeError instead ofERR_INVALID_ARG_TYPE) matches Node: Node also readsemitter.listenerCountunguarded, so a null emitter throws the engine's own TypeError there too. - Test asserts exact
{ name: "TypeError", code: "ERR_INVALID_ARG_TYPE" }(not baretoThrow()), and exercises both success paths before the failure loop so the assertions can't be vacuous. - Gate evidence shows fails-without-fix / passes-with-fix on both debug-ASAN and release.
…nt (#34667) ## Repro ```js const { EventEmitter } = require("events"); new EventEmitter().listenerCount("x"); ``` ``` ReferenceError: listenerCountSlow is not defined at listenerCount (node:events:450:29) ``` Every single-argument `emitter.listenerCount(type)` call hits this, which broke `test/js/node/http2/node-http2.test.js` (38 fail) via `emitSessionCloseNT` calling `listenerCount("close")`, along with anything else that counts listeners. ## Cause #34605 (efab3b2) removed the `listenerCountSlow` helper when it dropped the fallback path from the static `events.listenerCount`, but `EventEmitterPrototype.listenerCount` (added in a227ad9) still called it for the common 1-argument form. ## Fix Inline the lookup into the prototype method, structured the same way as Node's own `listenerCount`: read `_events[type]` once, then handle the bare-function and array storage shapes for both the 1-arg (`return 1` / `handlers.length`) and 2-arg (match against `method`) forms. ## Verification - `bun bd test test/js/node/events/event-emitter.test.ts`: 70 pass - New `EventEmitter.prototype.listenerCount` test fails on main with `ReferenceError: listenerCountSlow is not defined`, passes with the fix - All 26 `test/js/node/test/parallel/test-event-emitter-*.js` pass - Test expectations checked against Node.js
2 participants
What
events.listenerCount(emitter, type)(the deprecated static) now throwsERR_INVALID_ARG_TYPEwhenemitteris neither an EventEmitter (anything with a callable.listenerCount) nor anEventTarget, matching Node.Repro
Cause
Bun's
listenerCountfell through to alistenerCountSlowhelper that reademitter._eventsdirectly and returned 0 for anything without one. Node has no such fallback: after the duck-typed.listenerCountcheck and theisEventTargetcheck, it throws.Fix
Replace the
_eventsfallback with the sameERR_INVALID_ARG_TYPE("emitter", ["EventEmitter", "EventTarget"], emitter)throw Node uses, and drop the now-unusedlistenerCountSlowhelper. The EventEmitter path (callable.listenerCount) and EventTarget path (native listener count) are unchanged.Verification
bun bd test test/js/node/events/event-emitter.test.tspasses (68 tests)USE_SYSTEM_BUN=1test/js/node/test/parallel/test-events-once.jsandtest-events-on-async-iterator.jsstill pass[stamp-90s] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 2
evidence per changed file