Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: work around setTimeout memory leak, improve wrappers #75727

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

lubieowoce
Copy link
Member

@lubieowoce lubieowoce commented Feb 6, 2025

Potential fix for a leak reported in #74855 on older node versions (see 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:

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:

> 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

Collapsed because it's long

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.

// 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:

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:

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.

-------------------
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
}

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Feb 6, 2025
@ijjk
Copy link
Member

ijjk commented Feb 6, 2025

Tests Passed

@lubieowoce lubieowoce enabled auto-merge (squash) February 6, 2025 14:33
@lubieowoce lubieowoce merged commit ad56554 into canary Feb 6, 2025
131 of 132 checks passed
@lubieowoce lubieowoce deleted the lubieowoce/edge-sandbox-settimeout-leak branch February 6, 2025 15:52
ztanner pushed a commit 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 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 added a commit that referenced this pull request Feb 10, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Backports:
- #75727

Co-authored-by: Janka Uryga <lolzatu2@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants