node:timers/promises: implement setInterval as async generator (lazy arm + iterator protocol) - #34619
node:timers/promises: implement setInterval as async generator (lazy arm + iterator protocol)#34619robobun wants to merge 4 commits into
Conversation
|
Status: Ready to merge. The evidence gate passed on bc59b8c ("test fails without the fix and passes with it on ASAN and release builds"). Code review is clean (no open threads). CI (#81059): 6 Local verification: 15/15 pass with the fix, 8 fail with |
WalkthroughChangesTimers Promises Interval
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — the async-generator rewrite is essentially Node's own setInterval implementation, and the generator runtime handles the queued-next()/return()/throw()/done-latch semantics the hand-rolled iterator got wrong. The two inline nits are test-hygiene only (cleanup ordering, timing margin) and don't affect the implementation.
Extended reasoning...
Overview
Replaces the hand-rolled async-iterator object in src/js/node/timers.promises.ts setInterval() with a real async function*, and adds seven tests in test/js/node/timers.promises/timers.promises.test.ts covering concurrent next(), return(v) shape, post-return next(), buffered-tick discard, second for await, throw(), and abort→next(). Net ~90 lines deleted; the asyncIterator helper and its only callers are removed in the same commit.
Correctness
I traced the generator against each scenario in the PR table plus a few edges the tests don't hit directly:
return()while suspended at the internalawait(not atyield): the queued return is processed after the next tick resolves the await and control reachesyield, thenfinallyclears the interval — no leak.- Abort while suspended at
yield(callback undefined):onCancelclears the interval; on resume thewhile (!signal?.aborted)check falls through to the trailingthrow $makeAbortError. Buffered ticks drain first, which matches Node. return()before firstnext(): generator body never starts, nothing to clean up.- Validation errors now throw synchronously inside the generator body → surface as a rejected first
next(), which is whattest-timers-interval-promisified.jsasserts (assert.rejects(() => iterable[Symbol.asyncIterator]().next(), /ERR_INVALID_ARG_TYPE/)). { once: true }on the abort listener is fine —getEventListeners(signal, 'abort').length === 1in the Node parallel test still holds untilfinallyremoves it.
Resolving the pending await with Promise.$reject(...) on abort is the same trick Node uses (resolve-with-rejected-thenable → the await throws), and finally runs on every exit path (return/throw/abort).
Security risks
None. Pure JS iterator-protocol change; no new external input surface. Uses the same primordial patterns (Promise.$reject, $makeAbortError) as the surrounding setTimeout/setImmediate code.
Level of scrutiny
Medium — Node-compat surface, but the change is a simplification that converges on the reference implementation rather than diverging from it. The upstream test/js/node/test/parallel/test-timers-interval-promisified.js exercises validation, pre-aborted signal, mid-iteration abort, ref: false, and listener cleanup, and the PR states it still passes.
Other factors
The two inline findings are both [Nit]-level test hygiene: a missing try/finally around assertions (leaks a harmless 10ms counter-increment interval only on assertion failure) and a 5× timing margin on the buffered-ticks precondition. Neither affects the implementation or introduces flakiness on the happy path, so I don't consider them blocking.
There was a problem hiding this comment.
LGTM — the two earlier nits (try/finally cleanup on the concurrent-next test, wider buffered-tick margin) are addressed in 3173345.
What was reviewed:
- New
async function*body traced against Node'slib/timers/promises.js— samenotYielded/callbackloop, same abort-via-resolve(rejectedPromise)shape,finallyclears interval + listener on every exit. - Checked that validation errors still surface on first
next()(not synchronously) as before, matching Node's async-generator semantics. - Verified
onCancel'sPromise.$reject(...)passed toresolvegets a handler attached via the resolution microtask, so no unhandled-rejection regression vs the existing setTimeout/setImmediate paths. test-timers-interval-promisified.jsstill enabled intest/js/node/test/parallel/; the 7 new cases each map to a row in the divergence table.
Extended reasoning...
Overview
Rewrites setInterval in src/js/node/timers.promises.ts from a hand-rolled { next, return, [Symbol.asyncIterator] } object to a real async function*, and adds 7 protocol-conformance tests in test/js/node/timers.promises/timers.promises.test.ts. Net −100/+30 in the source file; the async-generator runtime now provides next() queueing, return(v) shape, throw(), and done-latching, with a try/finally guaranteeing clearInterval and abort-listener removal on every exit path.
Security risks
None. Pure JS, no untrusted-input parsing beyond the existing validateNumber/validateObject/validateAbortSignal/validateBoolean calls (unchanged). No new allocation driven by external size fields.
Level of scrutiny
Medium — a node: compat behavioural change, but the new body is a near-verbatim port of Node's own lib/timers/promises.js setInterval (same notYielded counter, same callback slot resolved by tick or by PromiseReject(abortError), same while(!signal?.aborted) loop, same post-loop throw AbortError). The interesting edge cases — abort while suspended at yield vs at await, buffered ticks after abort, validation-error timing — all reduce to Node's exact control flow, so "matches the reference implementation" carries most of the correctness argument here.
Other factors
- The PR author addressed both prior inline nits in 3173345 (concurrent-next test now cleans up in
finally; buffered-tick test uses a 1 ms interval + a second awaitednext()for a 50× margin). - Existing Node parallel test
test-timers-interval-promisified.jsremains in the enabled set and covers pre-aborted signal, invalidref/signal/options, abort mid-iteration, andfor awaitbreak — the surface the old implementation already handled. - The dead
asyncIteratorhelper andsymbolAsyncIteratorcapture are deleted in the same PR, per the "delete dead code in the same PR that makes it dead" rule. bound()'s losing 500 mssetTimeoutisn't cancelled on race resolution, but it's a bounded ref'd timer that just fires and resolves into the void — not a leak, and each test's total wall time stays well under the ~1 s budget.
There was a problem hiding this comment.
LGTM — both earlier nits are addressed (try/finally cleanup in the concurrent-next test, and the buffered-ticks test now uses a 1ms interval with an extra awaited next() for a ~50× margin).
What was reviewed:
- New
setIntervalbody is a near-line-for-line port of Node'slib/timers/promises.jsasync generator — samenotYielded/callbackmechanism, samewhile (!signal?.aborted)loop, same abort-via-resolve(rejectedPromise)andfinallycleanup. - Checked validation-error semantics: throws inside the generator body still surface as a rejected first
next()(async generator doesn't run until firstnext()), matching both the old behavior and Node —test-timers-interval-promisified.jscovers this. - Confirmed the removed
asyncIteratorhelper was file-local; no other callers.
Extended reasoning...
Overview
Rewrites node:timers/promises setInterval() from a hand-rolled async-iterator object to a real async function*, fixing seven documented protocol divergences from Node (concurrent next() hangs, wrong return() shape, missing throw(), no done-latch, etc.). The new body is essentially a direct port of Node's implementation. The now-unused file-local asyncIterator helper and symbolAsyncIterator constant are deleted. Seven new tests in timers.promises.test.ts cover each divergence; the existing Node parallel test test-timers-interval-promisified.js continues to exercise validation errors, listener cleanup, ref: false, and abort-at-various-points.
Security risks
None. Pure JS timer/iterator plumbing; no parsing of untrusted input, no auth, no filesystem/network.
Level of scrutiny
Medium. This is user-facing Node-compat surface in a built-in module (src/js/), so tamper-resistance and correctness matter, but the change reduces bespoke logic by delegating iterator-protocol semantics (queued next(), return(v) shape, throw(), done-latch) to the async-generator runtime — strictly less to get wrong. The remaining hand-written logic (tick counter, abort wiring, finally cleanup) matches Node's reference line-for-line, which per REVIEW.md is the spec for ported code.
Other factors
- I traced the abort paths against Node's source: abort while awaiting →
callback(Promise.$reject(...))rejects the awaited promise → generator throws →finallycleans up → pendingnext()rejects; abort while suspended atyield→onCancelclears interval, nextnext()resumes intowhile (!signal?.aborted)→ throws. Both match Node. - Validation moved from try/catch-return-rejecting-iterator to plain throws inside the generator body; observably identical (async generator body runs on first
next()), and covered by the Node parallel test'sassert.rejects(() => iterable[Symbol.asyncIterator]().next(), /ERR_INVALID_ARG_TYPE/)block. - Both nits from my earlier review are resolved in 3173345 and visible in the current diff.
- Evidence block shows 7/7 new tests fail on the pre-fix debug build and pass post-fix on both debug+ASAN and release.
The hand-rolled async iterator armed its underlying interval eagerly at
setInterval() call time, so an iterator that was created but never
iterated kept the event loop alive forever. Node arms the interval
lazily on the first next() call (async generator body semantics).
Replacing the hand-rolled iterator with a real async generator matches
Node's implementation and fixes several other protocol divergences at
the same time: concurrent next() calls now all settle, return(v)
resolves { value: v, done: true }, next() after return() resolves
{ done: true }, throw() is present, and abort followed by next()
resolves { done: true } instead of rejecting again.
2410cee to
df61ee1
Compare
There was a problem hiding this comment.
LGTM — this is a near line-for-line port of Node's own async function* setInterval, and both earlier test nits (try/finally cleanup, 1ms interval for the buffered-ticks precondition) are addressed.
What was reviewed
- Diffed the new generator body against
nodejs/node lib/timers/promises.js— control flow (lazy arm,notYieldedcounter,onCancelrejecting the pending await,while/for/throw/finally) matches exactly. - Checked the abort-while-suspended-at-
yieldpath: buffered ticks drain then AbortError throws on the next loop check, same as Node. - Confirmed
test-timers-interval-promisified.jsstill exercises the validation paths (invalidref/signal/optionsreject on firstnext()). - The comment-cop flag on this file is a false positive — the 2-line "only validate non-number inputs" note mirrors the identical pre-existing comment on
setTimeoutabove it.
Extended reasoning...
Overview
Rewrites setInterval in src/js/node/timers.promises.ts from a hand-rolled async-iterator object to a real async function*, and adds 8 tests to test/js/node/timers.promises/timers.promises.test.ts. The generator body is a direct port of Node's reference implementation — I diffed it against nodejs/node/lib/timers/promises.js and the control flow is identical: lazy interval arming inside try, notYielded tick counter, callback slot resolved by either the interval tick or onCancel (which passes a rejected promise so the await throws), while (!signal?.aborted) / drain-for / trailing throw AbortError / finally cleanup. The only deltas are Bun-local spellings (setIntervalGlobal, $makeAbortError, resistStopPropagation, Promise.$reject) and the pre-existing after = 1 default + unconditional validateAbortSignal/validateBoolean calls, which the old implementation already did.
Security risks
None. Pure JS iterator-protocol change in a Node-compat module; no parsing of untrusted data, no native code, no auth/crypto surface.
Level of scrutiny
Moderate — user-facing Node-compat behavior, but the fix is "replace bespoke iterator with the language's built-in async generator that Node itself uses", which structurally eliminates the whole bug class (single-resolver slot, missing throw(), wrong return() shape, no done-latch, eager arming). Per REVIEW.md's ported-code rule, the Node reference is the spec here, and this matches it.
Other factors
- My two earlier nits are resolved: the concurrent-
next()test now wraps assertions intry/finally { await it.return() }, and the buffered-ticks test uses a 1ms interval with two awaitednext()calls before the 50ms sleep (50× margin, precondition established). - New tests cover the full divergence matrix from the PR description; the lazy-arm test spawns a subprocess with an unref'd 2s sentinel and asserts
exitCode === 0/ noSTILL_ALIVEoutput, so a regression would fail rather than hang. bound()usesPromise.racewith a 500ms fallback so hung promises fail assertions instead of timing out the file.- CI reds (
bake/devtimeouts on macOS x64,node-net.test.tsmimalloc threshold) are unrelated — none touchtimers/promisesand the failure sets differ across builds. - The
comment-copbot flagged a "paragraph-long comment" on this file; the only added comment is a 2-line note identical in substance to the one already onsetTimeoutin the same file — not a workaround justification, just explaining thetypeof after !== 'number'guard. Not actionable.
There was a problem hiding this comment.
LGTM — the two test-hygiene nits from my earlier pass are addressed, and the comment-cop note is resolved.
What was reviewed:
setIntervalgenerator body against Node's reference shape (lazy arm on firstnext(),notYieldedbuffer, abort viaresolve(rejectedPromise),finallycleanup on all exits) — matches.- Abort while suspended at
yield(not atawait): buffered ticks drain thenwhile (!signal?.aborted)falls through to the throw — same as Node. - Test hermeticity:
try/finallyaround concurrent-next()assertions, subprocess pipes drained concurrently,bound()timers self-clear so no leaks.
Extended reasoning...
Overview
Rewrites setInterval in src/js/node/timers.promises.ts from a hand-rolled async-iterator object to a real async function*, matching Node's implementation. Net -70/+25 lines in src. Adds 8 tests in test/js/node/timers.promises/timers.promises.test.ts covering lazy arming, concurrent next(), return(v) shape, post-return next(), buffered-tick discard, second for await, throw(), and abort-then-next(). Deletes the now-unused asyncIterator helper and symbolAsyncIterator constant.
Security risks
None. Pure JS timer/iterator semantics; no I/O, parsing of untrusted input, auth, or crypto. The { __proto__: null, once: true } listener options and captured setIntervalGlobal follow existing tamper-resistance patterns in the file.
Level of scrutiny
Medium-low. This is a Node-compat correctness fix in a small, self-contained builtin module. The new implementation is a near-verbatim port of Node's lib/timers/promises.js async generator (same notYielded counter, same while (!signal?.aborted) / inner for loop, same callback(PromiseReject(...)) abort path, same finally cleanup). The async-generator runtime provides the iterator-protocol guarantees (queued next(), return(v) shape, done-latching, throw()) that the old hand-rolled object got wrong, so most of the fix is deleting bespoke code in favor of language semantics.
Other factors
- My two prior nits (leak-on-assertion-failure in the concurrent-
next()test; thin timing margin in the buffered-ticks test) were both addressed in 3173345 and are visible in the current diff. - The comment-cop flag on the duplicated delay-validation comment was addressed in c80a2a1.
test/js/node/test/parallel/test-timers-interval-promisified.jscontinues to pass per the CI status; remaining CI red is unrelated (bake/dev, node-net mimalloc threshold).- I traced the one non-obvious path — abort firing while the generator is suspended at
yieldrather than at theawait— and confirmed it drains buffered ticks then throwsAbortError, which is exactly Node's behavior for the same code shape. - The subprocess lazy-arm test has a proper hang guard (unref'd 2s watchdog that exits 1 with
STILL_ALIVE), so a regression fails cleanly rather than hanging CI.
There was a problem hiding this comment.
LGTM — the async-generator rewrite mirrors Node's lib/timers/promises.js structure and the earlier review nits (try/finally cleanup, tighter buffered-tick margin, comment-cop) are all addressed.
What was reviewed:
- Generator control flow: abort-while-awaiting (resolve with rejected promise → await throws), abort-while-yielding (falls through to post-loop
throw), andfinallyclearing the interval on every exit path. - Validation timing: errors now surface on first
next()via generator-body throw — same observable behavior as before and matches Node. - New tests: subprocess lazy-arm check, concurrent
next(),return/throw/abort protocol, buffered-tick discard;bound()timers are ref'd but bounded at 500ms so no hang risk.
Extended reasoning...
Overview
This PR replaces the hand-rolled async-iterator object in src/js/node/timers.promises.ts's setInterval with a real async function*, and adds eight tests in test/js/node/timers.promises/timers.promises.test.ts covering the iterator-protocol divergences the old implementation had from Node (lazy arming, concurrent next() queueing, return(v) shape, done-latching, throw(), abort-then-next()). The now-unused asyncIterator helper and symbolAsyncIterator constant are deleted.
Security risks
None. This is a pure-JS Node-compat shim over the existing global setInterval; no native code, auth, crypto, or untrusted-input parsing is touched.
Level of scrutiny
Medium — it rewrites a user-facing node:timers/promises API, but the new body is a near-verbatim port of Node's own async-generator implementation, and the async-generator runtime supplies most of the corrected semantics (next() queueing, return shape, done-latch, throw) for free rather than via new hand-written logic. I traced the two abort paths: (a) abort during await — onCancel resolves the pending promise with a rejected promise, so await rethrows and the finally clears the interval; (b) abort during yield — callback is undefined, so the generator resumes, the while (!signal?.aborted) guard fails, and the trailing throw $makeAbortError fires. The post-loop signal?.reason is safe because that line is only reachable when signal exists and is aborted (with no signal the loop condition is always true). Validation moving from factory-time try/catch-return-rejecting-iterator to generator-body throws is behavior-equivalent: both reject the first next(), and Node does it the new way.
Other factors
My two prior inline nits (try/finally around the concurrent-next() assertions; widening the buffered-tick interval margin) were applied in 3173345, and the comment-cop duplicated-comment note was addressed in c80a2a1. The vendored Node parallel test test-timers-interval-promisified.js still exercises this path. The bound() helper leaves its 500ms fallback timers un-cancelled, but they're ref'd, bounded, and small in number — not a hang or flake risk. No outstanding reviewer comments remain.
Problem
The
timers/promisessetInterval()async iterator was a hand-rolled object that diverged from Node in several ways.Eager arming pins the event loop. The interval is created inside
setInterval()itself, before the iterator is ever consumed, so an iterator that is created but abandoned keeps the process alive forever. Node uses an async generator, whose body runs on the firstnext():Iterator protocol gaps. Separately, the hand-rolled object had a single pending-resolver slot and no completion latch:
next()it.return(v){ value: v, done: true }{}next()afterreturn(){ done: true }next()afterreturn()with buffered ticks{ done: true }{ done: false, value }from closed iteratorfor awaitover same iteratortypeof it.throw"function""undefined"next()after abort rejection{ done: true }Cause
src/js/node/timers.promises.tsbuilt the iterator by hand and calledsetIntervalGlobal(...)at factory time:Fix
Replace the hand-rolled iterator with a real
async function*, matching Node's implementation. The generator body runs lazily on the firstnext(), so an abandoned iterator never arms an interval. The async generator runtime provides correctnext()queueing,return(v)shape,throw(), and done-latching for free, and thetry/finallyclears the interval on every exit path.Verification
New tests in
test/js/node/timers.promises/timers.promises.test.tscover the event-loop pin (subprocess exits 0 with an un-iterated iterator), concurrentnext(),return(v)shape, post-returnnext(), buffered ticks after return, secondfor await,throw(), and abort-then-next(). All fail on currentmainand pass with this change.test/js/node/test/parallel/test-timers-interval-promisified.jscontinues to pass.[review] gate passed · iteration 7 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 7
evidence per changed file