async_hooks: C++ Embedder API overhaul

* Fix AsyncHooksGetTriggerAsyncId such it corresponds to
async_hooks.triggerAsyncId and not async_hooks.initTriggerId.
* Use an async_context struct instead of two async_uid values.
  This change was necessary since the fixing
  AsyncHooksGetTriggerAsyncId otherwise makes it impossible to
  get the correct default trigger id. It also prevents an invalid
  triggerAsyncId in MakeCallback.
* Rename async_uid to async_id for consistency
* Rename get_uid to get_async_id
* Add get_trigger_async_id to AsyncResource class

PR-URL: https://github.com/nodejs/node/pull/14040
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

src/async-wrap.cc

@@ -741,40 +741,51 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
 /* Public C++ embedder API */
 
 
-async_uid AsyncHooksGetExecutionAsyncId(Isolate* isolate) {
+async_id AsyncHooksGetExecutionAsyncId(Isolate* isolate) {
   return Environment::GetCurrent(isolate)->current_async_id();
 }
 
-async_uid AsyncHooksGetCurrentId(Isolate* isolate) {
+async_id AsyncHooksGetCurrentId(Isolate* isolate) {
   return AsyncHooksGetExecutionAsyncId(isolate);
 }
 
 
-async_uid AsyncHooksGetTriggerAsyncId(Isolate* isolate) {
+async_id AsyncHooksGetTriggerAsyncId(Isolate* isolate) {
-  return Environment::GetCurrent(isolate)->get_init_trigger_id();
+  return Environment::GetCurrent(isolate)->trigger_id();
 }
 
-async_uid AsyncHooksGetTriggerId(Isolate* isolate) {
+async_id AsyncHooksGetTriggerId(Isolate* isolate) {
   return AsyncHooksGetTriggerAsyncId(isolate);
 }
 
 
-async_uid EmitAsyncInit(Isolate* isolate,
+async_context EmitAsyncInit(Isolate* isolate,
-                     Local<Object> resource,
+                            Local<Object> resource,
-                     const char* name,
+                            const char* name,
-                     async_uid trigger_id) {
+                            async_id trigger_async_id) {
   Environment* env = Environment::GetCurrent(isolate);
-  async_uid async_id = env->new_async_id();
 
+  // Initialize async context struct
+  if (trigger_async_id == -1)
+    trigger_async_id = env->get_init_trigger_id();
+
+  async_context context = {
+    env->new_async_id(),  // async_id_
+    trigger_async_id  // trigger_async_id_
+  };
+
+  // Run init hooks
   Local<String> type =
       String::NewFromUtf8(isolate, name, v8::NewStringType::kInternalized)
           .ToLocalChecked();
-  AsyncWrap::EmitAsyncInit(env, resource, type, async_id, trigger_id);
+  AsyncWrap::EmitAsyncInit(env, resource, type, context.async_id,
-  return async_id;
+                           context.trigger_async_id);
+
+  return context;
 }
 
-void EmitAsyncDestroy(Isolate* isolate, async_uid id) {
+void EmitAsyncDestroy(Isolate* isolate, async_context asyncContext) {
-  PushBackDestroyId(Environment::GetCurrent(isolate), id);
+  PushBackDestroyId(Environment::GetCurrent(isolate), asyncContext.async_id);
 }
 
 }  // namespace node

src/inspector_agent.cc

@@ -604,7 +604,7 @@ bool Agent::StartIoThread(bool wait_for_connect) {
     message
   };
   MakeCallback(parent_env_->isolate(), process_object, emit_fn.As<Function>(),
-               arraysize(argv), argv, 0, 0);
+               arraysize(argv), argv, {0, 0});
 
   return true;
 }

src/node.cc

@@ -363,7 +363,7 @@ static void CheckImmediate(uv_check_t* handle) {
                env->immediate_callback_string(),
                0,
                nullptr,
-               0, 0).ToLocalChecked();
+               {0, 0}).ToLocalChecked();
 }
 
 
@@ -1298,8 +1298,7 @@ MaybeLocal<Value> MakeCallback(Environment* env,
                                const Local<Function> callback,
                                int argc,
                                Local<Value> argv[],
-                               double async_id,
+                               async_context asyncContext) {
-                               double trigger_id) {
   // If you hit this assertion, you forgot to enter the v8::Context first.
   CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
 
@@ -1321,10 +1320,12 @@ MaybeLocal<Value> MakeCallback(Environment* env,
   MaybeLocal<Value> ret;
 
   {
-    AsyncHooks::ExecScope exec_scope(env, async_id, trigger_id);
+    AsyncHooks::ExecScope exec_scope(env, asyncContext.async_id,
+                                     asyncContext.trigger_async_id);
 
-    if (async_id != 0) {
+    if (asyncContext.async_id != 0) {
-      if (!AsyncWrap::EmitBefore(env, async_id)) return Local<Value>();
+      if (!AsyncWrap::EmitBefore(env, asyncContext.async_id))
+        return Local<Value>();
     }
 
     ret = callback->Call(env->context(), recv, argc, argv);
@@ -1336,8 +1337,9 @@ MaybeLocal<Value> MakeCallback(Environment* env,
           ret : Undefined(env->isolate());
     }
 
-    if (async_id != 0) {
+    if (asyncContext.async_id != 0) {
-      if (!AsyncWrap::EmitAfter(env, async_id)) return Local<Value>();
+      if (!AsyncWrap::EmitAfter(env, asyncContext.async_id))
+        return Local<Value>();
     }
   }
 
@@ -1358,8 +1360,8 @@ MaybeLocal<Value> MakeCallback(Environment* env,
 
   // Make sure the stack unwound properly. If there are nested MakeCallback's
   // then it should return early and not reach this code.
-  CHECK_EQ(env->current_async_id(), async_id);
+  CHECK_EQ(env->current_async_id(), asyncContext.async_id);
-  CHECK_EQ(env->trigger_id(), trigger_id);
+  CHECK_EQ(env->trigger_id(), asyncContext.trigger_async_id);
 
   Local<Object> process = env->process_object();
 
@@ -1384,13 +1386,11 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
                                const char* method,
                                int argc,
                                Local<Value> argv[],
-                               async_uid async_id,
+                               async_context asyncContext) {
-                               async_uid trigger_id) {
   Local<String> method_string =
       String::NewFromUtf8(isolate, method, v8::NewStringType::kNormal)
           .ToLocalChecked();
-  return MakeCallback(isolate, recv, method_string, argc, argv,
+  return MakeCallback(isolate, recv, method_string, argc, argv, asyncContext);
-                      async_id, trigger_id);
 }
 
 
@@ -1399,14 +1399,12 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
                                Local<String> symbol,
                                int argc,
                                Local<Value> argv[],
-                               async_uid async_id,
+                               async_context asyncContext) {
-                               async_uid trigger_id) {
   Local<Value> callback_v = recv->Get(symbol);
   if (callback_v.IsEmpty()) return Local<Value>();
   if (!callback_v->IsFunction()) return Local<Value>();
   Local<Function> callback = callback_v.As<Function>();
-  return MakeCallback(isolate, recv, callback, argc, argv,
+  return MakeCallback(isolate, recv, callback, argc, argv, asyncContext);
-                      async_id, trigger_id);
 }
 
 
@@ -1415,8 +1413,7 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
                                Local<Function> callback,
                                int argc,
                                Local<Value> argv[],
-                               async_uid async_id,
+                               async_context asyncContext) {
-                               async_uid trigger_id) {
   // Observe the following two subtleties:
   //
   // 1. The environment is retrieved from the callback function's context.
@@ -1427,7 +1424,7 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
   Environment* env = Environment::GetCurrent(callback->CreationContext());
   Context::Scope context_scope(env->context());
   return MakeCallback(env, recv.As<Value>(), callback, argc, argv,
-                      async_id, trigger_id);
+                      asyncContext);
 }
 
 
@@ -1440,7 +1437,7 @@ Local<Value> MakeCallback(Isolate* isolate,
                           Local<Value>* argv) {
   EscapableHandleScope handle_scope(isolate);
   return handle_scope.Escape(
-      MakeCallback(isolate, recv, method, argc, argv, 0, 0)
+      MakeCallback(isolate, recv, method, argc, argv, {0, 0})
           .FromMaybe(Local<Value>()));
 }
 
@@ -1452,7 +1449,7 @@ Local<Value> MakeCallback(Isolate* isolate,
     Local<Value>* argv) {
   EscapableHandleScope handle_scope(isolate);
   return handle_scope.Escape(
-      MakeCallback(isolate, recv, symbol, argc, argv, 0, 0)
+      MakeCallback(isolate, recv, symbol, argc, argv, {0, 0})
           .FromMaybe(Local<Value>()));
 }
 
@@ -1464,7 +1461,7 @@ Local<Value> MakeCallback(Isolate* isolate,
     Local<Value>* argv) {
   EscapableHandleScope handle_scope(isolate);
   return handle_scope.Escape(
-      MakeCallback(isolate, recv, callback, argc, argv, 0, 0)
+      MakeCallback(isolate, recv, callback, argc, argv, {0, 0})
           .FromMaybe(Local<Value>()));
 }
 
@@ -4426,7 +4423,7 @@ void EmitBeforeExit(Environment* env) {
   };
   MakeCallback(env->isolate(),
                process_object, "emit", arraysize(args), args,
-               0, 0).ToLocalChecked();
+               {0, 0}).ToLocalChecked();
 }
 
 
