fix(ext/node): bind setImmediate callback this to the Immediate instance#33716
Conversation
0de5876 to
1db3f80
Compare
|
Force-pushed |
nathanwhitbot
left a comment
There was a problem hiding this comment.
Self-review of `1db3f80a6`:
`Immediate` callback `this` parity with Node (`lib/internal/timers.js:498-507`):
Node's invocation site (line 504/506):
```js
if (!argv)
immediate._onImmediate();
else
immediate._onImmediate(...argv);
```
This is a method call — `this === immediate`. Our previous Deno polyfill used:
```js
return ReflectApply(unboundCallback, globalThis, argv);
```
which set `this === globalThis` instead. Per Node, `setImmediate(function () { this })` should observe the Immediate instance, not the global.
Fix captures `this` in the constructor (before defining the inner closure) and passes it through:
```js
const self = this;
const callback = (...argv) => {
...
return ReflectApply(unboundCallback, self, argv);
};
```
✓ matches Node's observable behavior.
Why `Timeout` was already correct:
`Timeout`'s `invokeCallback` already does `ReflectApply(currentCb, self, args)` where `self` is the Timeout instance, so `setTimeout`/`setInterval` were unaffected — only the `Immediate` path was wrong. The arrow-function callback in `Immediate` shadowed `this` lexically (resolving to outer scope = module scope = `undefined`/`globalThis` in strict mode), which is why the constructor-level `const self = this` capture is needed.
Test enrollment: `parallel/test-timers-this.js` is alphabetically positioned between `test-timers-socket-timeout-removes-other-socket-unref-timer.js` and `test-timers-throw-when-cb-not-function.js`. The test asserts `this === handler` for all three of `setTimeout` / `setInterval` / `setImmediate`. ✓
LGTM, holding to COMMENT until CI completes.
nathanwhitbot
left a comment
There was a problem hiding this comment.
Single-commit, minimal diff. Confirmed Node behavior at lib/internal/timers.js#L502-506 — the dispatcher calls immediate._onImmediate(...argv) as a method, so this is the Immediate instance, not globalThis. PR's const self = this; ReflectApply(unboundCallback, self, argv) produces the matching binding. LGTM.
fibibot
left a comment
There was a problem hiding this comment.
Substance is right. Verified against Node's lib/internal/timers.js:
immediate._onImmediate(...argv); // method call → `this === immediate`Pre-PR Deno: ReflectApply(unboundCallback, globalThis, argv) (globalThis as this). Post-PR: captures this into self in the Immediate constructor and passes it to ReflectApply, matching Node's method-call semantics. ✓
The closure-capture pattern (const self = this; before defining the inner arrow function) is the standard idiom and is already what Timeout.invokeCallback (cited in the body) does, so the fix brings setImmediate in line with the existing setTimeout / setInterval behavior.
Test enrollment alphabetically positioned correctly (test-timers-socket-timeout-removes-other-socket-unref-timer < test-timers-this < test-timers-throw-when-cb-not-function). ✓
CI: 80 success, 0 failure, 24 still pending. Holding to COMMENT per the CI-must-complete rule. Substance is a 5-line one-liner; happy to flip to APPROVE on the next iteration once the relevant test node_compat debug shards land green.
Summary
The
Immediateclass ininternal/timers.mjsinvoked the user callback withReflectApply(unboundCallback, globalThis, argv), sosetImmediate(function () { this })sawWindow/globalThis. Node'slib/internal/timers.jscallsimmediate._onImmediate(...argv)(method call), sothisis theImmediateinstance.Fix: capture
thisin the constructor (before defining the inner closure) and pass it toReflectApply.Timeoutalready does this correctly via itsinvokeCallback(ReflectApply(currentCb, self, args)), sosetTimeout/setIntervalwere unaffected — onlysetImmediatewas wrong.Enables
parallel/test-timers-this.js(which assertsthis === handlerfor all ofsetTimeout/setInterval/setImmediate).Test plan
cargo test --test node_compat -- test-timers-thispasses.cargo test --test node_compaton Linux — only pre-existing flaky failures (test-process-threadCpuUsage-worker-threads.js,test-fs-promises-file-handle-readFile.js,test-dgram-send-cb-quelches-error.js,test-child-process-uid-gid.js,test-net-autoselectfamily.js); no regressions in any of the existingsetImmediatetests.