async_hooks: move PromiseHook handler to JS

This avoids the need to wrap every promise in an AsyncWrap and also
makes it easier to skip the machinery to track destroy events when
there's no destroy listener.

Co-authored-by: Andrey Pechkurov <apechkurov@gmail.com>

PR-URL: https://github.com/nodejs/node/pull/32891
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

benchmark/async_hooks/promises.js

@@ -2,10 +2,31 @@
 const common = require('../common.js');
 const { createHook } = require('async_hooks');
 
+let hook;
+const tests = {
+  disabled() {
+    hook = createHook({
+      promiseResolve() {}
+    });
+  },
+  enabled() {
+    hook = createHook({
+      promiseResolve() {}
+    }).enable();
+  },
+  enabledWithDestroy() {
+    hook = createHook({
+      promiseResolve() {},
+      destroy() {}
+    }).enable();
+  }
+};
+
 const bench = common.createBenchmark(main, {
   n: [1e6],
   asyncHooks: [
     'enabled',
+    'enabledWithDestroy',
     'disabled',
   ]
 });
@@ -19,10 +40,8 @@ async function run(n) {
 }
 
 function main({ n, asyncHooks }) {
-  const hook = createHook({ promiseResolve() {} });
-  if (asyncHooks !== 'disabled') {
-    hook.enable();
-  }
+  if (hook) hook.disable();
+  tests[asyncHooks]();
   bench.start();
   run(n).then(() => {
     bench.end(n);

doc/api/async_hooks.md

@@ -306,11 +306,6 @@ currently not considered public, but using the Embedder API, users can provide
 and document their own resource objects. For example, such a resource object
 could contain the SQL query being executed.
 
-In the case of Promises, the `resource` object will have an
-`isChainedPromise` property, set to `true` if the promise has a parent promise,
-and `false` otherwise. For example, in the case of `b = a.then(handler)`, `a` is
-considered a parent `Promise` of `b`. Here, `b` is considered a chained promise.
-
 In some cases the resource object is reused for performance reasons, it is
 thus not safe to use it as a key in a `WeakMap` or add properties to it.
 

lib/async_hooks.js

@@ -26,6 +26,7 @@ const {
   getHookArrays,
   enableHooks,
   disableHooks,
+  updatePromiseHookMode,
   executionAsyncResource,
   // Internal Embedder API
   newAsyncId,
@@ -101,6 +102,8 @@ class AsyncHook {
       enableHooks();
     }
 
+    updatePromiseHookMode();
+
     return this;
   }
 

lib/internal/async_hooks.js

@@ -3,7 +3,9 @@
 const {
   Error,
   FunctionPrototypeBind,
+  ObjectPrototypeHasOwnProperty,
   ObjectDefineProperty,
+  Promise,
   Symbol,
 } = primordials;
 
@@ -86,9 +88,10 @@ const { kInit, kBefore, kAfter, kDestroy, kTotals, kPromiseResolve,
         kCheck, kExecutionAsyncId, kAsyncIdCounter, kTriggerAsyncId,
         kDefaultTriggerAsyncId, kStackLength } = async_wrap.constants;
 
+const { async_id_symbol,
+        trigger_async_id_symbol } = internalBinding('symbols');
+
 // Used in AsyncHook and AsyncResource.
-const async_id_symbol = Symbol('asyncId');
-const trigger_async_id_symbol = Symbol('triggerAsyncId');
 const init_symbol = Symbol('init');
 const before_symbol = Symbol('before');
 const after_symbol = Symbol('after');
@@ -243,28 +246,90 @@ function restoreActiveHooks() {
   active_hooks.tmp_fields = null;
 }
 
+function trackPromise(promise, parent, silent) {
+  const asyncId = getOrSetAsyncId(promise);
+
+  promise[trigger_async_id_symbol] = parent ? getOrSetAsyncId(parent) :
+    getDefaultTriggerAsyncId();
+
+  if (!silent && initHooksExist()) {
+    const triggerId = promise[trigger_async_id_symbol];
+    emitInitScript(asyncId, 'PROMISE', triggerId, promise);
+  }
+}
+
+function fastPromiseHook(type, promise, parent) {
+  if (type === kInit || !promise[async_id_symbol]) {
+    const silent = type !== kInit;
+    if (parent instanceof Promise) {
+      trackPromise(promise, parent, silent);
+    } else {
+      trackPromise(promise, null, silent);
+    }
+
+    if (!silent) return;
+  }
+
+  const asyncId = promise[async_id_symbol];
+  switch (type) {
+    case kBefore:
+      const triggerId = promise[trigger_async_id_symbol];
+      emitBeforeScript(asyncId, triggerId, promise);
+      break;
+    case kAfter:
+      if (hasHooks(kAfter)) {
+        emitAfterNative(asyncId);
+      }
+      if (asyncId === executionAsyncId()) {
+        // This condition might not be true if async_hooks was enabled during
+        // the promise callback execution.
+        // Popping it off the stack can be skipped in that case, because it is
+        // known that it would correspond to exactly one call with
+        // PromiseHookType::kBefore that was not witnessed by the PromiseHook.
+        popAsyncContext(asyncId);
+      }
+      break;
+    case kPromiseResolve:
+      emitPromiseResolveNative(asyncId);
+      break;
+  }
+}
 
 let wantPromiseHook = false;
 function enableHooks() {
   async_hook_fields[kCheck] += 1;
+}
 
+let promiseHookMode = -1;
+function updatePromiseHookMode() {
   wantPromiseHook = true;
+  if (destroyHooksExist()) {
+    if (promiseHookMode !== 1) {
+      promiseHookMode = 1;
       enablePromiseHook();
     }
+  } else if (promiseHookMode !== 0) {
+    promiseHookMode = 0;
+    enablePromiseHook(fastPromiseHook);
+  }
+}
 
 function disableHooks() {
   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)
+  if (!wantPromiseHook) {
+    promiseHookMode = -1;
     disablePromiseHook();
   }
+}
 
 // Internal Embedder API //
 
@@ -276,7 +341,7 @@ function newAsyncId() {
 }
 
 function getOrSetAsyncId(object) {
-  if (object.hasOwnProperty(async_id_symbol)) {
+  if (ObjectPrototypeHasOwnProperty(object, async_id_symbol)) {
     return object[async_id_symbol];
   }
 
@@ -447,6 +512,7 @@ module.exports = {
   },
   enableHooks,
   disableHooks,
+  updatePromiseHookMode,
   clearDefaultTriggerAsyncId,
   clearAsyncIdStack,
   hasAsyncIdStack,

src/async_wrap.cc

@@ -39,7 +39,9 @@ using v8::HandleScope;
 using v8::Integer;
 using v8::Isolate;
 using v8::Local;
+using v8::Maybe;
 using v8::MaybeLocal;
+using v8::Name;
 using v8::Number;
 using v8::Object;
 using v8::ObjectTemplate;
@@ -189,46 +191,107 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
 
 class PromiseWrap : public AsyncWrap {
  public:
-  enum InternalFields {
-    kIsChainedPromiseField = AsyncWrap::kInternalFieldCount,
-    kInternalFieldCount
-  };
   PromiseWrap(Environment* env, Local<Object> object, bool silent)
       : AsyncWrap(env, object, PROVIDER_PROMISE, kInvalidAsyncId, silent) {
     MakeWeak();
   }
 
+  PromiseWrap(Environment* env, Local<Object> object, double asyncId,
+    double triggerAsyncId)
+      : AsyncWrap(env, object, PROVIDER_PROMISE, asyncId, triggerAsyncId) {
+    MakeWeak();
+  }
+
   SET_NO_MEMORY_INFO()
   SET_MEMORY_INFO_NAME(PromiseWrap)
   SET_SELF_SIZE(PromiseWrap)
 
   static PromiseWrap* New(Environment* env,
                           Local<Promise> promise,
-                          PromiseWrap* parent_wrap,
                           bool silent);
-  static void getIsChainedPromise(Local<String> property,
-                                  const PropertyCallbackInfo<Value>& info);
+  static void GetAsyncId(Local<Name> property,
+                         const PropertyCallbackInfo<Value>& args);
+  static void GetTriggerAsyncId(Local<Name> property,
+                                const PropertyCallbackInfo<Value>& args);
+
+  static void Initialize(Environment* env);
 };
 
 PromiseWrap* PromiseWrap::New(Environment* env,
                               Local<Promise> promise,
-                              PromiseWrap* parent_wrap,
                               bool silent) {
+  Local<Context> context = env->context();
+
   Local<Object> obj;
-  if (!env->promise_wrap_template()->NewInstance(env->context()).ToLocal(&obj))
+  if (!env->promise_wrap_template()->NewInstance(context).ToLocal(&obj))
     return nullptr;
-  obj->SetInternalField(PromiseWrap::kIsChainedPromiseField,
-                        parent_wrap != nullptr ? v8::True(env->isolate())
-                                               : v8::False(env->isolate()));
+
   CHECK_NULL(promise->GetAlignedPointerFromInternalField(0));
   promise->SetInternalField(0, obj);
+
+  // Skip for init events
+  if (silent) {
+    Local<Value> maybeAsyncId = promise
+        ->Get(context, env->async_id_symbol())
+        .ToLocalChecked();
+
+    Local<Value> maybeTriggerAsyncId = promise
+        ->Get(context, env->trigger_async_id_symbol())
+        .ToLocalChecked();
+
+    if (maybeAsyncId->IsNumber() && maybeTriggerAsyncId->IsNumber()) {
+      double asyncId = maybeAsyncId->NumberValue(context).ToChecked();
+      double triggerAsyncId = maybeTriggerAsyncId->NumberValue(context)
+          .ToChecked();
+      return new PromiseWrap(env, obj, asyncId, triggerAsyncId);
+    }
+  }
+
   return new PromiseWrap(env, obj, silent);
 }
 
-void PromiseWrap::getIsChainedPromise(Local<String> property,
+void PromiseWrap::GetAsyncId(Local<Name> property,
                              const PropertyCallbackInfo<Value>& info) {
-  info.GetReturnValue().Set(
-      info.Holder()->GetInternalField(PromiseWrap::kIsChainedPromiseField));
+  Isolate* isolate = info.GetIsolate();
+  HandleScope scope(isolate);
+
+  PromiseWrap* wrap = Unwrap<PromiseWrap>(info.Holder());
+  double value = wrap->get_async_id();
+
+  info.GetReturnValue().Set(Number::New(isolate, value));
+}
+
+void PromiseWrap::GetTriggerAsyncId(Local<Name> property,
+                                    const PropertyCallbackInfo<Value>& info) {
+  Isolate* isolate = info.GetIsolate();
+  HandleScope scope(isolate);
+
+  PromiseWrap* wrap = Unwrap<PromiseWrap>(info.Holder());
+  double value = wrap->get_trigger_async_id();
+
+  info.GetReturnValue().Set(Number::New(isolate, value));
+}
+
+void PromiseWrap::Initialize(Environment* env) {
+  Isolate* isolate = env->isolate();
+  HandleScope scope(isolate);
+
+  Local<FunctionTemplate> ctor = FunctionTemplate::New(isolate);
+  ctor->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "PromiseWrap"));
+
+  Local<ObjectTemplate> promise_wrap_template = ctor->InstanceTemplate();
+  env->set_promise_wrap_template(promise_wrap_template);
+
+  promise_wrap_template->SetInternalFieldCount(
+      PromiseWrap::kInternalFieldCount);
+
+  promise_wrap_template->SetAccessor(
+      env->async_id_symbol(),
+      PromiseWrap::GetAsyncId);
+
+  promise_wrap_template->SetAccessor(
+      env->trigger_async_id_symbol(),
+      PromiseWrap::GetTriggerAsyncId);
 }
 
 static PromiseWrap* extractPromiseWrap(Local<Promise> promise) {
@@ -240,7 +303,34 @@ static PromiseWrap* extractPromiseWrap(Local<Promise> promise) {
   return obj->IsObject() ? Unwrap<PromiseWrap>(obj.As<Object>()) : nullptr;
 }
 
-static void PromiseHook(PromiseHookType type, Local<Promise> promise,
+static uint16_t ToAsyncHooksType(PromiseHookType type) {
+  switch (type) {
+    case PromiseHookType::kInit:    return AsyncHooks::kInit;
+    case PromiseHookType::kBefore:  return AsyncHooks::kBefore;
+    case PromiseHookType::kAfter:   return AsyncHooks::kAfter;
+    case PromiseHookType::kResolve: return AsyncHooks::kPromiseResolve;
+  }
+}
+
+// Simplified JavaScript hook fast-path for when there is no destroy hook
+static void FastPromiseHook(PromiseHookType type, Local<Promise> promise,
+                            Local<Value> parent) {
+  Local<Context> context = promise->CreationContext();
+  Environment* env = Environment::GetCurrent(context);
+  if (env == nullptr) return;
+
+  Local<Value> argv[] = {
+    Integer::New(env->isolate(), ToAsyncHooksType(type)),
+    promise,
+    parent
+  };
+
+  TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal);
+  Local<Function> promise_hook = env->promise_hook_handler();
+  USE(promise_hook->Call(context, Undefined(env->isolate()), 3, argv));
+}
+
+static void FullPromiseHook(PromiseHookType type, Local<Promise> promise,
                             Local<Value> parent) {
   Local<Context> context = promise->CreationContext();
 
@@ -261,14 +351,14 @@ static void PromiseHook(PromiseHookType type, Local<Promise> promise,
       Local<Promise> parent_promise = parent.As<Promise>();
       PromiseWrap* parent_wrap = extractPromiseWrap(parent_promise);
       if (parent_wrap == nullptr) {
-        parent_wrap = PromiseWrap::New(env, parent_promise, nullptr, true);
+        parent_wrap = PromiseWrap::New(env, parent_promise, true);
         if (parent_wrap == nullptr) return;
       }
 
       AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(parent_wrap);
-      wrap = PromiseWrap::New(env, promise, parent_wrap, silent);
+      wrap = PromiseWrap::New(env, promise, silent);
     } else {
-      wrap = PromiseWrap::New(env, promise, nullptr, silent);
+      wrap = PromiseWrap::New(env, promise, silent);
     }
   }
 
@@ -295,7 +385,6 @@ static void PromiseHook(PromiseHookType type, Local<Promise> promise,
   }
 }
 
-
 static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
@@ -324,34 +413,28 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
   SET_HOOK_FN(destroy);
   SET_HOOK_FN(promise_resolve);
 #undef SET_HOOK_FN
-
-  {
-    Local<FunctionTemplate> ctor =
-        FunctionTemplate::New(env->isolate());
-    ctor->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "PromiseWrap"));
-    Local<ObjectTemplate> promise_wrap_template = ctor->InstanceTemplate();
-    promise_wrap_template->SetInternalFieldCount(
-        PromiseWrap::kInternalFieldCount);
-    promise_wrap_template->SetAccessor(
-        FIXED_ONE_BYTE_STRING(env->isolate(), "isChainedPromise"),
-        PromiseWrap::getIsChainedPromise);
-    env->set_promise_wrap_template(promise_wrap_template);
 }
-}
-
 
 static void EnablePromiseHook(const FunctionCallbackInfo<Value>& args) {
-  args.GetIsolate()->SetPromiseHook(PromiseHook);
+  Environment* env = Environment::GetCurrent(args);
+
+  if (args[0]->IsFunction()) {
+    env->set_promise_hook_handler(args[0].As<Function>());
+    args.GetIsolate()->SetPromiseHook(FastPromiseHook);
+  } else {
+    args.GetIsolate()->SetPromiseHook(FullPromiseHook);
+  }
 }
 
 
 static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
