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.

Fixes #53408

Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: https://github.com/nodejs/node/pull/53443
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

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');
@@ -584,7 +585,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,

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

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;
@@ -528,14 +567,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);
 
@@ -607,6 +654,8 @@ function getTimerCallbacks(runNextTicks) {
         if (!timer._destroyed) {
           timer._destroyed = true;
 
+          cleanTimer(timer);
+
           if (timer[kHasPrimitive])
             delete knownTimersById[asyncId];
 
@@ -629,26 +678,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;
+        } 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);
 
-          if (timer[kHasPrimitive])
-            delete knownTimersById[asyncId];
+            timer._destroyed = true;
 
-          if (timer[kRefed])
-            timeoutInfo[0]--;
+            if (timer[kHasPrimitive])
+              delete knownTimersById[asyncId];
 
-          emitDestroy(asyncId);
+            if (timer[kRefed])
+              timeoutInfo[0]--;
+
+            emitDestroy(asyncId);
+          }
         }
       }
 
@@ -728,8 +793,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,

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;
 }
@@ -238,7 +246,7 @@ function clearImmediate(immediate) {
 
   emitDestroy(immediate[async_id_symbol]);
 
-  immediate._onImmediate = null;
+  cleanImmediate(immediate);
 
   immediateQueue.remove(immediate);
 }

test/parallel/test-timers-async-store-leak.js

@@ -0,0 +1,167 @@
+// 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') {
+  // Test with async-context-frame disabled (legacy ALS mode)
+  spawnSyncAndAssert(process.execPath, [
+    '--expose-internals',
+    '--expose-gc',
+    '--no-async-context-frame',
+    __filename,
+    'child',
+  ], {});
+
+  // Test with async-context-frame enabled (default)
+  spawnSyncAndAssert(process.execPath, [
+    '--expose-internals',
+    '--expose-gc',
+    __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);
+  }));
+}