Skip to content

Commit

Permalink
fix: work around setTimeout memory leak, improve wrappers (#75727)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
lubieowoce authored and ztanner committed Feb 10, 2025
1 parent b010112 commit d6405f1
Showing 1 changed file with 41 additions and 5 deletions.
46 changes: 41 additions & 5 deletions packages/next/src/server/web/sandbox/resource-managers.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
abstract class ResourceManager<T, K> {
abstract class ResourceManager<T, Args> {
private resources: T[] = []

abstract create(resourceArgs: K): T
abstract create(resourceArgs: Args): T
abstract destroy(resource: T): void

add(resourceArgs: K) {
add(resourceArgs: Args) {
const resource = this.create(resourceArgs)
this.resources.push(resource)
return resource
Expand All @@ -27,7 +27,7 @@ class IntervalsManager extends ResourceManager<
> {
create(args: Parameters<typeof setInterval>) {
// TODO: use the edge runtime provided `setInterval` instead
return setInterval(...args)[Symbol.toPrimitive]()
return webSetIntervalPolyfill(...args)
}

destroy(interval: number) {
Expand All @@ -41,13 +41,49 @@ class TimeoutsManager extends ResourceManager<
> {
create(args: Parameters<typeof setTimeout>) {
// TODO: use the edge runtime provided `setTimeout` instead
return setTimeout(...args)[Symbol.toPrimitive]()
return webSetTimeoutPolyfill(...args)
}

destroy(timeout: number) {
clearTimeout(timeout)
}
}

function webSetIntervalPolyfill<TArgs extends any[]>(
callback: (...args: TArgs) => void,
ms?: number,
...args: TArgs
): number {
return setInterval(() => {
// node's `setInterval` sets `this` to the `Timeout` instance it returned,
// but web `setInterval` always sets `this` to `window`
// see: https://developer.mozilla.org/en-US/docs/Web/API/Window/setInterval#the_this_problem
return callback.apply(globalThis, args)
}, ms)[Symbol.toPrimitive]()
}

function webSetTimeoutPolyfill<TArgs extends any[]>(
callback: (...args: TArgs) => void,
ms?: number,
...args: TArgs
): number {
const wrappedCallback = () => {
try {
// node's `setTimeout` sets `this` to the `Timeout` instance it returned,
// but web `setTimeout` always sets `this` to `window`
// see: https://developer.mozilla.org/en-US/docs/Web/API/Window/setTimeout#the_this_problem
return callback.apply(globalThis, args)
} finally {
// On certain older node versions (<20.16.0, <22.4.0),
// a `setTimeout` whose Timeout was converted to a primitive will leak.
// See: https://github.com/nodejs/node/issues/53335
// We can work around this by explicitly calling `clearTimeout` after the callback runs.
clearTimeout(timeout)
}
}
const timeout = setTimeout(wrappedCallback, ms)
return timeout[Symbol.toPrimitive]()
}

export const intervalsManager = new IntervalsManager()
export const timeoutsManager = new TimeoutsManager()

0 comments on commit d6405f1

Please sign in to comment.