From 3e73ea874551d93fcf6b921b03385116ade92b43 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 2 Aug 2017 01:34:59 -0600 Subject: [PATCH] async_hooks: improve comments and function names * Reword several of the comments to be more descriptive. * Rename functions to better indicate what they are doing. * Change AsyncHooks::uid_fields_ to be a fixed array instead of a pointer. * Define regex early so line ends before 80 columns. * Remove obsolete comments. * Rename AsyncHooks::InitScope::uid_fields_ to uid_fields_ptr_ because using the same name as AsyncHooks::uid_fields_ was confusing. * Place variables that are used to store the new set of hooks if another hook is enabled/disabled during hook execution into an object to act as a namespace. PR-URL: https://github.com/nodejs/node/pull/14722 Reviewed-By: Anna Henningsen Reviewed-By: Refael Ackermann --- lib/async_hooks.js | 190 ++++++++++++++++++++++++--------------------- src/env-inl.h | 6 +- src/env.h | 10 +-- 3 files changed, 109 insertions(+), 97 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 51039e9f36d24d..38bdbb0306b6be 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -2,39 +2,58 @@ const internalUtil = require('internal/util'); const async_wrap = process.binding('async_wrap'); -/* Both these arrays are used to communicate between JS and C++ with as little - * overhead as possible. +/* async_hook_fields is a Uint32Array wrapping the uint32_t array of + * Environment::AsyncHooks::fields_[]. Each index tracks the number of active + * hooks for each type. * - * async_hook_fields is a Uint32Array() that communicates the number of each - * type of active hooks of each type and wraps the uin32_t array of - * node::Environment::AsyncHooks::fields_. - * - * async_uid_fields is a Float64Array() that contains the async/trigger ids for - * several operations. These fields are as follows: - * kCurrentAsyncId: The async id of the current execution stack. - * kCurrentTriggerId: The trigger id of the current execution stack. - * kAsyncUidCntr: Counter that tracks the unique ids given to new resources. - * kInitTriggerId: Written to just before creating a new resource, so the - * constructor knows what other resource is responsible for its init(). - * Used this way so the trigger id doesn't need to be passed to every - * resource's constructor. + * async_uid_fields is a Float64Array wrapping the double array of + * Environment::AsyncHooks::uid_fields_[]. Each index contains the ids for the + * various asynchronous states of the application. These are: + * kCurrentAsyncId: The async_id assigned to the resource responsible for the + * current execution stack. + * kCurrentTriggerId: The trigger_async_id of the resource responsible for the + * current execution stack. + * kAsyncUidCntr: Incremental counter tracking the next assigned async_id. + * kInitTriggerId: Written immediately before a resource's constructor that + * sets the value of the init()'s triggerAsyncId. The order of retrieving + * the triggerAsyncId value is passing directly to the constructor -> value + * set in kInitTriggerId -> executionAsyncId of the current resource. */ const { async_hook_fields, async_uid_fields } = async_wrap; -// Used to change the state of the async id stack. +// Store the pair executionAsyncId and triggerAsyncId in a std::stack on +// Environment::AsyncHooks::ids_stack_ tracks the resource responsible for the +// current execution stack. This is unwound as each resource exits. In the case +// of a fatal exception this stack is emptied after calling each hook's after() +// callback. const { pushAsyncIds, popAsyncIds } = async_wrap; -// Array of all AsyncHooks that will be iterated whenever an async event fires. -// 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 = []; -// 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; -// Keep track of the field counts held in tmp_active_hooks_array. -var tmp_async_hook_fields = null; +// For performance reasons, only track Proimses when a hook is enabled. +const { enablePromiseHook, disablePromiseHook } = async_wrap; +// Properties in active_hooks are used to keep track of the set of hooks being +// executed in case another hook is enabled/disabled. The new set of hooks is +// then restored once the active set of hooks is finished executing. +const active_hooks = { + // Array of all AsyncHooks that will be iterated whenever an async event + // fires. Using var instead of (preferably const) in order to assign + // active_hooks.tmp_array if a hook is enabled/disabled during hook + // execution. + array: [], + // Use a counter to track nested calls of async hook callbacks and make sure + // the active_hooks.array isn't altered mid execution. + call_depth: 0, + // Use to temporarily store and updated active_hooks.array if the user + // enables or disables a hook while hooks are being processed. If a hook is + // enabled() or disabled() during hook execution then the current set of + // active hooks is duplicated and set equal to active_hooks.tmp_array. Any + // subsequent changes are on the duplicated array. When all hooks have + // completed executing active_hooks.tmp_array is assigned to + // active_hooks.array. + tmp_array: null, + // Keep track of the field counts held in active_hooks.tmp_array. Because the + // async_hook_fields can't be reassigned, store each uint32 in an array that + // is written back to async_hook_fields when active_hooks.array is restored. + tmp_fields: null +}; + // 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 @@ -43,6 +62,9 @@ const { kInit, kBefore, kAfter, kDestroy, kTotals, kCurrentAsyncId, kCurrentTriggerId, kAsyncUidCntr, kInitTriggerId } = async_wrap.constants; +// Symbols used to store the respective ids on both AsyncResource instances and +// internal resources. They will also be assigned to arbitrary objects passed +// in by the user that take place of internally constructed objects. const { async_id_symbol, trigger_id_symbol } = async_wrap; // Used in AsyncHook and AsyncResource. @@ -54,6 +76,9 @@ const emitBeforeNative = emitHookFactory(before_symbol, 'emitBeforeNative'); const emitAfterNative = emitHookFactory(after_symbol, 'emitAfterNative'); const emitDestroyNative = emitHookFactory(destroy_symbol, 'emitDestroyNative'); +// TODO(refack): move to node-config.cc +const abort_regex = /^--abort[_-]on[_-]uncaught[_-]exception$/; + // Setup the callbacks that node::AsyncWrap will call when there are hooks to // process. They use the same functions as the JS embedder API. These callbacks // are setup immediately to prevent async_wrap.setupHooks() from being hijacked @@ -72,7 +97,7 @@ function fatalError(e) { Error.captureStackTrace(o, fatalError); process._rawDebug(o.stack); } - if (process.execArgv.some((e) => /^--abort[_-]on[_-]uncaught[_-]exception$/.test(e))) { + if (process.execArgv.some((e) => abort_regex.test(e))) { process.abort(); } process.exit(1); @@ -122,7 +147,7 @@ class AsyncHook { hooks_array.push(this); if (prev_kTotals === 0 && hook_fields[kTotals] > 0) - async_wrap.enablePromiseHook(); + enablePromiseHook(); return this; } @@ -144,7 +169,7 @@ class AsyncHook { hooks_array.splice(index, 1); if (prev_kTotals > 0 && hook_fields[kTotals] === 0) - async_wrap.disablePromiseHook(); + disablePromiseHook(); return this; } @@ -152,41 +177,41 @@ class AsyncHook { function getHookArrays() { - if (processing_hook === 0) - return [active_hooks_array, async_hook_fields]; + if (active_hooks.call_depth === 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 // and store this there. This shouldn't fire until the next time hooks are // processed. - if (tmp_active_hooks_array === null) + if (active_hooks.tmp_array === null) storeActiveHooks(); - return [tmp_active_hooks_array, tmp_async_hook_fields]; + return [active_hooks.tmp_array, active_hooks.tmp_fields]; } function storeActiveHooks() { - tmp_active_hooks_array = active_hooks_array.slice(); + active_hooks.tmp_array = active_hooks.array.slice(); // Don't want to make the assumption that kInit to kDestroy are indexes 0 to // 4. So do this the long way. - tmp_async_hook_fields = []; - tmp_async_hook_fields[kInit] = async_hook_fields[kInit]; - tmp_async_hook_fields[kBefore] = async_hook_fields[kBefore]; - tmp_async_hook_fields[kAfter] = async_hook_fields[kAfter]; - tmp_async_hook_fields[kDestroy] = async_hook_fields[kDestroy]; + active_hooks.tmp_fields = []; + active_hooks.tmp_fields[kInit] = async_hook_fields[kInit]; + active_hooks.tmp_fields[kBefore] = async_hook_fields[kBefore]; + active_hooks.tmp_fields[kAfter] = async_hook_fields[kAfter]; + active_hooks.tmp_fields[kDestroy] = async_hook_fields[kDestroy]; } // Then restore the correct hooks array in case any hooks were added/removed // during hook callback execution. -function restoreTmpHooks() { - active_hooks_array = tmp_active_hooks_array; - async_hook_fields[kInit] = tmp_async_hook_fields[kInit]; - async_hook_fields[kBefore] = tmp_async_hook_fields[kBefore]; - async_hook_fields[kAfter] = tmp_async_hook_fields[kAfter]; - async_hook_fields[kDestroy] = tmp_async_hook_fields[kDestroy]; - - tmp_active_hooks_array = null; - tmp_async_hook_fields = null; +function restoreActiveHooks() { + active_hooks.array = active_hooks.tmp_array; + async_hook_fields[kInit] = active_hooks.tmp_fields[kInit]; + async_hook_fields[kBefore] = active_hooks.tmp_fields[kBefore]; + async_hook_fields[kAfter] = active_hooks.tmp_fields[kAfter]; + async_hook_fields[kDestroy] = active_hooks.tmp_fields[kDestroy]; + + active_hooks.tmp_array = null; + active_hooks.tmp_fields = null; } @@ -334,25 +359,30 @@ function emitHookFactory(symbol, name) { // before this is called. // eslint-disable-next-line func-style const fn = function(asyncId) { - processing_hook += 1; + active_hooks.call_depth += 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++) { - if (typeof active_hooks_array[i][symbol] === 'function') { - active_hooks_array[i][symbol](asyncId); + for (var i = 0; i < active_hooks.array.length; i++) { + if (typeof active_hooks.array[i][symbol] === 'function') { + active_hooks.array[i][symbol](asyncId); } } } catch (e) { fatalError(e); } finally { - processing_hook -= 1; + active_hooks.call_depth -= 1; } - if (processing_hook === 0 && tmp_active_hooks_array !== null) { - restoreTmpHooks(); + // Hooks can only be restored if there have been no recursive hook calls. + // Also the active hooks do not need to be restored if enable()/disable() + // weren't called during hook execution, in which case + // active_hooks.tmp_array will be null. + if (active_hooks.call_depth === 0 && active_hooks.tmp_array !== null) { + restoreActiveHooks(); } }; + // Set the name property of the anonymous function as it looks good in the // stack trace. Object.defineProperty(fn, 'name', { @@ -379,9 +409,6 @@ function emitBeforeScript(asyncId, triggerAsyncId) { } -// TODO(trevnorris): Calling emitBefore/emitAfter from native can't adjust the -// kIdStackIndex. But what happens if the user doesn't have both before and -// after callbacks. function emitAfterScript(asyncId) { if (async_hook_fields[kAfter] > 0) emitAfterNative(asyncId); @@ -399,26 +426,16 @@ function emitDestroyScript(asyncId) { } -// Emit callbacks for native calls. Since some state can be setup directly from -// C++ there's no need to perform all the work here. - -// This should only be called if hooks_array has kInit > 0. There are no global -// values to setup. Though hooks_array will be cloned if C++ needed to call -// init(). -// TODO(trevnorris): Perhaps have MakeCallback call a single JS function that -// does the before/callback/after calls to remove two additional calls to JS. - -// Force the application to shutdown if one of the callbacks throws. This may -// change in the future depending on whether it can be determined if there's a -// slim chance of the application remaining stable after handling one of these -// exceptions. +// Used by C++ to call all init() callbacks. Because some state can be setup +// from C++ there's no need to perform all the same operations as in +// emitInitScript. function emitInitNative(asyncId, type, triggerAsyncId, resource) { - processing_hook += 1; + active_hooks.call_depth += 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++) { - if (typeof active_hooks_array[i][init_symbol] === 'function') { - active_hooks_array[i][init_symbol]( + for (var i = 0; i < active_hooks.array.length; i++) { + if (typeof active_hooks.array[i][init_symbol] === 'function') { + active_hooks.array[i][init_symbol]( asyncId, type, triggerAsyncId, resource ); @@ -427,18 +444,15 @@ function emitInitNative(asyncId, type, triggerAsyncId, resource) { } catch (e) { fatalError(e); } finally { - processing_hook -= 1; + active_hooks.call_depth -= 1; } - // * `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(); + // Hooks can only be restored if there have been no recursive hook calls. + // Also the active hooks do not need to be restored if enable()/disable() + // weren't called during hook execution, in which case active_hooks.tmp_array + // will be null. + if (active_hooks.call_depth === 0 && active_hooks.tmp_array !== null) { + restoreActiveHooks(); } } diff --git a/src/env-inl.h b/src/env-inl.h index b651d969a3d7e8..87fe7e36a2f001 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -180,13 +180,13 @@ inline void Environment::AsyncHooks::clear_id_stack() { inline Environment::AsyncHooks::InitScope::InitScope( Environment* env, double init_trigger_id) : env_(env), - uid_fields_(env->async_hooks()->uid_fields()) { - env->async_hooks()->push_ids(uid_fields_[AsyncHooks::kCurrentAsyncId], + uid_fields_ref_(env->async_hooks()->uid_fields()) { + env->async_hooks()->push_ids(uid_fields_ref_[AsyncHooks::kCurrentAsyncId], init_trigger_id); } inline Environment::AsyncHooks::InitScope::~InitScope() { - env_->async_hooks()->pop_ids(uid_fields_[AsyncHooks::kCurrentAsyncId]); + env_->async_hooks()->pop_ids(uid_fields_ref_[AsyncHooks::kCurrentAsyncId]); } inline Environment::AsyncHooks::ExecScope::ExecScope( diff --git a/src/env.h b/src/env.h index 47a6487b77dbe6..7ec6bfe08bb7e9 100644 --- a/src/env.h +++ b/src/env.h @@ -410,7 +410,7 @@ class Environment { private: Environment* env_; - double* uid_fields_; + double* uid_fields_ref_; DISALLOW_COPY_AND_ASSIGN(InitScope); }; @@ -443,12 +443,10 @@ class Environment { v8::Isolate* isolate_; // Stores the ids of the current execution context stack. std::stack ids_stack_; - // Used to communicate state between C++ and JS cheaply. Is placed in an - // Uint32Array() and attached to the async_wrap object. + // Attached to a Uint32Array that tracks the number of active hooks for + // each type. uint32_t fields_[kFieldsCount]; - // Used to communicate ids between C++ and JS cheaply. Placed in a - // Float64Array and attached to the async_wrap object. Using a double only - // gives us 2^53-1 unique ids, but that should be sufficient. + // Attached to a Float64Array that tracks the state of async resources. double uid_fields_[kUidFieldsCount]; DISALLOW_COPY_AND_ASSIGN(AsyncHooks);