-  Isolate* isolate = args.GetIsolate();
+  Environment* env = Environment::GetCurrent(args);
+  env->set_promise_hook_handler(Local<Function>());
 
   // 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);
+  args.GetIsolate()->SetPromiseHook(nullptr);
 }
 
 
@@ -577,6 +660,9 @@ void AsyncWrap::Initialize(Local<Object> target,
       FIXED_ONE_BYTE_STRING(env->isolate(), "AsyncWrap"),
       AsyncWrapObject::GetConstructorTemplate(env)
           ->GetFunction(env->context()).ToLocalChecked()).Check();
+
+  // TODO(qard): maybe this should be GetConstructorTemplate instead?
+  PromiseWrap::Initialize(env);
 }
 
 
@@ -600,6 +686,15 @@ AsyncWrap::AsyncWrap(Environment* env,
   init_hook_ran_ = true;
 }
 
+AsyncWrap::AsyncWrap(Environment* env,
+                     Local<Object> object,
+                     ProviderType provider,
+                     double execution_async_id,
+                     double trigger_async_id)
+    : AsyncWrap(env, object, provider, execution_async_id, true) {
+  trigger_async_id_ = trigger_async_id;
+}
+
 AsyncWrap::AsyncWrap(Environment* env, Local<Object> object)
   : BaseObject(env, object) {
 }

