node:domain: return the callback's value from bind() and intercept() - #35444
node:domain: return the callback's value from bind() and intercept()#35444robobun wants to merge 4 commits into
Conversation
The wrappers returned by d.bind(fn) and d.intercept(fn) called fn but dropped its return value, and invoked it with this === null instead of the caller's receiver. gulp's async-done wraps every task via d.bind(task) and checks the return value to detect a promise or stream, so under bun the promise was never seen and gulp reported "Did you forget to signal async completion?". Both wrappers now enter/exit the domain around the call, forward the caller's this and arguments, and return the callback's result. intercept() now only diverts to the domain's 'error' listener when the first argument is an Error instance (with the domainBound/domainThrown/ domain properties Node sets), and bind() attaches the .domain property to the returned function, matching Node. Fixes #5923 Fixes #24287
|
Warning Review limit reached
Next review available in: 10 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 (2)
Comment |
|
Updated 6:16 PM PT - Jul 24th, 2026
❌ @robobun, your commit c4e09f2 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 35444That installs a local version of the PR into your bun-35444 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused Node compat fix that makes bind()/intercept() return the callback's value and enter/exit the domain, unblocking gulp/async-done.
What was reviewed:
bind()/intercept()now forward the caller'sthisand returnfn's result — cross-checked against Node'slib/domain.js(ReflectApply(cb, this, args)path).intercept()'sinstanceof Errorgate anddomainBound/domainThrown/domaintagging match Node'sinterceptedhelper.- Considered whether
intercept()should also stamp.domainon its wrapper and whether Node uses the domain asthis— both examined and ruled out as blockers. - Tests cover return value, receiver forwarding, active-domain-during-call, the Error-vs-truthy branch, and the async-done regression shape.
Extended reasoning...
Overview
The PR touches two files: src/js/node/domain.ts (the bind() and intercept() wrapper implementations inside domain.create()) and a new test file test/js/node/domain/domain.test.ts. The functional change is that both wrappers now return fn.$apply(this, ...) inside a d.enter() / try…finally d.exit() block instead of calling fn.$apply(null, args) and discarding the result. bind() additionally defines a non-enumerable .domain property on the returned function, and intercept() now only diverts to the domain's 'error' listener when arguments[0] instanceof Error (previously any truthy value), tagging the error with domainBound / domainThrown / domain before emitting.
Security risks
None. node:domain is a deprecated in-process error-routing API; the change only adjusts return-value plumbing and this forwarding in a pure-JS builtin. No untrusted input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. node:domain is a legacy/deprecated module and Bun's implementation is already a lightweight polyfill (it doesn't hook the microtask queue the way Node does). The change is small (~40 LOC), strictly moves behavior closer to Node, and is motivated by two real user reports (#5923, #24287) where gulp's async-done relies on d.bind(task)() returning the task's promise. I traced Node's Domain.prototype.bind / intercept in lib/domain.js: both call the callback via ReflectApply(cb, this, args) where this is the caller's receiver forwarded through FunctionPrototypeCall(…, this, …), and both return the callback's result — matching this PR. Node's intercepted helper also gates on arguments[0] instanceof Error and sets domainBound/domainThrown = false/domain on the error before emitting, which this PR replicates.
Other factors
Finders raised two candidate gaps that verifiers refuted: (1) intercept() not stamping .domain on its returned wrapper the way bind() does, and (2) whether Node passes the domain (not the caller's receiver) as this. On (2), Node's source confirms the caller's this is forwarded, so the PR is correct. On (1), even if Node does set it, it's a cosmetic property on a deprecated API and orthogonal to the return-value bug being fixed — not a blocker. The test file exercises each new behavior with tight assertions (exact return values, toBe identity checks, process.domain before/during/after), plus a subprocess-spawned async-done-shaped repro that would have failed on the old code. No existing tests were weakened. The remaining minor deviations from Node (e.g. emitError setting domainThrown = false on caught throws) are pre-existing and out of scope.
Addresses review: - Hoist the enter/try/catch/finally/exit block into a single helper the three methods delegate to, matching Node's shape. - Drop the bind()-throw test that asserted the shim's swallow-and-emit behaviour; Node lets the throw propagate to the caller, so that test pinned a divergence rather than parity. - Add before/after process.domain checks to the intercept() active-domain test so the exit() call site is covered.
There was a problem hiding this comment.
LGTM — focused Node compat fix that makes bind()/intercept()/run() return the callback's value and forward this, unblocking gulp/async-done.
What was reviewed
runInDomainhelper: enter/exit balance viafinally, return value and receiver forwarding match Node'sbound/interceptedhelpers.intercept()'s newinstanceof Errorgate +domainBound/domainThrown/domaintagging — matches Node'slib/domain.js.d.runrefactor: still passes the domain asthisand still enters/exits the closure-capturedd, so no behavior change there.- Tests: serial for the
process.domainassertions (no cross-test leakage), subprocess test drains pipes concurrently and asserts output before exit code.
Extended reasoning...
Overview
Two files: src/js/node/domain.ts (the built-in node:domain shim) and a new test file. The old bind()/intercept() wrappers called the user's callback with this === null and discarded its return value, which broke gulp's async-done (it wraps tasks via d.bind(task) and inspects the return for a thenable). The fix extracts a shared runInDomain(thisArg, fn, args) helper that enters the domain, applies fn with the caller's receiver, returns the result, routes throws to the domain's error event, and exits in finally. bind() now also sets .domain on the returned function, and intercept() now only treats an Error-instance first arg as an error (tagging it with domainBound/domainThrown/domain), matching Node's lib/domain.js.
Security risks
None. node:domain is a deprecated control-flow helper with no auth/crypto/filesystem surface; the change only affects how a user-supplied callback's return value and receiver are forwarded.
Level of scrutiny
Low-to-medium. This is a small, self-contained Node-compat fix in a deprecated module. I cross-checked the shape against Node's Domain.prototype.bind/intercept/run — the return-value forwarding, this forwarding, .domain property descriptor, and the er && er instanceof Error gate all line up. The one deliberate deviation (catching the throw locally instead of relying on process._fatalException) is documented in a comment and preserves the pre-existing behavior of run(). The d.run refactor is behavior-preserving: runInDomain calls d.enter()/d.exit() on the closure-captured d and applies with this (the domain when called as d.run(...)), same as before.
Other factors
The finder agents flagged that intercept() doesn't set .domain on its wrapper the way bind() does; verifiers ruled it out — it's a pre-existing minor gap, not a regression, and doesn't affect the gulp/async-done use case this PR targets. The new tests follow harness conventions (bunExe/bunEnv, concurrent pipe draining, output asserted before exit code, test.concurrent for the subprocess case) and the process.domain-mutating tests are serial so they can't race each other. No existing Bun-authored domain.test.ts existed, so creating test/js/node/domain/domain.test.ts is the right location.
There was a problem hiding this comment.
LGTM — focused Node compat fix in the domain shim with good coverage.
What was reviewed:
runInDomain()correctly forwardsthis/return value and preserves the pre-existing enter/catch/exit shape;d.run's switch fromthis.enter()to closure-capturedd.enter()is equivalent since each domain instance gets its own closure.intercept()'sinstanceof Errorgate anddomainBound/domainThrown/domaintagging match Node's implementation; verified it correctly emits directly rather than viaemitError(which would setdomainEmitterinstead).- Confirmed Node does not set
.domainon theintercept()wrapper — onlybind()does, so the asymmetry is intentional. - Tests cover return value, receiver, active-domain,
.domain, Error-vs-truthy, and the async-done regression; subprocess test drains both pipes and asserts exit code last.
Extended reasoning...
Overview
This PR fixes node:domain's bind() and intercept() wrappers, which previously called the wrapped function but discarded its return value and passed null as the receiver. The fix extracts a shared runInDomain(thisArg, fn, args) helper that enters the domain, applies the function with the caller's this and args, returns the result, catches synchronous throws into the domain's 'error' listener, and exits in finally. bind(), intercept(), and run() all delegate to it. bind() now also sets .domain on the returned function, and intercept() now only diverts to the error listener when the first argument is an Error instance (tagging it with domainBound/domainThrown/domain), matching Node. A new test file covers each of these behaviors plus an async-done-shaped subprocess regression test for #5923/#24287.
Security risks
None. node:domain is a deprecated error-routing shim with no auth, crypto, filesystem, or network surface. The change only affects how return values and receivers are forwarded through user-supplied callbacks.
Level of scrutiny
Low-to-moderate. The module is a ~100-line JS shim for a deprecated Node API, and the change is a straightforward compat fix cross-checked against Node v26.3.0. The refactor into runInDomain() is a net simplification (three copies of enter/try/catch/exit collapsed to one). The only behavioral changes beyond the headline fix — instanceof Error gating in intercept() and the .domain property on the bound function — are direct ports of Node's semantics and are each covered by a dedicated test.
Other factors
d.runpreviously calledthis.enter()/this.exit();runInDomaincallsd.enter()/d.exit()on the closure-captured domain. Since every method here is a per-instance closure created insidecreateDomain(),this === din any sane use, so this is not a behavior change.- The intercept error path emits via
d.emit("error", er)directly rather than the existingemitErrorhelper — this is correct, becauseemitErrorsetsdomainEmitterwhereas Node's intercept path setsdomainBoundinstead. - Candidate issue "intercept() doesn't set .domain on the returned function" was checked against Node — Node's
Domain.prototype.interceptdoes not set.domainonrunIntercepted, onlybinddoes, so the asymmetry is correct. - Tests follow harness conventions:
bunExe()/bunEnv, concurrent subprocess test, both pipes drained viaPromise.all, stderr/stdout asserted before exit code, issue URLs in the regression comment. - The pre-existing divergence (catching synchronous throws locally rather than via
process._fatalException) is called out in a comment and left unchanged, which is the right scope boundary.
|
The new
Ready for a maintainer to merge. |
1 participant
What does this PR do?
d.bind(fn)andd.intercept(fn)returned wrappers that calledfnbut dropped its return value (and invoked it withthis === nullinstead of the caller's receiver).gulp's
async-donewraps every task viad.bind(task)and inspects the return value to decide whether it got back a promise, stream, or observable. Becausebind()swallowed the return value, the task's promise was never awaited and gulp immediately reported:Fix
Both wrappers now match Node for the pieces
async-donedepends on:thisand argumentsbind()sets.domainon the returned functionintercept()only diverts to the domain's'error'listener when the first argument is anErrorinstance (previously any truthy value), and tags the error withdomainBound/domainThrown/domainthe way Node doesThe enter/apply/exit body is now a single
runInDomain()helper thatbind,intercept, andrunall delegate to. The helper still catches a synchronous throw and routes it to the domain's'error'listener; Node lets the throw propagate and catches it later via theprocess._fatalExceptiondomain hook, which Bun does not have yet. That is pre-existing shim behaviour and outside the scope of this change.How did you verify your code works?
test/js/node/domain/domain.test.tscovers the return value, receiver forwarding, active-domain-during-call,.domainproperty, theError-vs-truthy intercept branch, and anasync-done-shaped integration case. Every assertion was cross-checked against Node v26.3.0.With a gulp 4 project (
gulp.series(del, gulp.src().pipe(gulp.dest()))):Fixes #5923
Fixes #24287
[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file