Skip to content

node:events: make EventEmitterAsyncResource match Node.js - #33689

Closed
robobun wants to merge 3 commits into
mainfrom
farm/54fe1423/eventemitter-asyncresource-compat
Closed

robobun wants to merge 3 commits into
mainfrom
farm/54fe1423/eventemitter-asyncresource-compat

Conversation

@robobun

@robobun robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Repro

import { EventEmitterAsyncResource } from "node:events";

const r = new EventEmitterAsyncResource({ name: "X" });
console.log(r.asyncResource?.constructor?.name);
console.log("eventEmitter" in Object(r.asyncResource));
console.log(r.asyncResource?.eventEmitter === r);
console.log(Object.getOwnPropertyNames(EventEmitterAsyncResource.prototype).sort().join(","));
try { new EventEmitterAsyncResource(); console.log("no throw"); } catch (e) { console.log(e.code); }
Node v26.3.0 Bun (before)
asyncResource.constructor.name EventEmitterReferencingAsyncResource AsyncResource
"eventEmitter" in asyncResource true false
asyncResource.eventEmitter === r true false
prototype own names asyncId,asyncResource,constructor,emit,emitDestroy,triggerAsyncId constructor,emit,emitDestroy
new EventEmitterAsyncResource() throws ERR_INVALID_ARG_TYPE no throw

Cause

src/js/node/events.ts constructed this.asyncResource = new AsyncResource(name, ...) directly and stored asyncResource / triggerAsyncId as own instance properties. There was no EventEmitterReferencingAsyncResource subclass, no eventEmitter back-reference, no prototype accessors, and no options.name validation. emit() also discarded the runInAsyncScope return value.

Fix

Rewrote the class to mirror Node's implementation:

  • asyncResource is an EventEmitterReferencingAsyncResource (extends AsyncResource) with a #eventEmitter private field exposed via an eventEmitter getter.
  • asyncId, triggerAsyncId, and asyncResource are prototype getters backed by a #asyncResource private field, so accessing them on an invalid receiver throws TypeError like Node.
  • When instantiated directly (new.target === EventEmitterAsyncResource), options.name is validated with validateString and throws ERR_INVALID_ARG_TYPE if missing or not a string. Subclasses default to new.target.name. A bare string argument is still accepted as the name.
  • emit() now returns the boolean from super.emit (also addressed by node:events: return boolean from EventEmitterAsyncResource#emit #33688; this change subsumes it).

AsyncResource continues to be required lazily on first construction to avoid a bootstrap cycle with node:async_hooks.

Verification

Added tests to test/js/node/async_hooks/EventEmitterAsyncResource.test.ts covering the back-reference, prototype shape, options.name validation, subclass defaulting, string-name form, emit() return value, and invalid-receiver errors. All new tests fail on the released binary and pass with this change; the existing node:events and node:async_hooks suites remain green.

- asyncResource is now an EventEmitterReferencingAsyncResource with an
  eventEmitter getter pointing back to the owning emitter
- asyncId, triggerAsyncId, and asyncResource are prototype getters
  instead of own value properties
- constructing EventEmitterAsyncResource directly without options.name
  throws ERR_INVALID_ARG_TYPE (subclasses default to new.target.name)
- emit() forwards the boolean returned by EventEmitter#emit
@coderabbitai

coderabbitai Bot commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 81972ba0-917f-4bde-87fc-8b313bb53c89

📥 Commits

Reviewing files that changed from the base of the PR and between 3f5d816 and d69de04.

📒 Files selected for processing (2)
  • src/js/node/events.ts
  • test/js/node/async_hooks/EventEmitterAsyncResource.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 7, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:02 PM PT - Jul 7th, 2026

❌ @robobun, your commit d69de04 has 1 failures in Build #70003 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33689

That installs a local version of the PR into your bun-33689 executable, so you can run:

bun-33689 --bun

@github-actions github-actions Bot added the claude label Jul 7, 2026
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. Bug in executionAsyncId from node:async_hooks #32168 - Directly reproduces EventEmitterAsyncResource bugs: asyncId returning undefined and executionAsyncId() not tracking context inside event handlers, both fixed by this PR's getter and runInAsyncScope changes

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #32168

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. node:events: return boolean from EventEmitterAsyncResource#emit #33688 - Fixes EventEmitterAsyncResource#emit return value, which is a subset of this PR's comprehensive rewrite
  2. async_hooks,events,http,http2,perf_hooks: port Node.js async compatibility tests and fix the gaps they surface — ALS run/disable + withScope/defaultValue, http client ALS across reused agent sockets, http2 ALS context, AsyncResource.bind, EventEmitterAsyncResource, timerify (+22 tests) #31825 - Includes EventEmitterAsyncResource rewrite as part of a broader async_hooks/events Node.js compatibility port

🤖 Generated with Claude Code

@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

On the bot suggestions above:

#32168: this change makes that issue's repro print miao instead of throwing, but only because both executionAsyncId() and AsyncResource#asyncId() are stubbed to 0 in Bun. The underlying request there is real async id tracking inside runInAsyncScope, which this PR does not implement (and would regress again once #31711 assigns unique ids). Leaving it out of the description.

Overlap:

@robobun

robobun commented Jul 7, 2026 •

Copy link
Copy Markdown
Collaborator Author

The diff is ready. CI red on build 70003 is unrelated to this change:

  • :darwin: 26 aarch64 - test-bun (both shards, and build 69989 before the retrigger): fails before any test runs with buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun' on agent darwin-aarch64-26-5-1-1. Pure infra.
  • test/js/node/test/parallel/test-net-connect-memleak.js on alpine 3.23 x64 / x64-baseline: a node:net GC-timing test (assert.strictEqual(collected, true) after one globalThis.gc()). node:net does not use EventEmitterAsyncResource; the only src/js/ consumer of that class is node:events itself.

test/js/node/async_hooks/EventEmitterAsyncResource.test.ts and the rest of the node:events / node:async_hooks suites pass on all 278 completed test lanes. All other annotations are pre-existing flaky retries (napi, valkey, solc, postgres, install, http-keep-alive) unrelated to node:events.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-07, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

@robobun robobun closed this Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant