From d05d56a1283b09351dc8852bd7c5cb3cd340f1db Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Thu, 6 Feb 2025 16:52:43 +0100 Subject: [PATCH] fix: work around setTimeout memory leak, improve wrappers (#75727) Potential fix for a leak reported in #74855 on older node versions (see [comment](https://github.com/vercel/next.js/issues/74855#issuecomment-2638334820)). ### 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: https://github.com/nodejs/node/issues/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](https://github.com/nodejs/node/pull/53945) - [node v22.4.0](https://github.com/nodejs/node/pull/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
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. ```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 } ```
--- .../server/web/sandbox/resource-managers.ts | 46 +++++++++++++++++-- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/packages/next/src/server/web/sandbox/resource-managers.ts b/packages/next/src/server/web/sandbox/resource-managers.ts index 29433fb81c0979..7a51095d75cf66 100644 --- a/packages/next/src/server/web/sandbox/resource-managers.ts +++ b/packages/next/src/server/web/sandbox/resource-managers.ts @@ -1,10 +1,10 @@ -abstract class ResourceManager { +abstract class ResourceManager { 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 @@ -27,7 +27,7 @@ class IntervalsManager extends ResourceManager< > { create(args: Parameters) { // TODO: use the edge runtime provided `setInterval` instead - return setInterval(...args)[Symbol.toPrimitive]() + return webSetIntervalPolyfill(...args) } destroy(interval: number) { @@ -41,7 +41,7 @@ class TimeoutsManager extends ResourceManager< > { create(args: Parameters) { // TODO: use the edge runtime provided `setTimeout` instead - return setTimeout(...args)[Symbol.toPrimitive]() + return webSetTimeoutPolyfill(...args) } destroy(timeout: number) { @@ -49,5 +49,41 @@ class TimeoutsManager extends ResourceManager< } } +function webSetIntervalPolyfill( + 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( + 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()