From e7fd2928bb5b8eaf6e4a257cf011af4a2135686a Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 7 May 2019 00:33:59 +0200
Subject: [PATCH] async_hooks: only disable promise hook if wanted

The promise hook has been disabled asynchronously in order to solve
issues when an async hook is disabled during a microtask.

This means that after scheduling the disable-promise-hook call,
attempts to enable it synchronously will later be unintentionally
overridden.

In order to solve this, make sure that the promise hooks are still
no longer desired at the time at which we would disable them.

Fixes: https://github.com/nodejs/node/issues/27585
---
 lib/internal/async_hooks.js                     | 17 +++++++++++++++--
 src/async_wrap.cc                               | 13 ++++---------
 .../test-async-hooks-enable-disable-enable.js   | 17 +++++++++++++++++
 3 files changed, 36 insertions(+), 11 deletions(-)
 create mode 100644 test/parallel/test-async-hooks-enable-disable-enable.js

diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js
index 64f5cb2462f6ba..bf803885cacfa3 100644
--- a/lib/internal/async_hooks.js
+++ b/lib/internal/async_hooks.js
@@ -69,6 +69,7 @@ const active_hooks = {
 };
 
 const { registerDestroyHook } = async_wrap;
+const { enqueueMicrotask } = internalBinding('task_queue');
 
 // Each constant tracks how many callbacks there are for any given step of
 // async execution. These are tracked so if the user didn't include callbacks
@@ -231,14 +232,26 @@ function restoreActiveHooks() {
 }
 
 
+let wantPromiseHook = false;
 function enableHooks() {
-  enablePromiseHook();
   async_hook_fields[kCheck] += 1;
+
+  wantPromiseHook = true;
+  enablePromiseHook();
 }
 
 function disableHooks() {
-  disablePromiseHook();
   async_hook_fields[kCheck] -= 1;
+
+  wantPromiseHook = false;
+  // Delay the call to `disablePromiseHook()` because we might currently be
+  // between the `before` and `after` calls of a Promise.
+  enqueueMicrotask(disablePromiseHookIfNecessary);
+}
+
+function disablePromiseHookIfNecessary() {
+  if (!wantPromiseHook)
+    disablePromiseHook();
 }
 
 // Internal Embedder API //
diff --git a/src/async_wrap.cc b/src/async_wrap.cc
index 0c84b06fdde346..24ef24ae6c8185 100644
--- a/src/async_wrap.cc
+++ b/src/async_wrap.cc
@@ -331,15 +331,10 @@ static void EnablePromiseHook(const FunctionCallbackInfo<Value>& args) {
 static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = args.GetIsolate();
 
-  // Delay the call to `RemovePromiseHook` because we might currently be
-  // between the `before` and `after` calls of a Promise.
-  isolate->EnqueueMicrotask([](void* data) {
-    // The per-Isolate API provides no way of knowing whether there are multiple
-    // users of the PromiseHook. That hopefully goes away when V8 introduces
-    // a per-context API.
-    Isolate* isolate = static_cast<Isolate*>(data);
-    isolate->SetPromiseHook(nullptr);
-  }, static_cast<void*>(isolate));
+  // The per-Isolate API provides no way of knowing whether there are multiple
+  // users of the PromiseHook. That hopefully goes away when V8 introduces
+  // a per-context API.
+  isolate->SetPromiseHook(nullptr);
 }
 
 
diff --git a/test/parallel/test-async-hooks-enable-disable-enable.js b/test/parallel/test-async-hooks-enable-disable-enable.js
new file mode 100644
index 00000000000000..640815e14c8c9d
--- /dev/null
+++ b/test/parallel/test-async-hooks-enable-disable-enable.js
@@ -0,0 +1,17 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+
+// Regression test for https://github.com/nodejs/node/issues/27585.
+
+async_hooks.createHook({ init: () => {} }).enable().disable().enable();
+async_hooks.createHook({ init: () => {} }).enable();
+
+async function main() {
+  const initialAsyncId = async_hooks.executionAsyncId();
+  await 0;
+  assert.notStrictEqual(async_hooks.executionAsyncId(), initialAsyncId);
+}
+
+main().then(common.mustCall());