From 029ea16a24f1c762529b3dce8d14bb2d709c4373 Mon Sep 17 00:00:00 2001
From: Stephen Belanger <admin@stephenbelanger.com>
Date: Fri, 23 Oct 2020 11:07:39 -0700
Subject: [PATCH] async_hooks: fix leak in AsyncLocalStorage exit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If exit is called and then run or enterWith are called within the
exit function, the als instace should not be added to the storageList
additional times. The correct behaviour is to remove the instance
from the storageList before executing the exit handler and then to
restore it after.

PR-URL: https://github.com/nodejs/node/pull/35779
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 lib/async_hooks.js                            | 18 +++++++------
 ...-async-local-storage-exit-does-not-leak.js | 25 +++++++++++++++++++
 2 files changed, 36 insertions(+), 7 deletions(-)
 create mode 100644 test/parallel/test-async-local-storage-exit-does-not-leak.js

diff --git a/lib/async_hooks.js b/lib/async_hooks.js
index 7dd888b61f79f9..b6865b6f1cd03e 100644
--- a/lib/async_hooks.js
+++ b/lib/async_hooks.js
@@ -271,6 +271,14 @@ class AsyncLocalStorage {
     }
   }
 
+  _enable() {
+    if (!this.enabled) {
+      this.enabled = true;
+      storageList.push(this);
+      storageHook.enable();
+    }
+  }
+
   // Propagate the context from a parent resource to a child one
   _propagate(resource, triggerResource) {
     const store = triggerResource[this.kResourceStore];
@@ -280,11 +288,7 @@ class AsyncLocalStorage {
   }
 
   enterWith(store) {
-    if (!this.enabled) {
-      this.enabled = true;
-      storageList.push(this);
-      storageHook.enable();
-    }
+    this._enable();
     const resource = executionAsyncResource();
     resource[this.kResourceStore] = store;
   }
@@ -308,11 +312,11 @@ class AsyncLocalStorage {
     if (!this.enabled) {
       return callback(...args);
     }
-    this.enabled = false;
+    this.disable();
     try {
       return callback(...args);
     } finally {
-      this.enabled = true;
+      this._enable();
     }
   }
 
diff --git a/test/parallel/test-async-local-storage-exit-does-not-leak.js b/test/parallel/test-async-local-storage-exit-does-not-leak.js
new file mode 100644
index 00000000000000..636d80f788b7fb
--- /dev/null
+++ b/test/parallel/test-async-local-storage-exit-does-not-leak.js
@@ -0,0 +1,25 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { AsyncLocalStorage } = require('async_hooks');
+
+const als = new AsyncLocalStorage();
+
+// Make sure _propagate function exists.
+assert.ok(typeof als._propagate === 'function');
+
+// The als instance should be getting removed from the storageList in
+// lib/async_hooks.js when exit(...) is called, therefore when the nested runs
+// are called there should be no copy of the als in the storageList to run the
+// _propagate method on.
+als._propagate = common.mustNotCall('_propagate() should not be called');
+
+const done = common.mustCall();
+
+function run(count) {
+  if (count === 0) return done();
+  als.run({}, () => {
+    als.exit(run, --count);
+  });
+}
+run(100);