src/async_wrap.h

@@ -209,6 +209,11 @@ class AsyncWrap : public BaseObject {
             ProviderType provider,
             double execution_async_id,
             bool silent);
+  AsyncWrap(Environment* env,
+            v8::Local<v8::Object> promise,
+            ProviderType provider,
+            double execution_async_id,
+            double trigger_async_id);
   ProviderType provider_type_ = PROVIDER_NONE;
   bool init_hook_ran_ = false;
   // Because the values may be Reset(), cannot be made const.

src/env.h

@@ -156,11 +156,13 @@ constexpr size_t kFsStatsBufferLength =
 // Symbols are per-isolate primitives but Environment proxies them
 // for the sake of convenience.
 #define PER_ISOLATE_SYMBOL_PROPERTIES(V)                                       \
+  V(async_id_symbol, "async_id_symbol")                                        \
   V(handle_onclose_symbol, "handle_onclose")                                   \
   V(no_message_symbol, "no_message_symbol")                                    \
   V(oninit_symbol, "oninit")                                                   \
   V(owner_symbol, "owner")                                                     \
   V(onpskexchange_symbol, "onpskexchange")                                     \
+  V(trigger_async_id_symbol, "trigger_async_id_symbol")                        \
 
 // Strings are per-isolate primitives but Environment proxies them
 // for the sake of convenience.  Strings should be ASCII-only.
