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: #14722
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>

Author: trevnorris

lib/async_hooks.js

@@ -1,39 +1,58 @@
 'use strict';
 
 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
@@ -42,6 +61,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.
@@ -53,6 +75,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
@@ -71,7 +96,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);
@@ -121,7 +146,7 @@ class AsyncHook {
     hooks_array.push(this);
 
     if (prev_kTotals === 0 && hook_fields[kTotals] > 0)
-      async_wrap.enablePromiseHook();
+      enablePromiseHook();
 
     return this;
   }
@@ -143,49 +168,49 @@ class AsyncHook {
     hooks_array.splice(index, 1);
 
     if (prev_kTotals > 0 && hook_fields[kTotals] === 0)
-      async_wrap.disablePromiseHook();
+      disablePromiseHook();
 
     return this;
   }
 }
 
 
 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;
 }
 
 
@@ -322,25 +347,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', {
@@ -367,9 +397,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);
@@ -387,26 +414,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
         );
@@ -415,18 +432,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();
   }
 }
 

src/env-inl.h

@@ -176,13 +176,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(

src/env.h

@@ -405,7 +405,7 @@ class Environment {
 
      private:
       Environment* env_;
-      double* uid_fields_;
+      double* uid_fields_ref_;
 
       DISALLOW_COPY_AND_ASSIGN(InitScope);
     };
@@ -438,12 +438,10 @@ class Environment {
     v8::Isolate* isolate_;
     // Stores the ids of the current execution context stack.
     std::stack<struct node_async_ids> 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);