@@ -4447,7 +4444,7 @@ int EmitExit(Environment* env) {
 
   MakeCallback(env->isolate(),
                process_object, "emit", arraysize(args), args,
-               0, 0).ToLocalChecked();
+               {0, 0}).ToLocalChecked();
 
   // Reload exit code, it may be changed by `emit('exit')`
   return process_object->Get(exitCode)->Int32Value();

src/node.h

@@ -145,7 +145,7 @@ inline v8::Local<v8::Value> UVException(int errorno,
  * These methods need to be called in a HandleScope.
  *
  * It is preferred that you use the `MakeCallback` overloads taking
- * `async_uid` arguments.
+ * `async_id` arguments.
  */
 
 NODE_EXTERN v8::Local<v8::Value> MakeCallback(
@@ -517,7 +517,11 @@ typedef void (*promise_hook_func) (v8::PromiseHookType type,
                                    v8::Local<v8::Value> parent,
                                    void* arg);
 
-typedef double async_uid;
+typedef double async_id;
+struct async_context {
+  ::node::async_id async_id;
+  ::node::async_id trigger_async_id;
+};
 
 /* Registers an additional v8::PromiseHook wrapper. This API exists because V8
  * itself supports only a single PromiseHook. */
@@ -528,17 +532,17 @@ NODE_EXTERN void AddPromiseHook(v8::Isolate* isolate,
 /* Returns the id of the current execution context. If the return value is
  * zero then no execution has been set. This will happen if the user handles
  * I/O from native code. */
-NODE_EXTERN async_uid AsyncHooksGetExecutionAsyncId(v8::Isolate* isolate);
+NODE_EXTERN async_id AsyncHooksGetExecutionAsyncId(v8::Isolate* isolate);
 /* legacy alias */
 NODE_EXTERN NODE_DEPRECATED("Use AsyncHooksGetExecutionAsyncId(isolate)",
-                async_uid AsyncHooksGetCurrentId(v8::Isolate* isolate));
+                async_id AsyncHooksGetCurrentId(v8::Isolate* isolate));
 
 
 /* Return same value as async_hooks.triggerAsyncId(); */
-NODE_EXTERN async_uid AsyncHooksGetTriggerAsyncId(v8::Isolate* isolate);
+NODE_EXTERN async_id AsyncHooksGetTriggerAsyncId(v8::Isolate* isolate);
 /* legacy alias */
 NODE_EXTERN NODE_DEPRECATED("Use AsyncHooksGetTriggerAsyncId(isolate)",
-                async_uid AsyncHooksGetTriggerId(v8::Isolate* isolate));
+                async_id AsyncHooksGetTriggerId(v8::Isolate* isolate));
 
 
 /* If the native API doesn't inherit from the helper class then the callbacks
@@ -548,13 +552,14 @@ NODE_EXTERN NODE_DEPRECATED("Use AsyncHooksGetTriggerAsyncId(isolate)",
  * The `trigger_async_id` parameter should correspond to the resource which is
  * creating the new resource, which will usually be the return value of
  * `AsyncHooksGetTriggerAsyncId()`. */
-NODE_EXTERN async_uid EmitAsyncInit(v8::Isolate* isolate,
+NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate,
-                                    v8::Local<v8::Object> resource,
+                                        v8::Local<v8::Object> resource,
-                                    const char* name,
+                                        const char* name,
-                                    async_uid trigger_async_id);
+                                        async_id trigger_async_id = -1);
 
 /* Emit the destroy() callback. */
-NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate, async_uid id);
+NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate,
+                                  async_context asyncContext);
 
 /* An API specific to emit before/after callbacks is unnecessary because
  * MakeCallback will automatically call them for you.
@@ -572,24 +577,21 @@ v8::MaybeLocal<v8::Value> MakeCallback(v8::Isolate* isolate,
                                        v8::Local<v8::Function> callback,
                                        int argc,
                                        v8::Local<v8::Value>* argv,
-                                       async_uid asyncId,
+                                       async_context asyncContext);
-                                       async_uid triggerAsyncId);
 NODE_EXTERN
 v8::MaybeLocal<v8::Value> MakeCallback(v8::Isolate* isolate,
                                        v8::Local<v8::Object> recv,
                                        const char* method,
                                        int argc,
                                        v8::Local<v8::Value>* argv,
-                                       async_uid asyncId,
+                                       async_context asyncContext);
-                                       async_uid triggerAsyncId);
 NODE_EXTERN
 v8::MaybeLocal<v8::Value> MakeCallback(v8::Isolate* isolate,
                                        v8::Local<v8::Object> recv,
                                        v8::Local<v8::String> symbol,
                                        int argc,
                                        v8::Local<v8::Value>* argv,
-                                       async_uid asyncId,
+                                       async_context asyncContext);
-                                       async_uid triggerAsyncId);
 
 /* Helper class users can optionally inherit from. If
  * `AsyncResource::MakeCallback()` is used, then all four callbacks will be
@@ -599,18 +601,15 @@ class AsyncResource {
     AsyncResource(v8::Isolate* isolate,
                   v8::Local<v8::Object> resource,
                   const char* name,
-                  async_uid trigger_async_id = -1)
+                  async_id trigger_async_id = -1)
         : isolate_(isolate),
-          resource_(isolate, resource),
+          resource_(isolate, resource) {
-          trigger_async_id_(trigger_async_id) {
+      async_context_ = EmitAsyncInit(isolate, resource, name,
-      if (trigger_async_id_ == -1)
+                                     trigger_async_id);
-        trigger_async_id_ = AsyncHooksGetTriggerAsyncId(isolate);
-
-      uid_ = EmitAsyncInit(isolate, resource, name, trigger_async_id_);
     }
 
     ~AsyncResource() {
-      EmitAsyncDestroy(isolate_, uid_);
+      EmitAsyncDestroy(isolate_, async_context_);
     }
 
     v8::MaybeLocal<v8::Value> MakeCallback(
@@ -619,7 +618,7 @@ class AsyncResource {
         v8::Local<v8::Value>* argv) {
       return node::MakeCallback(isolate_, get_resource(),
                                 callback, argc, argv,
-                                uid_, trigger_async_id_);
+                                async_context_);
     }
 
     v8::MaybeLocal<v8::Value> MakeCallback(
@@ -628,7 +627,7 @@ class AsyncResource {
         v8::Local<v8::Value>* argv) {
       return node::MakeCallback(isolate_, get_resource(),
                                 method, argc, argv,
-                                uid_, trigger_async_id_);
+                                async_context_);
     }
 
     v8::MaybeLocal<v8::Value> MakeCallback(
@@ -637,21 +636,30 @@ class AsyncResource {
         v8::Local<v8::Value>* argv) {
       return node::MakeCallback(isolate_, get_resource(),
                                 symbol, argc, argv,
-                                uid_, trigger_async_id_);
+                                async_context_);
     }
 
     v8::Local<v8::Object> get_resource() {
       return resource_.Get(isolate_);
     }
 
-    async_uid get_uid() const {
+    NODE_DEPRECATED("Use AsyncResource::get_async_id()",
-      return uid_;
+      async_id get_uid() const {
+        return get_async_id();
+      }
+    )
+
+    async_id get_async_id() const {
+      return async_context_.async_id;
+    }
+
+    async_id get_trigger_async_id() const {
+      return async_context_.trigger_async_id;
     }
   private:
     v8::Isolate* isolate_;
     v8::Persistent<v8::Object> resource_;
-    async_uid uid_;
+    async_context async_context_;
-    async_uid trigger_async_id_;
 };
 
 }  // namespace node

src/node_crypto.cc

@@ -1237,7 +1237,7 @@ int SecureContext::TicketKeyCallback(SSL* ssl,
                                         env->ticketkeycallback_string(),
                                         arraysize(argv),
                                         argv,
-                                        0, 0).ToLocalChecked();
+                                        {0, 0}).ToLocalChecked();
   Local<Array> arr = ret.As<Array>();
 
   int r = arr->Get(kTicketKeyReturnIndex)->Int32Value();

test/addons/async-hooks-id/binding.cc

@@ -0,0 +1,27 @@
+#include "node.h"
+
+namespace {
+
+using v8::FunctionCallbackInfo;
+using v8::Local;
+using v8::Object;
+using v8::Value;
+
+void GetExecutionAsyncId(const FunctionCallbackInfo<Value>& args) {
+  args.GetReturnValue().Set(
+    node::AsyncHooksGetExecutionAsyncId(args.GetIsolate()));
+}
+
+void GetTriggerAsyncId(const FunctionCallbackInfo<Value>& args) {
+  args.GetReturnValue().Set(
+    node::AsyncHooksGetTriggerAsyncId(args.GetIsolate()));
+}
+
+void Initialize(Local<Object> exports) {
+  NODE_SET_METHOD(exports, "getExecutionAsyncId", GetExecutionAsyncId);
+  NODE_SET_METHOD(exports, "getTriggerAsyncId", GetTriggerAsyncId);
+}
+
+}  // namespace
+
+NODE_MODULE(binding, Initialize)

test/addons/async-hooks-id/binding.gyp

@@ -0,0 +1,9 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'defines': [ 'V8_DEPRECATION_WARNINGS=1' ],
+      'sources': [ 'binding.cc' ]
+    }
+  ]
+}

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

@@ -0,0 +1,26 @@
+'use strict';
+
+const common = require('../../common');
+const assert = require('assert');
+const binding = require(`./build/${common.buildType}/binding`);
+const async_hooks = require('async_hooks');
+
+assert.strictEqual(
+  binding.getExecutionAsyncId(),
+  async_hooks.executionAsyncId()
+);
+assert.strictEqual(
+  binding.getTriggerAsyncId(),
+  async_hooks.triggerAsyncId()
+);
+
+process.nextTick(common.mustCall(function() {
+  assert.strictEqual(
+    binding.getExecutionAsyncId(),
+    async_hooks.executionAsyncId()
+  );
+  assert.strictEqual(
+    binding.getTriggerAsyncId(),
+    async_hooks.triggerAsyncId()
+  );
+}));

test/addons/async-resource/binding.cc

@@ -80,10 +80,16 @@ void CallViaUtf8Name(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(ret.FromMaybe(Local<Value>()));
 }
 
-void GetUid(const FunctionCallbackInfo<Value>& args) {
+void GetAsyncId(const FunctionCallbackInfo<Value>& args) {
   assert(args[0]->IsExternal());
   auto r = static_cast<AsyncResource*>(args[0].As<External>()->Value());
-  args.GetReturnValue().Set(r->get_uid());
+  args.GetReturnValue().Set(r->get_async_id());
+}
+
+void GetTriggerAsyncId(const FunctionCallbackInfo<Value>& args) {
+  assert(args[0]->IsExternal());
+  auto r = static_cast<AsyncResource*>(args[0].As<External>()->Value());
+  args.GetReturnValue().Set(r->get_trigger_async_id());
 }
 
 void GetResource(const FunctionCallbackInfo<Value>& args) {
@@ -92,20 +98,15 @@ void GetResource(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(r->get_resource());
 }
 
-void GetCurrentId(const FunctionCallbackInfo<Value>& args) {
-  args.GetReturnValue().Set(
-    node::AsyncHooksGetExecutionAsyncId(args.GetIsolate()));
-}
-
 void Initialize(Local<Object> exports) {
   NODE_SET_METHOD(exports, "createAsyncResource", CreateAsyncResource);
   NODE_SET_METHOD(exports, "destroyAsyncResource", DestroyAsyncResource);
   NODE_SET_METHOD(exports, "callViaFunction", CallViaFunction);
   NODE_SET_METHOD(exports, "callViaString", CallViaString);
   NODE_SET_METHOD(exports, "callViaUtf8Name", CallViaUtf8Name);
-  NODE_SET_METHOD(exports, "getUid", GetUid);
+  NODE_SET_METHOD(exports, "getAsyncId", GetAsyncId);
+  NODE_SET_METHOD(exports, "getTriggerAsyncId", GetTriggerAsyncId);
   NODE_SET_METHOD(exports, "getResource", GetResource);
-  NODE_SET_METHOD(exports, "getCurrentId", GetCurrentId);
 }
 
 }  // namespace

test/addons/async-resource/test.js

@@ -6,6 +6,7 @@ const binding = require(`./build/${common.buildType}/binding`);
 const async_hooks = require('async_hooks');
 
 const kObjectTag = Symbol('kObjectTag');
+const rootAsyncId = async_hooks.executionAsyncId();
 
 const bindingUids = [];
 let expectedTriggerId;
@@ -38,8 +39,6 @@ async_hooks.createHook({
   }
 }).enable();
 
-assert.strictEqual(binding.getCurrentId(), 1);
-
 for (const call of [binding.callViaFunction,
                     binding.callViaString,
                     binding.callViaUtf8Name]) {
@@ -49,14 +48,14 @@ for (const call of [binding.callViaFunction,
       methöd(arg) {
         assert.strictEqual(this, object);
         assert.strictEqual(arg, 42);
-        assert.strictEqual(binding.getCurrentId(), uid);
+        assert.strictEqual(async_hooks.executionAsyncId(), uid);
         return 'baz';
       },
       kObjectTag
     };
 
     if (passedTriggerId === undefined)
-      expectedTriggerId = 1;
+      expectedTriggerId = rootAsyncId;
     else
       expectedTriggerId = passedTriggerId;
 
@@ -66,7 +65,8 @@ for (const call of [binding.callViaFunction,
     const ret = call(resource);
     assert.strictEqual(ret, 'baz');
     assert.strictEqual(binding.getResource(resource), object);
-    assert.strictEqual(binding.getUid(resource), uid);
+    assert.strictEqual(binding.getAsyncId(resource), uid);
+    assert.strictEqual(binding.getTriggerAsyncId(resource), expectedTriggerId);
 
     binding.destroyAsyncResource(resource);
   }