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); + })); +}