From 0c69ec12a9fe4588f27f3e78234c5bbe6d5c709d Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Sun, 9 Jul 2017 13:50:59 +0200 Subject: [PATCH] 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 Reviewed-By: Trevor Norris Reviewed-By: Anna Henningsen --- lib/async_hooks.js | 32 +++++++++++++++--------- test/async-hooks/test-disable-in-init.js | 23 +++++++++++++++++ test/async-hooks/test-enable-in-init.js | 22 ++++++++++++++++ 3 files changed, 65 insertions(+), 12 deletions(-) create mode 100644 test/async-hooks/test-disable-in-init.js create mode 100644 test/async-hooks/test-enable-in-init.js diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 42d04f063081c2..4056fe1635f7c8 100644 --- a/lib/async_hooks.js +++ b/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 -// sure active_hooks_array isn't altered in mid execution if another hook is -// added or removed. -var processing_hook = false; +// Use a counter to track whether a hook callback is currently being processed. +// Used to make sure active_hooks_array isn't altered in mid execution if +// another hook is added or removed. A counter is used to track nested calls. +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. - if (tmp_active_hooks_array !== null) { + // * `tmp_active_hooks_array` is null if no hooks were added/removed while + // 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(); } } diff --git a/test/async-hooks/test-disable-in-init.js b/test/async-hooks/test-disable-in-init.js new file mode 100644 index 00000000000000..21588bf504651f --- /dev/null +++ b/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()); diff --git a/test/async-hooks/test-enable-in-init.js b/test/async-hooks/test-enable-in-init.js new file mode 100644 index 00000000000000..e4be9f9f87e3eb --- /dev/null +++ b/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());