v20.16.0 proposal by marco-ippolito · Pull Request #53945 · nodejs/node

and others added 30 commits

July 19, 2024 02:59
Refs: #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>
PR-URL: #51168
Backport-PR-URL: #52773
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #52855
Refs: #52273
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@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>
PR-URL: #45131
Fixes: #42286
Reviewed-By: Antoine du Hamel <duhamelantoine1995@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>
PR-URL: #53000
Fixes: #52775
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
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>
Fixes: #49902
PR-URL: #53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Fixes: #52929
PR-URL: #53118
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
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>
PR-URL: #52905
Fixes: #51227
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>

marco-ippolito added a commit that referenced this pull request

Jul 21, 2024
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

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, 2024
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 to nodejs/nodejs.org that referenced this pull request

Jul 24, 2024

marco-ippolito added a commit to nodejs/nodejs.org that referenced this pull request

Jul 24, 2024

This was referenced

Feb 6, 2025

lubieowoce added a commit to vercel/next.js that referenced this pull request

Feb 6, 2025
Potential 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, 2025
Potential 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, 2025
Potential 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>