fix(ext/node): improve Node.js timer compatibility#33479
Conversation
Node.js Timeout objects support a close() method to cancel the timer, and respect _idleTimeout = -1 or _onTimeout = null set during a repeating timer callback to stop the interval from firing again.
Node.js allows converting a setTimeout into an interval by setting _repeat on the Timeout object inside the callback. Rename _isRepeat to _repeat to match Node.js internals, and reschedule the timer as repeating when _repeat is set after a one-shot callback fires.
Node.js scheduler is an instance of a class with a throwing constructor, so `new scheduler.constructor()` throws ERR_ILLEGAL_CONSTRUCTOR. Use ObjectCreate(Scheduler.prototype) to create the singleton without calling the constructor.
Node.js internal/util exports a synchronous sleep function using Atomics.wait on a SharedArrayBuffer. Lazily initialize the buffer to avoid issues during V8 snapshot creation.
Add getLibuvNow() to the timers internal binding (backed by performance.now()) to support test-timers-ordering. Ignore test-timers-now which requires V8 natives syntax.
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
The _isRepeat → _repeat rename in Timeout (correct, matches Node's internal name) breaks the tests/specs/run/node_prefix_missing spec because its node_globals.out fixture has a hard-coded _isRepeat: false, line that the inspector-output match is looking for. Every test specs (1/2) and (2/2) job on linux/macos/windows × x86_64/aarch64 × debug/release fails with pattern match failed on unstable_node_globals_enabled2 for that reason.
Fix is a one-line update to tests/specs/run/node_prefix_missing/node_globals.out (rename _isRepeat to _repeat to match the new field name). Once that's in, CI should be green — the rest of the timer changes (Timeout.close(), invokeCallback extraction, scheduler illegal-constructor, sleep via Atomics.wait, getLibuvNow via performance.now) all look correct against Node's internals.
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM. The new commit 84a5303c updates tests/specs/run/node_prefix_missing/node_globals.out exactly as I requested in my prior review — single-line fixture change from _isRepeat: false, → _repeat: false, to match the renamed Timeout field. The original CI failure across all test specs jobs is now resolved.
The test node_compat jobs are still red, but the failure is parallel/test-webstream-structured-clone-no-leftovers.mjs — the unrelated regression introduced by #33465 that #33482 is currently ignoring. Not introduced by this PR.
2 participants
Summary
Fixes several Node.js timer compatibility issues, enabling 13 more
node_compat tests to pass (and ignoring 1 untestable):
close()method onTimeoutobjects_idleTimeout = -1and_onTimeout = nullset inside interval callbacks to stop repeating
setTimeouttosetIntervalby setting
_repeatinside the callback (renamed_isRepeatto_repeatto match Node.js internals)
new scheduler.constructor()now throwsERR_ILLEGAL_CONSTRUCTORmatching Node.js behaviorsleep()usingAtomics.waitperformance.now()test-timers-now.js(requires V8 natives syntax)Test plan
./x test-compat parallel::test-timerspasses 48/58 (was 44/58)./tools/lint.js --jspasses./tools/format.jspasses