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