You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
fix: work around setTimeout memory leak, improve wrappers (#75727)
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>
0 commit comments