timers: do not retain a reference to the async store after firing

After firing timers, clean up the async context references held by timer
resources so AsyncLocalStorage stores can be collected. Also clear callbacks
and arguments when timeouts or immediates are explicitly cleared.

This covers timeouts, intervals, and immediates with and without
AsyncContextFrame.

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

Author: mcollina

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,

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;
@@ -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,

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