@@ -457,6 +459,7 @@ constexpr size_t kFsStatsBufferLength =
   V(prepare_stack_trace_callback, v8::Function)                                \
   V(process_object, v8::Object)                                                \
   V(primordials, v8::Object)                                                   \
+  V(promise_hook_handler, v8::Function)                                        \
   V(promise_reject_callback, v8::Function)                                     \
   V(script_data_constructor_function, v8::Function)                            \
   V(source_map_cache_getter, v8::Function)                                     \

test/addons/async-hooks-promise/test.js

@@ -1,8 +1,11 @@
 'use strict';
+// Flags: --expose-internals
 
 const common = require('../../common');
 const assert = require('assert');
 const async_hooks = require('async_hooks');
+const { async_id_symbol,
+        trigger_async_id_symbol } = require('internal/async_hooks').symbols;
 const binding = require(`./build/${common.buildType}/binding`);
 
 if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) {
@@ -15,28 +18,60 @@ assert.strictEqual(
   binding.getPromiseField(Promise.resolve(1)),
   0);
 
-const hook0 = async_hooks.createHook({}).enable();
+const emptyHook = async_hooks.createHook({}).enable();
 
 // Check that no PromiseWrap is created when there are no hook callbacks.
 assert.strictEqual(
   binding.getPromiseField(Promise.resolve(1)),
   0);
 
-hook0.disable();
+emptyHook.disable();
+
+let lastResource;
+let lastAsyncId;
+let lastTriggerAsyncId;
+const initOnlyHook = async_hooks.createHook({
+  init(asyncId, type, triggerAsyncId, resource) {
+    lastAsyncId = asyncId;
+    lastTriggerAsyncId = triggerAsyncId;
+    lastResource = resource;
+  }
+}).enable();
+
+// Check that no PromiseWrap is created when only using an init hook.
+{
+  const promise = Promise.resolve(1);
+  assert.strictEqual(binding.getPromiseField(promise), 0);
+  assert.strictEqual(lastResource, promise);
+  assert.strictEqual(lastAsyncId, promise[async_id_symbol]);
+  assert.strictEqual(lastTriggerAsyncId, promise[trigger_async_id_symbol]);
+}
+
+initOnlyHook.disable();
+
+lastResource = null;
+const hookWithDestroy = async_hooks.createHook({
+  init(asyncId, type, triggerAsyncId, resource) {
+    lastAsyncId = asyncId;
+    lastTriggerAsyncId = triggerAsyncId;
+    lastResource = resource;
+  },
+
+  destroy() {
 
-let pwrap = null;
-const hook1 = async_hooks.createHook({
-  init(id, type, tid, resource) {
-    pwrap = resource;
   }
 }).enable();
 
 // Check that the internal field returns the same PromiseWrap passed to init().
-assert.strictEqual(
-  binding.getPromiseField(Promise.resolve(1)),
-  pwrap);
+{
+  const promise = Promise.resolve(1);
+  const promiseWrap = binding.getPromiseField(promise);
+  assert.strictEqual(lastResource, promiseWrap);
+  assert.strictEqual(lastAsyncId, promiseWrap[async_id_symbol]);
+  assert.strictEqual(lastTriggerAsyncId, promiseWrap[trigger_async_id_symbol]);
+}
 
-hook1.disable();
+hookWithDestroy.disable();
 
 // Check that internal fields are no longer being set. This needs to be delayed
 // a bit because the `disable()` call only schedules disabling the hook in a
@@ -45,4 +80,25 @@ setImmediate(() => {
   assert.strictEqual(
     binding.getPromiseField(Promise.resolve(1)),
     0);
+
+  const noDestroyHook = async_hooks.createHook({
+    init(asyncId, type, triggerAsyncId, resource) {
+      lastAsyncId = asyncId;
+      lastTriggerAsyncId = triggerAsyncId;
+      lastResource = resource;
+    },
+
+    before() {},
+    after() {},
+    resolve() {}
+  }).enable();
+
+  // Check that no PromiseWrap is created when there is no destroy hook.
+  const promise = Promise.resolve(1);
+  assert.strictEqual(binding.getPromiseField(promise), 0);
+  assert.strictEqual(lastResource, promise);
+  assert.strictEqual(lastAsyncId, promise[async_id_symbol]);
+  assert.strictEqual(lastTriggerAsyncId, promise[trigger_async_id_symbol]);
+
+  noDestroyHook.disable();
 });

test/parallel/test-async-hooks-promise.js

@@ -24,6 +24,4 @@ const a = Promise.resolve(42);
 a.then(common.mustCall());
 
 assert.strictEqual(initCalls[0].triggerId, 1);
-assert.strictEqual(initCalls[0].resource.isChainedPromise, false);
 assert.strictEqual(initCalls[1].triggerId, initCalls[0].id);
-assert.strictEqual(initCalls[1].resource.isChainedPromise, true);

test/parallel/test-bootstrap-modules.js

@@ -24,6 +24,7 @@ const expectedModules = new Set([
   'Internal Binding process_methods',
   'Internal Binding report',
   'Internal Binding string_decoder',
+  'Internal Binding symbols',
   'Internal Binding task_queue',
   'Internal Binding timers',
   'Internal Binding trace_events',