From b83e431ba8a91452b2713731b4ab23db9fd95b3c Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sun, 31 May 2026 18:14:57 +0000 Subject: [PATCH] timers: do not retain a reference to the async store after firing After firing timers, we can clean them up by iterating over all active stores and setting the relevant symbols to undefined. This also covers immediates and intervals. Refs: https://github.com/nodejs/node/issues/53408 PR-URL: https://github.com/nodejs/node/pull/53443 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 | 86 ++++++++++++-- lib/timers.js | 12 +- test/parallel/test-timers-async-store-leak.js | 109 ++++++++++++++++++ 5 files changed, 219 insertions(+), 22 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..5e7b1159cc45a0 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,36 @@ const AsyncContextFrame = require('internal/async_context_frame'); const async_context_frame = Symbol('kAsyncContextFrame'); +function removeStoresFromResource(resource) { + if (!AsyncContextFrame.enabled && + 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 +534,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 +621,8 @@ function getTimerCallbacks(runNextTicks) { if (!timer._destroyed) { timer._destroyed = true; + cleanTimer(timer); + if (timer[kHasPrimitive]) delete knownTimersById[asyncId]; @@ -599,26 +645,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); + } } } @@ -703,6 +765,8 @@ module.exports = { kRefed, kHasPrimitive, initAsyncResource, + cleanImmediate, + cleanTimer, setUnrefTimeout, getTimerDuration, immediateQueue, diff --git a/lib/timers.js b/lib/timers.js index f6a2f74f5ec2c7..a218929e61e40d 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -38,6 +38,8 @@ const { async_id_symbol, Timeout, Immediate, + cleanImmediate, + cleanTimer, 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; } @@ -238,7 +246,7 @@ function clearImmediate(immediate) { emitDestroy(immediate[async_id_symbol]); - immediate._onImmediate = null; + 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..154aa966a5b45a --- /dev/null +++ b/test/parallel/test-timers-async-store-leak.js @@ -0,0 +1,109 @@ +// Flags: --expose-internals --no-async-context-frame +'use strict'; + +const common = require('../common'); +const { AsyncLocalStorage } = require('async_hooks'); +const assert = require('assert'); +const { + symbols: { + async_local_storage_context_symbol, + }, +} = require('internal/async_hooks'); + +function getStore(resource, asyncLocalStorage) { + return resource[async_local_storage_context_symbol]?.[asyncLocalStorage.kResourceStore]; +} + +function assertNoStore(resource) { + 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); + assert.strictEqual(getStore(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); + assert.strictEqual(getStore(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); + clearInterval(interval); + assert.strictEqual(asyncLocalStorage.getStore(), store); + setImmediate(common.mustCall(() => { + assertNoStore(interval); + assert.strictEqual(interval._onTimeout, undefined); + assert.strictEqual(interval._timerArgs, undefined); + })); + }), 1, arg); + assert.strictEqual(getStore(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); + assert.strictEqual(getStore(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); + assert.strictEqual(getStore(immediate, asyncLocalStorage), store); + clearImmediate(immediate); + assertNoStore(immediate); + assert.strictEqual(immediate._onImmediate, undefined); + assert.strictEqual(immediate._argv, undefined); + })); +}