From c2a26733b3ae06ba7ad5944e970794b3c7d4c315 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sat, 20 Jun 2026 10:23:22 +0000 Subject: [PATCH] timers: do not retain a reference to the async store after firing After firing timers, clean up the async context references held by timer resources so AsyncLocalStorage stores can be collected. Also clear callbacks and arguments when timeouts or immediates are explicitly cleared. This covers timeouts, intervals, and immediates with and without AsyncContextFrame. Fixes: https://github.com/nodejs/node/issues/53408 Signed-off-by: Matteo Collina --- lib/internal/async_hooks.js | 4 +- .../async_local_storage/async_hooks.js | 30 +++- lib/internal/timers.js | 90 ++++++++-- lib/timers.js | 12 +- test/parallel/test-timers-async-store-leak.js | 158 ++++++++++++++++++ 5 files changed, 273 insertions(+), 21 deletions(-) create mode 100644 test/parallel/test-timers-async-store-leak.js diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index 71f6bc06414e7d..30c060d0a019e4 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -105,6 +105,7 @@ const before_symbol = Symbol('before'); const after_symbol = Symbol('after'); const destroy_symbol = Symbol('destroy'); const promise_resolve_symbol = Symbol('promiseResolve'); +const async_local_storage_context_symbol = Symbol('kAsyncLocalStorageContext'); const emitBeforeNative = emitHookFactory(before_symbol, 'emitBeforeNative'); const emitAfterNative = emitHookFactory(after_symbol, 'emitAfterNative'); const emitDestroyNative = emitHookFactory(destroy_symbol, 'emitDestroyNative'); @@ -595,7 +596,8 @@ module.exports = { symbols: { async_id_symbol, trigger_async_id_symbol, init_symbol, before_symbol, after_symbol, destroy_symbol, - promise_resolve_symbol, owner_symbol, + promise_resolve_symbol, async_local_storage_context_symbol, + owner_symbol, }, constants: { kInit, kBefore, kAfter, kDestroy, kTotals, kPromiseResolve, diff --git a/lib/internal/async_local_storage/async_hooks.js b/lib/internal/async_local_storage/async_hooks.js index dab76538845a95..2d39e1f18322a5 100644 --- a/lib/internal/async_local_storage/async_hooks.js +++ b/lib/internal/async_local_storage/async_hooks.js @@ -12,6 +12,11 @@ const { const { validateObject, } = require('internal/validators'); +const { + symbols: { + async_local_storage_context_symbol, + }, +} = require('internal/async_hooks'); const { AsyncResource, @@ -23,6 +28,11 @@ const RunScope = require('internal/async_local_storage/run_scope'); const { kEmptyObject } = require('internal/util'); const storageList = []; + +function getOrCreateResourceStore(resource) { + return resource[async_local_storage_context_symbol] ??= { __proto__: null }; +} + const storageHook = createHook({ init(asyncId, type, triggerAsyncId, resource) { const currentResource = executionAsyncResource(); @@ -91,16 +101,18 @@ class AsyncLocalStorage { // Propagate the context from a parent resource to a child one _propagate(resource, triggerResource, type) { - const store = triggerResource[this.kResourceStore]; + const store = triggerResource[async_local_storage_context_symbol]?.[this.kResourceStore]; if (this.enabled) { - resource[this.kResourceStore] = store; + const resourceStore = getOrCreateResourceStore(resource); + resourceStore[this.kResourceStore] = store; } } enterWith(store) { this._enable(); const resource = executionAsyncResource(); - resource[this.kResourceStore] = store; + const resourceStore = getOrCreateResourceStore(resource); + resourceStore[this.kResourceStore] = store; } run(store, callback, ...args) { @@ -112,14 +124,15 @@ class AsyncLocalStorage { this._enable(); const resource = executionAsyncResource(); - const oldStore = resource[this.kResourceStore]; + const resourceStore = getOrCreateResourceStore(resource); + const oldStore = resourceStore[this.kResourceStore]; - resource[this.kResourceStore] = store; + resourceStore[this.kResourceStore] = store; try { return ReflectApply(callback, null, args); } finally { - resource[this.kResourceStore] = oldStore; + resourceStore[this.kResourceStore] = oldStore; } } @@ -138,10 +151,11 @@ class AsyncLocalStorage { getStore() { if (this.enabled) { const resource = executionAsyncResource(); - if (!(this.kResourceStore in resource)) { + const resourceStore = resource[async_local_storage_context_symbol]; + if (resourceStore === undefined || !(this.kResourceStore in resourceStore)) { return this.#defaultValue; } - return resource[this.kResourceStore]; + return resourceStore[this.kResourceStore]; } return this.#defaultValue; } diff --git a/lib/internal/timers.js b/lib/internal/timers.js index 9c7366d6ca772f..03646e847b17fb 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -87,6 +87,9 @@ const { immediateInfo, timeoutInfo, } = binding; +const { + enqueueMicrotask, +} = internalBinding('task_queue'); const { getDefaultTriggerAsyncId, @@ -97,6 +100,9 @@ const { emitBefore, emitAfter, emitDestroy, + symbols: { + async_local_storage_context_symbol, + }, } = require('internal/async_hooks'); // Symbols for storing async id state. @@ -125,6 +131,39 @@ const AsyncContextFrame = require('internal/async_context_frame'); const async_context_frame = Symbol('kAsyncContextFrame'); +function removeStoresFromResource(resource) { + if (AsyncContextFrame.enabled) { + if (resource[async_context_frame] !== undefined) { + resource[async_context_frame] = undefined; + } + } else if (resource[async_local_storage_context_symbol] !== undefined) { + resource[async_local_storage_context_symbol] = undefined; + } +} + +function cleanTimer(timer) { + removeStoresFromResource(timer); + timer._onTimeout = undefined; + timer._timerArgs = undefined; +} + +function cleanImmediate(immediate) { + removeStoresFromResource(immediate); + immediate._onImmediate = undefined; + immediate._argv = undefined; +} + +function enqueueRemoveStoresFromResource(resource) { + enqueueMicrotask(() => removeStoresFromResource(resource)); +} + +function enqueueRemoveStoresIfNotReinserted(resource) { + enqueueMicrotask(() => { + if (!resource._idleNext && !resource._idlePrev) + removeStoresFromResource(resource); + }); +} + // *Must* match Environment::ImmediateInfo::Fields in src/env.h. const kCount = 0; const kRefCount = 1; @@ -498,14 +537,22 @@ function getTimerCallbacks(runNextTicks) { const asyncId = immediate[async_id_symbol]; emitBefore(asyncId, immediate[trigger_async_id_symbol], immediate); + let threw = true; try { const argv = immediate._argv; if (!argv) immediate._onImmediate(); else immediate._onImmediate(...argv); + threw = false; } finally { - immediate._onImmediate = null; + if (threw) { + immediate._onImmediate = undefined; + immediate._argv = undefined; + enqueueRemoveStoresFromResource(immediate); + } else { + cleanImmediate(immediate); + } emitDestroy(asyncId); @@ -577,6 +624,8 @@ function getTimerCallbacks(runNextTicks) { if (!timer._destroyed) { timer._destroyed = true; + cleanTimer(timer); + if (timer[kHasPrimitive]) delete knownTimersById[asyncId]; @@ -599,26 +648,42 @@ function getTimerCallbacks(runNextTicks) { start = binding.getLibuvNow(); } + let threw = true; try { const args = timer._timerArgs; if (args === undefined) timer._onTimeout(); else ReflectApply(timer._onTimeout, timer, args); + threw = false; } finally { if (timer._repeat && timer._idleTimeout !== -1) { timer._idleTimeout = timer._repeat; insert(timer, timer._idleTimeout, start); - } else if (!timer._idleNext && !timer._idlePrev && !timer._destroyed) { - timer._destroyed = true; - - if (timer[kHasPrimitive]) - delete knownTimersById[asyncId]; - - if (timer[kRefed]) - timeoutInfo[0]--; - - emitDestroy(asyncId); + } else if (!timer._idleNext && !timer._idlePrev) { + if (timer._destroyed) { + timer._onTimeout = undefined; + timer._timerArgs = undefined; + if (threw) + enqueueRemoveStoresIfNotReinserted(timer); + else + removeStoresFromResource(timer); + } else { + if (threw) + enqueueRemoveStoresIfNotReinserted(timer); + else + removeStoresFromResource(timer); + + timer._destroyed = true; + + if (timer[kHasPrimitive]) + delete knownTimersById[asyncId]; + + if (timer[kRefed]) + timeoutInfo[0]--; + + emitDestroy(asyncId); + } } } @@ -698,8 +763,11 @@ module.exports = { kTimeout: Symbol('timeout'), // For hiding Timeouts on other internals. async_id_symbol, trigger_async_id_symbol, + async_context_frame, Timeout, Immediate, + cleanImmediate, + cleanTimer, kRefed, kHasPrimitive, initAsyncResource, diff --git a/lib/timers.js b/lib/timers.js index f6a2f74f5ec2c7..cf5722f9189a20 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -38,6 +38,8 @@ const { async_id_symbol, Timeout, Immediate, + cleanTimer, + cleanImmediate, decRefCount, immediateInfoFields: { kCount, @@ -69,9 +71,12 @@ const { // Remove a timer. Cancels the timeout and resets the relevant timer properties. function unenroll(item) { - if (item._destroyed) + if (item._destroyed) { + cleanTimer(item); return; + } + const wasEnrolled = item._idleNext !== null || item._idlePrev !== null; item._destroyed = true; if (item[kHasPrimitive]) @@ -99,6 +104,9 @@ function unenroll(item) { decRefCount(); } + if (wasEnrolled) + cleanTimer(item); + // If active is called later, then we want to make sure not to insert again item._idleTimeout = -1; } @@ -239,6 +247,8 @@ function clearImmediate(immediate) { emitDestroy(immediate[async_id_symbol]); immediate._onImmediate = null; + immediate._argv = undefined; + cleanImmediate(immediate); immediateQueue.remove(immediate); } diff --git a/test/parallel/test-timers-async-store-leak.js b/test/parallel/test-timers-async-store-leak.js new file mode 100644 index 00000000000000..00574b97f4870c --- /dev/null +++ b/test/parallel/test-timers-async-store-leak.js @@ -0,0 +1,158 @@ +// Flags: --expose-internals --expose-gc +'use strict'; + +const common = require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const { AsyncLocalStorage } = require('async_hooks'); +const assert = require('assert'); + +if (process.argv[2] !== 'child') { + spawnSyncAndAssert(process.execPath, [ + '--expose-internals', + '--expose-gc', + '--no-async-context-frame', + __filename, + 'child', + ], {}); +} + +const AsyncContextFrame = require('internal/async_context_frame'); +const { + async_context_frame, +} = require('internal/timers'); +const { + symbols: { + async_local_storage_context_symbol, + }, +} = require('internal/async_hooks'); + +// When async-context-frame is enabled, stores are stored in the async context +// frame, not directly on the resource. The resource holds a reference to the +// frame via async_context_frame. +const isACFEnabled = AsyncContextFrame.enabled; + +function getStore(resource, als) { + if (isACFEnabled) { + return resource[async_context_frame]?.get(als); + } + return resource[async_local_storage_context_symbol]?.[als.kResourceStore]; +} + +function assertStore(resource, als, store) { + assert.strictEqual(getStore(resource, als), store); +} + +function assertNoStore(resource) { + if (isACFEnabled) { + assert.strictEqual(resource[async_context_frame], undefined); + } else { + assert.strictEqual(resource[async_local_storage_context_symbol], undefined); + } +} + +// Test that setTimeout does not retain a reference to the async store after +// firing. The callback and arguments must stay on the Timeout object so that +// refresh() can reactivate the timer. +{ + const asyncLocalStorage = new AsyncLocalStorage(); + const store = {}; + const arg = {}; + asyncLocalStorage.run(store, common.mustCall(() => { + const timeout = setTimeout(common.mustCall((received) => { + assert.strictEqual(received, arg); + setImmediate(common.mustCall(() => { + assertNoStore(timeout); + })); + }), 1, arg); + assertStore(timeout, asyncLocalStorage, store); + })); +} + +// Test that clearTimeout cleans up the store, callback, and arguments before +// firing. +{ + const asyncLocalStorage = new AsyncLocalStorage(); + const store = {}; + const arg = {}; + asyncLocalStorage.run(store, common.mustCall(() => { + const timeout = setTimeout(common.mustNotCall(), common.platformTimeout(1000), arg); + assertStore(timeout, asyncLocalStorage, store); + clearTimeout(timeout); + assertNoStore(timeout); + assert.strictEqual(timeout._onTimeout, undefined); + assert.strictEqual(timeout._timerArgs, undefined); + })); +} + +// Test that setInterval does not retain a reference to the async store, +// callback, or arguments after it is cleared. +{ + const asyncLocalStorage = new AsyncLocalStorage(); + const store = {}; + const arg = {}; + asyncLocalStorage.run(store, common.mustCall(() => { + const interval = setInterval(common.mustCall((received) => { + assert.strictEqual(received, arg); + assert.strictEqual(asyncLocalStorage.getStore(), store); + clearInterval(interval); + assert.strictEqual(asyncLocalStorage.getStore(), store); + setImmediate(common.mustCall(() => { + assertNoStore(interval); + assert.strictEqual(interval._onTimeout, undefined); + assert.strictEqual(interval._timerArgs, undefined); + })); + }), 1, arg); + assertStore(interval, asyncLocalStorage, store); + })); +} + +// Test that setImmediate does not retain a reference to the async store, +// callback, or arguments after firing. +{ + const asyncLocalStorage = new AsyncLocalStorage(); + const store = {}; + const arg = {}; + asyncLocalStorage.run(store, common.mustCall(() => { + const immediate = setImmediate(common.mustCall((received) => { + assert.strictEqual(received, arg); + setImmediate(common.mustCall(() => { + assertNoStore(immediate); + assert.strictEqual(immediate._onImmediate, undefined); + assert.strictEqual(immediate._argv, undefined); + })); + }), arg); + assertStore(immediate, asyncLocalStorage, store); + })); +} + +// Test that clearImmediate cleans up the store, callback, and arguments before +// firing. +{ + const asyncLocalStorage = new AsyncLocalStorage(); + const store = {}; + const arg = {}; + asyncLocalStorage.run(store, common.mustCall(() => { + const immediate = setImmediate(common.mustNotCall(), arg); + assertStore(immediate, asyncLocalStorage, store); + clearImmediate(immediate); + assertNoStore(immediate); + assert.strictEqual(immediate._onImmediate, undefined); + assert.strictEqual(immediate._argv, undefined); + })); +} + +// Test that the async store reference is cleaned up and can be GC'd. +{ + const asyncLocalStorage = new AsyncLocalStorage(); + let timeout; + const weakStore = new WeakRef({}); + asyncLocalStorage.run(weakStore.deref(), common.mustCall(() => { + timeout = setTimeout(common.mustNotCall(), common.platformTimeout(1000)); + assert.strictEqual(weakStore.deref() !== undefined, true); + clearTimeout(timeout); + })); + setImmediate(common.mustCall(() => { + global.gc(); + assert.strictEqual(weakStore.deref(), undefined); + })); +}