v20.16.0 proposal by marco-ippolito · Pull Request #53945 · nodejs/node
and others added 30 commits
July 19, 2024 02:59Refs: #53212 (comment) PR-URL: #53233 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Some timer values such as `setImmediate` and `clearImmediate` are missed. And `milliseconds` which is argument of `timers.tick()` is optional and default is 1. Refs: #49534 (comment) PR-URL: #52969 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: #53008 Refs: nodejs/TSC#1550 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #53014 Reviewed-By: Luke Karrys <luke@lukekarrys.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This video(https://www.youtube.com/watch?v=HW0RPaJqm4g) isn't available anymore. And I couldn't find a proper github code review tutorial clip yet. PR-URL: #52982 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
There is no need to explicitly allow copy constructor and copy assignment, and some of these functions should be marked as const. PR-URL: #52973 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The current ninja build does not work with `--node-builtin-modules-path` flag without passing `--ninja` as it will use `make` to build from scratch again. PR-URL: #53007 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
For historical reasons, the second argument of SSL_CTX_set_timeout is a signed integer, and Node.js has so far passed arbitrary (signed) int32_t values. However, new versions of OpenSSL have changed the handling of negative values inside SSL_CTX_set_timeout, and we should shield users of Node.js from both the old and the new behavior. Hence, reject any negative values by throwing an error from within createSecureContext. Refs: openssl/openssl#19082 PR-URL: #53002 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
PR-URL: #53020 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
give appropriate permissions to the following scripts: * permission-processhas-fs-read.js: 'ChildProcess' permission * permission-startup.js: 'FileSystemRead' permission > Paths delimited by comma (,) are no longer allowed. Refs: https://github.com/nodejs/node/blob/main/doc/api/cli.md#--allow-fs-read Signed-off-by: Ryan Qian <i@bitbili.net> PR-URL: #51528 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #53004 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
The node:assert module contains several top level APIs that do not make sense to expose as methods on t.assert. Examples include AssertionError and CallTracker. This commit removes such APIs from t.assert. Refs: #52860 PR-URL: #53049 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #53052 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
PR-URL: #53053 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: #52763 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
PR-URL: #52948 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: #53034 Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: #53064 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
`String.prototype.substr` is deprecated, and using it will raise an error when using ESLint 9+. Co-authored-by: =?UTF-8?q?Micha=C3=ABl=20Zasso?= <targos@protonmail.com> PR-URL: #53070 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Tierney Cyren <hello@bnb.im> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: #53073 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #53073 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #53065 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: #52646 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito added a commit that referenced this pull request
Jul 21, 2024Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 doc: * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53945
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 doc: * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53945
marco-ippolito added a commit that referenced this pull request
Jul 24, 2024Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 doc: * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53945
This was referenced
Feb 6, 2025lubieowoce added a commit to vercel/next.js that referenced this pull request
Feb 6, 2025Potential fix for a leak reported in #74855 on older node versions (see [comment](#74855 (comment))). ### Background When running middleware (or other edge functions) in `next start`, we wrap them in an edge runtime sandbox. this includes polyfills of `setTimeout` and `setInterval` which return `number` instead of `NodeJS.Timeout`. Unfortunately, on some older node versions, converting a `NodeJS.Timeout` to a number will cause that timeout to leak: nodejs/node#53335 The leaked timeout will also hold onto the callback, thus also leaking anything that was closed over (which can be a lot of things!) ### Solution Ideally, users just upgrade to a Node version that includes the fix: - [node v20.16.0](nodejs/node#53945) - [node v22.4.0](nodejs/node#53583) - node v23.0.0 But we're currently still supporting node 18, so we can't necessarily rely on that. Luckily, as noted in the description of the nodejs issue, calling `clearTimeout` seems to unleak the timeout, so we can just do that after the callback runs! ### Unrelated stuff I did While i was at it, I also fixed a (very niche) discrepancy from how `setTimeout` and `setInterval` behave on the web. when running the callback, node sets `this` to the Timeout instance: ```js > void setTimeout(function () {console.log('this in setTimeout:', this) } ) undefined > this in setTimeout: Timeout { ... } ``` but on the web, `this` is always set to `globalThis`. Our wrapper now correctly does this. ### Testing <details> <summary>Collapsed because it's long</summary> Verifying this is kinda tricky, so bear with me... Here's a script that can verify whether calling `clearTimeout` fixes the leak by using a FinalizationRegistry and triggering GC to observe whether memory leaked or not. `setTimeoutWithFix` is a simplified version of `webSetTimeoutPolyfill` from the PR. ```js // setTimeout-test.js if (typeof gc !== 'function') { console.log('this test must be run with --expose-gc') process.exit(1) } function setTimeoutWithFix(callback, ms, ...args) { const wrappedCallback = function () { try { return callback.apply(this, args) } finally { clearTimeout(timeout) } } const timeout = setTimeout(wrappedCallback, ms) return timeout } const didFinalize = {} const registry = new FinalizationRegistry((id) => { didFinalize[id] = true }) { const id = 'node setTimeout'.padEnd(26, ' ') const timeout = setTimeout(() => {}, 0) registry.register(timeout, id) didFinalize[id] = false } { const id = 'node setTimeout as number'.padEnd(26, ' ') const timeout = setTimeout(() => {}, 0) timeout[Symbol.toPrimitive]() registry.register(timeout, id) didFinalize[id] = false } { const id = 'fixed setTimeout'.padEnd(26, ' ') const timeout = setTimeoutWithFix(() => {}, 0) registry.register(timeout, id) didFinalize[id] = false } { const id = 'fixed setTimeout as number'.padEnd(26, ' ') const timeout = setTimeoutWithFix(() => {}, 0) timeout[Symbol.toPrimitive]() registry.register(timeout, id) didFinalize[id] = false } // wait for the timeouts to run void setTimeout(() => { gc() // trigger garbage collection void registry // ...but make sure we keep the registry alive // wait a task so that finalization callbacks can run setTimeout(() => console.log('did the Timeout get released after GC?', didFinalize) ) }, 10) ``` To run it, Install the required node versions: ```bash for ver in v20.15.0 v20.16.0 v22.3.0 v22.4.0 v23.0.0; do ( nvm install "$ver" ); done ``` And run the test: ```bash for ver in v20.15.0 v20.16.0 v22.3.0 v22.4.0 v23.0.0; do ( echo '-------------------' nvm use "$ver" && node --expose-gc setTimeout-test.js echo ); done ``` The output on my machine is as follows. Note that the `node setTimeout as number` case comes up as false on the older versions (because it leaks and doesn't get finalized), but `fixed setTimeout as number` (which calls `clearTimeout`) gets released fine, which is exactly what we want. ```terminal ------------------- Now using node v20.15.0 (npm v10.7.0) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': false, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v20.16.0 (npm v10.8.1) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': true, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v22.3.0 (npm v10.8.1) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': false, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v22.4.0 (npm v10.8.1) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': true, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v23.0.0 (npm v10.9.0) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': true, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ``` </details>
ztanner pushed a commit to vercel/next.js that referenced this pull request
Feb 10, 2025Potential fix for a leak reported in #74855 on older node versions (see [comment](#74855 (comment))). ### Background When running middleware (or other edge functions) in `next start`, we wrap them in an edge runtime sandbox. this includes polyfills of `setTimeout` and `setInterval` which return `number` instead of `NodeJS.Timeout`. Unfortunately, on some older node versions, converting a `NodeJS.Timeout` to a number will cause that timeout to leak: nodejs/node#53335 The leaked timeout will also hold onto the callback, thus also leaking anything that was closed over (which can be a lot of things!) ### Solution Ideally, users just upgrade to a Node version that includes the fix: - [node v20.16.0](nodejs/node#53945) - [node v22.4.0](nodejs/node#53583) - node v23.0.0 But we're currently still supporting node 18, so we can't necessarily rely on that. Luckily, as noted in the description of the nodejs issue, calling `clearTimeout` seems to unleak the timeout, so we can just do that after the callback runs! ### Unrelated stuff I did While i was at it, I also fixed a (very niche) discrepancy from how `setTimeout` and `setInterval` behave on the web. when running the callback, node sets `this` to the Timeout instance: ```js > void setTimeout(function () {console.log('this in setTimeout:', this) } ) undefined > this in setTimeout: Timeout { ... } ``` but on the web, `this` is always set to `globalThis`. Our wrapper now correctly does this. ### Testing <details> <summary>Collapsed because it's long</summary> Verifying this is kinda tricky, so bear with me... Here's a script that can verify whether calling `clearTimeout` fixes the leak by using a FinalizationRegistry and triggering GC to observe whether memory leaked or not. `setTimeoutWithFix` is a simplified version of `webSetTimeoutPolyfill` from the PR. ```js // setTimeout-test.js if (typeof gc !== 'function') { console.log('this test must be run with --expose-gc') process.exit(1) } function setTimeoutWithFix(callback, ms, ...args) { const wrappedCallback = function () { try { return callback.apply(this, args) } finally { clearTimeout(timeout) } } const timeout = setTimeout(wrappedCallback, ms) return timeout } const didFinalize = {} const registry = new FinalizationRegistry((id) => { didFinalize[id] = true }) { const id = 'node setTimeout'.padEnd(26, ' ') const timeout = setTimeout(() => {}, 0) registry.register(timeout, id) didFinalize[id] = false } { const id = 'node setTimeout as number'.padEnd(26, ' ') const timeout = setTimeout(() => {}, 0) timeout[Symbol.toPrimitive]() registry.register(timeout, id) didFinalize[id] = false } { const id = 'fixed setTimeout'.padEnd(26, ' ') const timeout = setTimeoutWithFix(() => {}, 0) registry.register(timeout, id) didFinalize[id] = false } { const id = 'fixed setTimeout as number'.padEnd(26, ' ') const timeout = setTimeoutWithFix(() => {}, 0) timeout[Symbol.toPrimitive]() registry.register(timeout, id) didFinalize[id] = false } // wait for the timeouts to run void setTimeout(() => { gc() // trigger garbage collection void registry // ...but make sure we keep the registry alive // wait a task so that finalization callbacks can run setTimeout(() => console.log('did the Timeout get released after GC?', didFinalize) ) }, 10) ``` To run it, Install the required node versions: ```bash for ver in v20.15.0 v20.16.0 v22.3.0 v22.4.0 v23.0.0; do ( nvm install "$ver" ); done ``` And run the test: ```bash for ver in v20.15.0 v20.16.0 v22.3.0 v22.4.0 v23.0.0; do ( echo '-------------------' nvm use "$ver" && node --expose-gc setTimeout-test.js echo ); done ``` The output on my machine is as follows. Note that the `node setTimeout as number` case comes up as false on the older versions (because it leaks and doesn't get finalized), but `fixed setTimeout as number` (which calls `clearTimeout`) gets released fine, which is exactly what we want. ```terminal ------------------- Now using node v20.15.0 (npm v10.7.0) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': false, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v20.16.0 (npm v10.8.1) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': true, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v22.3.0 (npm v10.8.1) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': false, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v22.4.0 (npm v10.8.1) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': true, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v23.0.0 (npm v10.9.0) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': true, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ``` </details>
ztanner pushed a commit to vercel/next.js that referenced this pull request
Feb 10, 2025Potential fix for a leak reported in #74855 on older node versions (see [comment](#74855 (comment))). ### Background When running middleware (or other edge functions) in `next start`, we wrap them in an edge runtime sandbox. this includes polyfills of `setTimeout` and `setInterval` which return `number` instead of `NodeJS.Timeout`. Unfortunately, on some older node versions, converting a `NodeJS.Timeout` to a number will cause that timeout to leak: nodejs/node#53335 The leaked timeout will also hold onto the callback, thus also leaking anything that was closed over (which can be a lot of things!) ### Solution Ideally, users just upgrade to a Node version that includes the fix: - [node v20.16.0](nodejs/node#53945) - [node v22.4.0](nodejs/node#53583) - node v23.0.0 But we're currently still supporting node 18, so we can't necessarily rely on that. Luckily, as noted in the description of the nodejs issue, calling `clearTimeout` seems to unleak the timeout, so we can just do that after the callback runs! ### Unrelated stuff I did While i was at it, I also fixed a (very niche) discrepancy from how `setTimeout` and `setInterval` behave on the web. when running the callback, node sets `this` to the Timeout instance: ```js > void setTimeout(function () {console.log('this in setTimeout:', this) } ) undefined > this in setTimeout: Timeout { ... } ``` but on the web, `this` is always set to `globalThis`. Our wrapper now correctly does this. ### Testing <details> <summary>Collapsed because it's long</summary> Verifying this is kinda tricky, so bear with me... Here's a script that can verify whether calling `clearTimeout` fixes the leak by using a FinalizationRegistry and triggering GC to observe whether memory leaked or not. `setTimeoutWithFix` is a simplified version of `webSetTimeoutPolyfill` from the PR. ```js // setTimeout-test.js if (typeof gc !== 'function') { console.log('this test must be run with --expose-gc') process.exit(1) } function setTimeoutWithFix(callback, ms, ...args) { const wrappedCallback = function () { try { return callback.apply(this, args) } finally { clearTimeout(timeout) } } const timeout = setTimeout(wrappedCallback, ms) return timeout } const didFinalize = {} const registry = new FinalizationRegistry((id) => { didFinalize[id] = true }) { const id = 'node setTimeout'.padEnd(26, ' ') const timeout = setTimeout(() => {}, 0) registry.register(timeout, id) didFinalize[id] = false } { const id = 'node setTimeout as number'.padEnd(26, ' ') const timeout = setTimeout(() => {}, 0) timeout[Symbol.toPrimitive]() registry.register(timeout, id) didFinalize[id] = false } { const id = 'fixed setTimeout'.padEnd(26, ' ') const timeout = setTimeoutWithFix(() => {}, 0) registry.register(timeout, id) didFinalize[id] = false } { const id = 'fixed setTimeout as number'.padEnd(26, ' ') const timeout = setTimeoutWithFix(() => {}, 0) timeout[Symbol.toPrimitive]() registry.register(timeout, id) didFinalize[id] = false } // wait for the timeouts to run void setTimeout(() => { gc() // trigger garbage collection void registry // ...but make sure we keep the registry alive // wait a task so that finalization callbacks can run setTimeout(() => console.log('did the Timeout get released after GC?', didFinalize) ) }, 10) ``` To run it, Install the required node versions: ```bash for ver in v20.15.0 v20.16.0 v22.3.0 v22.4.0 v23.0.0; do ( nvm install "$ver" ); done ``` And run the test: ```bash for ver in v20.15.0 v20.16.0 v22.3.0 v22.4.0 v23.0.0; do ( echo '-------------------' nvm use "$ver" && node --expose-gc setTimeout-test.js echo ); done ``` The output on my machine is as follows. Note that the `node setTimeout as number` case comes up as false on the older versions (because it leaks and doesn't get finalized), but `fixed setTimeout as number` (which calls `clearTimeout`) gets released fine, which is exactly what we want. ```terminal ------------------- Now using node v20.15.0 (npm v10.7.0) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': false, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v20.16.0 (npm v10.8.1) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': true, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v22.3.0 (npm v10.8.1) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': false, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v22.4.0 (npm v10.8.1) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': true, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ------------------- Now using node v23.0.0 (npm v10.9.0) did the Timeout get released after GC? { 'node setTimeout ': true, 'node setTimeout as number ': true, 'fixed setTimeout ': true, 'fixed setTimeout as number': true } ``` </details>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters