timers: fix review comments

- fix removeStoresFromResource for ACF and new ALS impl
- add clearStoreFromResource to async_hooks
- add cleanup to clearTimeout, clearImmediate, processImmediate, listOnTimeout
- export cleanTimer/cleanImmediate from internal/timers
- update test for ACF and non-ACF variants, add GC test

Signed-off-by: Matteo Collina <hello@matteocollina.com>

Author: mcollina

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;

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,

lib/timers.js

@@ -38,6 +38,8 @@ const {
   async_id_symbol,
   Timeout,
   Immediate,
+  cleanTimer,
+  cleanImmediate,
   decRefCount,
   immediateInfoFields: {
     kCount,
@@ -136,13 +138,17 @@ ObjectDefineProperty(setTimeout, customPromisify, {
 function clearTimeout(timer) {
   if (timer?._onTimeout) {
     timer._onTimeout = null;
+    timer._timerArgs = undefined;
+    cleanTimer(timer);
     unenroll(timer);
     return;
   }
   if (typeof timer === 'number' || typeof timer === 'string') {
     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);
 }

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