async_hooks: fix nested hooks mutation

In some cases restoreTmpHooks is called too early, this causes
active_hooks_array to change during execution of the init hooks.

PR-URL: https://github.com/nodejs/node/pull/14143
Ref: https://github.com/nodejs/node/pull/14054#issuecomment-313915193
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

lib/async_hooks.js

@@ -26,10 +26,10 @@ const { pushAsyncIds, popAsyncIds } = async_wrap;
 // Using var instead of (preferably const) in order to assign
 // tmp_active_hooks_array if a hook is enabled/disabled during hook execution.
 var active_hooks_array = [];
-// Track whether a hook callback is currently being processed. Used to make
+// Use a counter to track whether a hook callback is currently being processed.
-// sure active_hooks_array isn't altered in mid execution if another hook is
+// Used to make sure active_hooks_array isn't altered in mid execution if
-// added or removed.
+// another hook is added or removed. A counter is used to track nested calls.
-var processing_hook = false;
+var processing_hook = 0;
 // Use to temporarily store and updated active_hooks_array if the user enables
 // or disables a hook while hooks are being processed.
 var tmp_active_hooks_array = null;
@@ -151,7 +151,7 @@ class AsyncHook {
 
 
 function getHookArrays() {
-  if (!processing_hook)
+  if (processing_hook === 0)
     return [active_hooks_array, async_hook_fields];
   // If this hook is being enabled while in the middle of processing the array
   // of currently active hooks then duplicate the current set of active hooks
@@ -342,7 +342,7 @@ function emitHookFactory(symbol, name) {
   // before this is called.
   // eslint-disable-next-line func-style
   const fn = function(asyncId) {
-    processing_hook = true;
+    processing_hook += 1;
     // Use a single try/catch for all hook to avoid setting up one per
     // iteration.
     try {
@@ -353,10 +353,11 @@ function emitHookFactory(symbol, name) {
       }
     } catch (e) {
       fatalError(e);
+    } finally {
+      processing_hook -= 1;
     }
-    processing_hook = false;
 
-    if (tmp_active_hooks_array !== null) {
+    if (processing_hook === 0 && tmp_active_hooks_array !== null) {
       restoreTmpHooks();
     }
   };
@@ -422,7 +423,7 @@ function emitDestroyS(asyncId) {
 // slim chance of the application remaining stable after handling one of these
 // exceptions.
 function init(asyncId, type, triggerAsyncId, resource) {
-  processing_hook = true;
+  processing_hook += 1;
   // Use a single try/catch for all hook to avoid setting up one per iteration.
   try {
     for (var i = 0; i < active_hooks_array.length; i++) {
@@ -435,11 +436,18 @@ function init(asyncId, type, triggerAsyncId, resource) {
     }
   } catch (e) {
     fatalError(e);
+  } finally {
+    processing_hook -= 1;
   }
-  processing_hook = false;
 
-  // Isn't null if hooks were added/removed while the hooks were running.
+  // * `tmp_active_hooks_array` is null if no hooks were added/removed while
-  if (tmp_active_hooks_array !== null) {
+  //   the hooks were running. In that case no restoration is needed.
+  // * In the case where another hook was added/removed while the hooks were
+  //   running and a handle was created causing the `init` hooks to fire again,
+  //   then `restoreTmpHooks` should not be called for the nested `hooks`.
+  //   Otherwise `active_hooks_array` can change during execution of the
+  //   `hooks`.
+  if (processing_hook === 0 && tmp_active_hooks_array !== null) {
     restoreTmpHooks();
   }
 }

test/async-hooks/test-disable-in-init.js

@@ -0,0 +1,23 @@
+'use strict';
+
+const common = require('../common');
+const async_hooks = require('async_hooks');
+const fs = require('fs');
+
+let nestedCall = false;
+
+async_hooks.createHook({
+  init: common.mustCall(function(id, type) {
+    nestedHook.disable();
+    if (!nestedCall) {
+      nestedCall = true;
+      fs.access(__filename, common.mustCall());
+    }
+  }, 2)
+}).enable();
+
+const nestedHook = async_hooks.createHook({
+  init: common.mustCall(2)
+}).enable();
+
+fs.access(__filename, common.mustCall());

test/async-hooks/test-enable-in-init.js

@@ -0,0 +1,22 @@
+'use strict';
+
+const common = require('../common');
+const async_hooks = require('async_hooks');
+const fs = require('fs');
+
+const nestedHook = async_hooks.createHook({
+  init: common.mustNotCall()
+});
+let nestedCall = false;
+
+async_hooks.createHook({
+  init: common.mustCall(function(id, type) {
+    nestedHook.enable();
+    if (!nestedCall) {
+      nestedCall = true;
+      fs.access(__filename, common.mustCall());
+    }
+  }, 2)
+}).enable();
+
+fs.access(__filename, common.mustCall());