Expose Node's Immediate._onImmediate on setImmediate() return value - #31802
Expose Node's Immediate._onImmediate on setImmediate() return value#31802robobun wants to merge 5 commits into
Immediate._onImmediate on setImmediate() return value#31802Conversation
WalkthroughAdds an ChangesImmediate._onImmediate Property Implementation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — verbatim mirror of the existing _onTimeout accessor pattern, and all prior feedback has been addressed.
Extended reasoning...
Overview
This PR exposes Immediate._onImmediate as a prototype getter/setter, matching Node.js. It touches three files: node.classes.ts adds a 5-line proto entry identical in shape to the existing _onTimeout entry; ImmediateObject.rs adds a js module with codegen_cached_accessors! and a get_on_immediate/set_on_immediate pair that are line-for-line copies of TimeoutObject.rs's get_on_timeout/set_on_timeout; and node-timers.test.ts gains 5 focused tests.
Security risks
None. The accessor reads/writes a cached WriteBarrier slot that already existed and was already populated at construction — this just exposes it to JS, exactly as Timeout._onTimeout already does. No new memory management, no new untrusted-input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Low. The implementation is a mechanical copy of an established sibling pattern (TimeoutObject.rs → ImmediateObject.rs, _onTimeout → _onImmediate) with no novel logic. The cached callback slot was already declared in the Immediate class's values array and written by TimerObjectInternals::init; this PR only adds the JS-visible accessor over it.
Other factors
All three of my earlier review threads are resolved: (1) the _onImmediate = null concern was refuted with a verified Node v24.3.0 repro showing Node also throws — so Bun's behavior is correct parity; (2) the AsyncContextFrame wrapper observation is pre-existing on _onTimeout and out of scope; (3) the test-validity nit was fixed in 4ae8b3e by asserting no own-property is created after the write. A 5th test ("fires the reassigned callback") was added in fa878ce to prove the setter actually feeds dispatch. The CI failures (musl LTO link error, bunx.test.ts on asan) are unrelated to timers. No CODEOWNERS cover these paths.
|
CI note for reviewers: the red lanes on the prior run are unrelated to this diff (a Node timer accessor — no platform, install, network, or Bake code). Verified from the job logs:
No failure references |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/timers/node-timers.test.ts`:
- Around line 196-203: Wrap the test body that creates the immediate in a
try/finally and call clearImmediate(immediate) in the finally block to match
other tests' cleanup; specifically, keep the Promise.withResolvers usage and the
reassignment of immediate._onImmediate (the resolve/reject logic) but ensure you
always call clearImmediate(immediate) in a finally to defensively clean up the
immediate created by setImmediate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 500318c2-2602-4673-b7b3-b255644d28a5
📒 Files selected for processing (1)
test/js/node/timers/node-timers.test.ts
There was a problem hiding this comment.
LGTM — verbatim mirror of the existing _onTimeout accessor pattern; prior feedback addressed, and the remaining clearImmediate nulling note is a pre-existing gap shared with _onTimeout that's fine as a follow-up.
Extended reasoning...
Overview
This PR adds an _onImmediate getter/setter to the Immediate prototype for Node.js compat (issue #31801). It touches three files: a 5-line addition to node.classes.ts (proto entry identical in shape to the adjacent _onTimeout), ~30 lines in ImmediateObject.rs (a codegen_cached_accessors! module + getter/setter that read/write the existing cached callback slot — copied verbatim from TimeoutObject.rs:65-75), and 5 new tests in node-timers.test.ts.
Security risks
None. This exposes a read/write accessor for a callback slot that was already stored and already invoked by the dispatch path; no new data flows, no auth/crypto/permissions surface, no untrusted input parsing.
Level of scrutiny
Low. The implementation is a mechanical copy of the established _onTimeout pattern in the same module — same macro, same slot, same signatures. The cached callback slot was already populated at init and already consumed by run_immediate_task; this PR only adds a JS-visible accessor over it. No event-loop, scheduling, or lifetime logic changes.
Other factors
All three prior inline concerns are resolved: the null-_onImmediate behavior was verified against Node v24.3.0 (Node also throws, so Bun's behavior is correct as-is); the AsyncContextFrame wrapper note is acknowledged as a pre-existing _onTimeout limitation and out of scope; and the test-validity nit was fixed in 4ae8b3e (the writable test now asserts no own-property is created, and a new "fires the reassigned callback" test directly proves the setter writes the dispatch slot). The one remaining inline nit (clearImmediate doesn't null the slot) is explicitly low-impact and applies equally to the pre-existing clearTimeout/_onTimeout path — reasonable to fix both together in a follow-up. CI failures (bunx.test.ts, x64-musl link) are unrelated to timers.
|
CI status — diff is green; the only red is unrelated flake, needs a maintainer to merge. I've now seen this across two runs (#60302, #60303). Every build lane (rust/cpp/bun, all platforms) passes. The sole failing test is I've already used my one re-roll, so I'm not pushing another retrigger (that would just be noise). This needs a maintainer to merge past the flaky install test, or a re-run of that lane. |
|
CI status (final): the diff is green; both red lanes are confirmed external and already tracked.
|
setImmediate() returns an Immediate object; Node exposes the scheduled callback on its _onImmediate property, mirroring Timeout._onTimeout. Bun's Immediate had no such accessor, so immediate._onImmediate was undefined. Node-based test suites and inspection logic rely on it. The callback is already stored in the cached "callback" slot at construction (TimerObjectInternals::init), so this adds an _onImmediate getter/setter to the Immediate class that reads/writes that slot, exactly mirroring Timeout._onTimeout.
Adds coverage proving the writable _onImmediate accessor is authoritative over dispatch: reassigning it before the immediate fires swaps which callback runs, matching Node. Fails on baseline (plain-property assignment is ignored by dispatch, so the original fires).
Strengthens the writable test so it exercises set_on_immediate rather than passing trivially: without the accessor, assignment creates an own expando data property, so asserting no own property exists after the write makes the test fail on a runtime lacking the accessor.
df10bde to
e580ac7
Compare
There was a problem hiding this comment.
LGTM — verbatim mirror of the existing _onTimeout accessor pattern, and all prior review threads are resolved.
Extended reasoning...
Overview
This PR adds an _onImmediate getter/setter to the Immediate prototype for Node compatibility, fixing #31801. Three files touched: a 5-line addition to node.classes.ts (the proto entry), ~25 lines in ImmediateObject.rs (a js module with codegen_cached_accessors! plus get_on_immediate/set_on_immediate), and 5 new tests in node-timers.test.ts. The Rust implementation is byte-for-byte the same as TimeoutObject.rs's get_on_timeout/set_on_timeout — it reads/writes the cached callback slot that TimerObjectInternals::init already populates.
Security risks
None. This exposes a read/write accessor for a callback slot the user already controls (they passed it to setImmediate). The setter lets JS overwrite it, but that's identical to the long-shipping _onTimeout setter and matches Node's behavior. No auth, crypto, filesystem, or network surface.
Level of scrutiny
Low. The change is mechanical pattern replication: the _onTimeout accessor on Timeout already exists with the exact same shape ({getter, setter, this: true} in node.classes.ts, cached-slot read/write in Rust), and this PR copies it onto Immediate. There's no novel logic or design decision — the callback is already stored in the slot at construction; this just exposes it. No CODEOWNERS cover these paths.
Other factors
I previously raised four points on this PR; all are resolved. The author refuted my null-assignment concern with empirical Node verification (Node also throws on im._onImmediate = null, so Bun's behavior matches), fixed the test-validity nit by adding a getOwnPropertyDescriptor assertion (and a "fires the reassigned callback" dispatch test), and correctly scoped the two pre-existing _onTimeout parity gaps (AsyncContextFrame wrapper, post-clear nulling) to a follow-up. CodeRabbit's one nitpick was correctly declined. The bug-hunting system found nothing this run. CI is green for the change itself — the only red is a CI-wide bunx.test.ts npm-registry flake unrelated to timers, and the ASAN lane passed the new tests.
1 participant
Fixes #31801.
Repro
Node:
Bun (before):
Cause
setImmediate()returns anImmediateobject. Node exposes the scheduled callback on its_onImmediateproperty — the siblingTimeoutobject already does the equivalent with_onTimeout. Bun'sImmediateclass had no_onImmediateaccessor, so the property read backundefined._onImmediateisn't a spec requirement, but Node-based test suites and internal inspection logic read it.Fix
The callback is already stored in the cached
callbackslot at construction (TimerObjectInternals::initwrites it viaJSImmediate::callback_set_cachedunconditionally). This mirrorsTimeout._onTimeoutforImmediate:src/runtime/node/node.classes.ts— add an_onImmediategetter/setter to theImmediateproto (same shape as_onTimeout).src/runtime/timer/ImmediateObject.rs— add ajsmodule withcodegen_cached_accessors!("Immediate"; arguments, callback,)andget_on_immediate/set_on_immediatereading/writing the cachedcallbackslot — verbatim theTimeoutObject.rspattern._onImmediateis exposed as a prototype accessor (getter + setter), identical to how_onTimeoutis exposed, and returns the exact callback the user passed.Verification
New tests in
test/js/node/timers/node-timers.test.ts(_onImmediatedescribe):typeofisfunction, identity=== fn)Timeout._onTimeoutTimeout._onTimeout3 of 4 fail on
USE_SYSTEM_BUN=1 bun test(returningundefined) and all 4 pass withbun bd test. Fullnode-timers.test.tsis green (24/24).