fix(ext/node): reimplement setImmediate API#30328
fix(ext/node): reimplement setImmediate API#30328bartlomieju merged 28 commits intodenoland:mainfrom
setImmediate API#30328Conversation
WalkthroughThe PR adds a Node-like immediate system: introduces an Immediate class, ImmediateList linked-list queue, kRefed symbol, immediateQueue, and runImmediates in timers.mjs with op_immediate_* integrations. process.ts registers runImmediates via core.setImmediateCallback. timers.ts validates callbacks in setImmediate and tightens clearImmediate to mark destruction, update op counters, and remove entries from immediateQueue. ext/web/02_timers.js removes its setImmediate export. Cargo.toml bumps deno_core 0.370.0 → 0.371.0. Three timer tests were enabled. Sequence DiagramsequenceDiagram
participant App as Application
participant SetImm as setImmediate()
participant Queue as immediateQueue (ImmediateList)
participant Immediate as Immediate instance
participant Bootstrap as Process Bootstrap
participant Runner as runImmediates()
participant Callback as User Callback
participant Ops as op_immediate_* ops
App->>SetImm: setImmediate(fn)
SetImm->>SetImm: validateFunction(fn)
SetImm->>Queue: new Immediate(fn) -> enqueue
SetImm->>Ops: op_immediate_count(true)
Immediate->>Ops: op_immediate_ref_count(true) [if refed]
Bootstrap->>Bootstrap: core.setImmediateCallback(runImmediates)
Note over Bootstrap: event loop tick
Bootstrap->>Runner: runImmediates()
Runner->>Queue: dequeue Immediate
Runner->>Callback: invoke callback with argv
Callback-->>Runner: return
Runner->>Ops: op_immediate_count(false)
Runner->>Ops: op_immediate_ref_count(false) [if was refed]
Estimated code review effort3 (Moderate) — ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/node/polyfills/process.ts (1)
920-970: Remove leftoverconsole.logdebugging from process event wiringThe new
console.logcalls when installing/removinguncaughtExceptionlisteners, when handling"error"events, and when installing the internal unhandledRejection callback will print on every such transition and error. That’s observable behavior, not present in Node, and will be very noisy in real applications and tests.These should be removed or gated behind an internal debug logger/flag before merging:
- Line 929:
console.log("installed uncaughtException1");- Line 953:
console.log("removed uncaughtException");- Line 969:
console.log("process on error uncaughtException");- Line 996:
console.log("installing internal listener");Also applies to: 996-1010
🧹 Nitpick comments (2)
tests/node_compat/config.toml (1)
1190-1205: Nice immediate coverage; consider re-validating the zlib test that was disabled for brokensetImmediateEnabling the new
test-timers-immediate-*cases here is good coverage for the new implementation. Given thatparallel/test-zlib-destroy-pipe.js(around line 1402) is still disabled explicitly “because setImmediate is broken”, it’s worth re-running it with this PR and either re‑enabling it or updating the comment if a different issue remains.ext/node/polyfills/timers.ts (1)
15-21:setImmediate/clearImmediatelogic looks sound; consider minor API polishThe new implementation correctly:
- Validates the callback (
validateFunction(cb, "callback")).- Uses the new
Immediateclass for scheduling so async context and ref/unref semantics are handled in one place.- In
clearImmediate, avoids double‑decrementing by early‑returning if_onImmediateis falsy or_destroyedis already set, and updates bothop_immediate_countandop_immediate_ref_countbefore unlinking fromimmediateQueue.Two small follow‑ups you may want to consider:
Return type annotation
setImmediateis annotated as returningTimeoutbut now returns anImmediateinstance. IfTimeoutandImmediatearen’t intentionally aliased in the type layer, changing the return type here toImmediatewould better document the runtime behavior and avoid surprising TS consumers.Defensive clearImmediate argument handling
clearImmediate(immediate: Immediate)currently assumes a validImmediateobject. If you want closer parity with Node’s “no‑op on invalid handle” behavior, you could add a quick guard:export function clearImmediate(immediate: Immediate | null | undefined) { if (!immediate || !immediate._onImmediate || immediate._destroyed) { return; } // existing body… }That would tolerate stray calls like
clearImmediate(undefined)without throwing.Also applies to: 160-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(2 hunks)ext/node/polyfills/internal/timers.mjs(3 hunks)ext/node/polyfills/process.ts(6 hunks)ext/node/polyfills/timers.ts(2 hunks)ext/web/02_timers.js(0 hunks)tests/node_compat/config.toml(1 hunks)
💤 Files with no reviewable changes (1)
- ext/web/02_timers.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-20T09:12:20.189Z
Learnt from: marvinhagemeister
Repo: denoland/deno PR: 31358
File: ext/node/polyfills/process.ts:759-766
Timestamp: 2025-11-20T09:12:20.189Z
Learning: In ext/node/polyfills/process.ts, the process.sourceMapsEnabled property should be defined with enumerable: true to match Node.js behavior (as seen in Node.js source: lib/internal/bootstrap/node.js).
Applied to files:
ext/node/polyfills/process.ts
🧬 Code graph analysis (3)
ext/node/polyfills/internal/timers.mjs (2)
ext/web/02_timers.js (1)
core(12-15)ext/node/polyfills/_next_tick.ts (1)
runNextTicks(94-106)
ext/node/polyfills/timers.ts (2)
ext/node/polyfills/internal/validators.mjs (1)
validateFunction(289-295)ext/node/polyfills/internal/timers.mjs (7)
cb(73-80)Immediate(314-373)immediate(230-230)kRefed(46-46)kRefed(46-46)immediateQueue(220-220)immediateQueue(220-220)
ext/node/polyfills/process.ts (1)
ext/node/polyfills/internal/timers.mjs (1)
runImmediates(226-312)
🪛 Biome (2.1.2)
ext/node/polyfills/internal/timers.mjs
[error] 17-17: Do not shadow the global "Symbol" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test debug macos-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: bench release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (1)
ext/node/polyfills/process.ts (1)
73-73: WiringrunImmediatesinto the core callback looks correctHooking
runImmediatesviacore.setImmediateCallback(runImmediates)alongsidesetNextTickCallback/setMacrotaskCallbackmatches the new Immediate queue design and should give Node‑likesetImmediatescheduling semantics.Also applies to: 1058-1061
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/node_compat/config.toml (1)
1203-1227: All 21 referenced test files are missing from the repository.The script confirms that none of the timer test files added to config.toml at lines 1203-1227 exist in the codebase. These entries cannot remain in the configuration file without the corresponding test files present. Either the test files need to be added to the repository or their entries removed from config.toml.
🧹 Nitpick comments (1)
ext/node/polyfills/timers.ts (1)
160-166: AlignsetImmediatereturn type withImmediatehandleThe implementation now returns an
Immediateinstance but the signature still advertisesTimeout. GivenclearImmediatetakes anImmediate, it would be clearer and more type-consistent to havesetImmediatereturnImmediate(or loosenclearImmediate’s parameter type) soclearImmediate(setImmediate(...))is cleanly typed.Consider changing the signature along these lines (if it matches the
Immediate/Timeoutrelationship ininternal/timers.mjs):-export function setImmediate( - cb: (...args: unknown[]) => void, - ...args: unknown[] -): Timeout { +export function setImmediate( + cb: (...args: unknown[]) => void, + ...args: unknown[] +): Immediate {The added
validateFunction(cb, "callback")looks good and matches the other timer APIs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ext/node/polyfills/timers.ts(2 hunks)tests/node_compat/config.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (2)
tests/node_compat/config.toml (1)
1197-1201: Two setImmediate tests appropriately deferred with clear TODOs.The disabled tests have documented reasons: one requires code rearrangement and the other depends on proper
node:domainerror handling. These can be addressed in follow-up work.ext/node/polyfills/timers.ts (1)
15-21:clearImmediateguard and counter logic look correct; ensure op balance with constructorThe new
clearImmediatecorrectly no-ops fornull/undefinedor already-destroyed handles, flips_destroyed, clears ref state viakRefed, and removes the item fromimmediateQueuewhile decrementingop_immediate_count(andop_immediate_ref_countonly when still refed). This should makeclearImmediateidempotent and safe.Please double-check that:
- Each
Immediatecreation/run path incrementsop_immediate_count(true)andop_immediate_ref_count(true)in ways that are exactly balanced by thisclearImmediatelogic (and the run path), andimmediateQueue.remove(immediate)is safe to call only for queue members (no double-remove on already-run handles, which you seem to guard via_onImmediate/_destroyedbut is worth confirming against theImmediateimplementation).Also applies to: 168-184
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Cargo.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: build libs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ext/node/polyfills/internal/timers.mjs (2)
18-18: Symbol shadowing lint error persists.Static analysis still flags
Symbolon line 18 as shadowing the global. The previous review suggested renaming toPrimordialSymbol. If the fix was applied in commit d616ad5, this lint configuration may need updating, or the fix may not have stuck.
266-269: Crash whenprevImmediateis undefined.On the first loop iteration,
prevImmediateis undefined. Ifimmediate._destroyedis true (e.g., when resuming fromoutstandingQueueafter an exception handler calledclearImmediate), this will throwTypeError: Cannot read property '_idleNext' of undefined.Apply this fix:
if (immediate._destroyed) { - outstandingQueue.head = immediate = prevImmediate._idleNext; + const next = prevImmediate ? prevImmediate._idleNext : immediate._idleNext; + outstandingQueue.head = immediate = next; continue; }
🧹 Nitpick comments (1)
ext/node/polyfills/internal/timers.mjs (1)
202-202: Remove debug comment.Leftover debug code should be removed.
- // console.log("append", this.tail);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ext/node/polyfills/internal/timers.mjs(4 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
ext/node/polyfills/internal/timers.mjs
[error] 18-18: Do not shadow the global "Symbol" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (1)
ext/node/polyfills/internal/timers.mjs (1)
320-377: Immediate class implementation looks solid.Async context capture, linked list integration, and ref counting with op counters are all handled correctly. The strict
=== false/=== truechecks inref()/unref()combined with settingkRefedtonullinrunImmediates(line 277) prevents double-counting on processed Immediates.
Closes #28016
tick.js:Running server: