diff --git a/lib/internal/async_local_storage/async_hooks.js b/lib/internal/async_local_storage/async_hooks.js index dab76538845a95..bf319154c364a4 100644 --- a/lib/internal/async_local_storage/async_hooks.js +++ b/lib/internal/async_local_storage/async_hooks.js @@ -23,6 +23,13 @@ const RunScope = require('internal/async_local_storage/run_scope'); const { kEmptyObject } = require('internal/util'); const storageList = []; + +function clearStoreFromResource(resource) { + for (let i = 0; i < storageList.length; ++i) { + resource[storageList[i].kResourceStore] = undefined; + } +} + const storageHook = createHook({ init(asyncId, type, triggerAsyncId, resource) { const currentResource = executionAsyncResource(); @@ -152,3 +159,4 @@ class AsyncLocalStorage { } module.exports = AsyncLocalStorage; +module.exports.clearStoreFromResource = clearStoreFromResource; diff --git a/lib/internal/timers.js b/lib/internal/timers.js index 9c7366d6ca772f..ad15e9881d4a23 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -125,6 +125,38 @@ 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 { + require('internal/async_local_storage/async_hooks').clearStoreFromResource(resource); + } +} + +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 +530,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); @@ -599,26 +639,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 +754,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..f3e61e9d1b24b4 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -38,6 +38,8 @@ const { async_id_symbol, Timeout, Immediate, + cleanTimer, + cleanImmediate, decRefCount, immediateInfoFields: { kCount, @@ -136,6 +138,8 @@ ObjectDefineProperty(setTimeout, customPromisify, { function clearTimeout(timer) { if (timer?._onTimeout) { timer._onTimeout = null; + timer._timerArgs = undefined; + cleanTimer(timer); unenroll(timer); return; } @@ -143,6 +147,8 @@ function clearTimeout(timer) { const timerInstance = knownTimersById[timer]; if (timerInstance !== undefined) { timerInstance._onTimeout = null; + timerInstance._timerArgs = undefined; + cleanTimer(timerInstance); unenroll(timerInstance); } } @@ -239,6 +245,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..0a5ed32be41457 --- /dev/null +++ b/test/parallel/test-timers-async-store-leak.js @@ -0,0 +1,128 @@ +// Flags: --expose-internals --expose-gc +'use strict'; + +const common = require('../common'); +const { AsyncLocalStorage } = require('async_hooks'); +const assert = require('assert'); + +const AsyncContextFrame = require('internal/async_context_frame'); +const async_context_frame = require('internal/timers').async_context_frame; + +// 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 assertNoStore(resource, als) { + if (isACFEnabled) { + assert.strictEqual(resource[async_context_frame], undefined); + } else { + assert.strictEqual(resource[als.kResourceStore], 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, asyncLocalStorage); + })); + }), 1, arg); + assert.strictEqual(asyncLocalStorage.getStore(), 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(asyncLocalStorage.getStore(), store); + clearTimeout(timeout); + assertNoStore(timeout, asyncLocalStorage); + 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); + setImmediate(common.mustCall(() => { + assertNoStore(interval, asyncLocalStorage); + assert.strictEqual(interval._onTimeout, undefined); + assert.strictEqual(interval._timerArgs, undefined); + })); + }), 1, arg); + assert.strictEqual(asyncLocalStorage.getStore(), 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, asyncLocalStorage); + assert.strictEqual(immediate._onImmediate, undefined); + assert.strictEqual(immediate._argv, undefined); + })); + }), arg); + assert.strictEqual(asyncLocalStorage.getStore(), 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(asyncLocalStorage.getStore(), store); + clearImmediate(immediate); + assertNoStore(immediate, asyncLocalStorage); + 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); + })); +}