src: refactor async callback handling

- Merge the two almost-but-not-quite identical `MakeCallback()`
  implementations
- Provide a public `CallbackScope` class for embedders in order
  to enable `MakeCallback()`-like behaviour without tying that
  to calling a JS function

PR-URL: https://github.com/nodejs/node/pull/14697
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

src/async-wrap.cc

@@ -657,73 +657,8 @@ void AsyncWrap::EmitAsyncInit(Environment* env,
 MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
                                           int argc,
                                           Local<Value>* argv) {
-  CHECK(env()->context() == env()->isolate()->GetCurrentContext());
-
-  Environment::AsyncCallbackScope callback_scope(env());
-
-  Environment::AsyncHooks::ExecScope exec_scope(env(),
-                                                get_id(),
-                                                get_trigger_id());
-
-  // Return v8::Undefined() because returning an empty handle will cause
-  // ToLocalChecked() to abort.
-  if (env()->using_domains() && DomainEnter(env(), object())) {
-    return Undefined(env()->isolate());
-  }
-
-  // No need to check a return value because the application will exit if an
-  // exception occurs.
-  AsyncWrap::EmitBefore(env(), get_id());
-
-  MaybeLocal<Value> ret = cb->Call(env()->context(), object(), argc, argv);
-
-  if (ret.IsEmpty()) {
-    return ret;
-  }
-
-  AsyncWrap::EmitAfter(env(), get_id());
-
-  // Return v8::Undefined() because returning an empty handle will cause
-  // ToLocalChecked() to abort.
-  if (env()->using_domains() && DomainExit(env(), object())) {
-    return Undefined(env()->isolate());
-  }
-
-  exec_scope.Dispose();
-
-  if (callback_scope.in_makecallback()) {
-    return ret;
-  }
-
-  Environment::TickInfo* tick_info = env()->tick_info();
-
-  if (tick_info->length() == 0) {
-    env()->isolate()->RunMicrotasks();
-  }
-
-  // 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(), 0);
-  CHECK_EQ(env()->trigger_id(), 0);
-
-  Local<Object> process = env()->process_object();
-
-  if (tick_info->length() == 0) {
-    tick_info->set_index(0);
-    return ret;
-  }
-
-  MaybeLocal<Value> rcheck =
-      env()->tick_callback_function()->Call(env()->context(),
-                                            process,
-                                            0,
-                                            nullptr);
-
-  // Make sure the stack unwound properly.
-  CHECK_EQ(env()->current_async_id(), 0);
-  CHECK_EQ(env()->trigger_id(), 0);
-
-  return rcheck.IsEmpty() ? MaybeLocal<Value>() : ret;
+  async_context context { get_id(), get_trigger_id() };
+  return InternalMakeCallback(env(), object(), cb, argc, argv, context);
 }
 
 

src/node.cc

@@ -161,6 +161,7 @@ using v8::SealHandleScope;
 using v8::String;
 using v8::TryCatch;
 using v8::Uint32Array;
+using v8::Undefined;
 using v8::V8;
 using v8::Value;
 
@@ -1311,85 +1312,153 @@ void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) {
   env->AddPromiseHook(fn, arg);
 }
 
+class InternalCallbackScope {
+ public:
+  InternalCallbackScope(Environment* env,
+                        Local<Object> object,
+                        const async_context& asyncContext);
+  ~InternalCallbackScope();
+  void Close();
+
+  inline bool Failed() const { return failed_; }
+  inline void MarkAsFailed() { failed_ = true; }
+  inline bool IsInnerMakeCallback() const {
+    return callback_scope_.in_makecallback();
+  }
+
+ private:
+  Environment* env_;
+  async_context async_context_;
+  v8::Local<v8::Object> object_;
+  Environment::AsyncCallbackScope callback_scope_;
+  bool failed_ = false;
+  bool pushed_ids_ = false;
+  bool closed_ = false;
+};
+
+CallbackScope::CallbackScope(Isolate* isolate,
+                             Local<Object> object,
+                             async_context asyncContext)
+  : private_(new InternalCallbackScope(Environment::GetCurrent(isolate),
+                                       object,
+                                       asyncContext)),
+    try_catch_(isolate) {
+  try_catch_.SetVerbose(true);
+}
+
+CallbackScope::~CallbackScope() {
+  if (try_catch_.HasCaught())
+    private_->MarkAsFailed();
+  delete private_;
+}
+
+InternalCallbackScope::InternalCallbackScope(Environment* env,
+                                             Local<Object> object,
+                                             const async_context& asyncContext)
+  : env_(env),
+    async_context_(asyncContext),
+    object_(object),
+    callback_scope_(env) {
+  CHECK(!object.IsEmpty());
 
-MaybeLocal<Value> MakeCallback(Environment* env,
-                               Local<Value> recv,
-                               const Local<Function> callback,
-                               int argc,
-                               Local<Value> argv[],
-                               async_context asyncContext) {
   // If you hit this assertion, you forgot to enter the v8::Context first.
   CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
 
-  Local<Object> object;
-
-  Environment::AsyncCallbackScope callback_scope(env);
-  bool disposed_domain = false;
-
-  if (recv->IsObject()) {
-    object = recv.As<Object>();
+  if (env->using_domains()) {
+    failed_ = DomainEnter(env, object_);
+    if (failed_)
+      return;
   }
 
-  if (env->using_domains()) {
-    CHECK(recv->IsObject());
-    disposed_domain = DomainEnter(env, object);
-    if (disposed_domain) return Undefined(env->isolate());
+  if (asyncContext.async_id != 0) {
+    // No need to check a return value because the application will exit if
+    // an exception occurs.
+    AsyncWrap::EmitBefore(env, asyncContext.async_id);
+  }
+
+  env->async_hooks()->push_ids(async_context_.async_id,
+                               async_context_.trigger_async_id);
+  pushed_ids_ = true;
+}
+
+InternalCallbackScope::~InternalCallbackScope() {
+  Close();
+}
+
+void InternalCallbackScope::Close() {
+  if (closed_) return;
+  closed_ = true;
+
+  if (pushed_ids_)
+    env_->async_hooks()->pop_ids(async_context_.async_id);
+
+  if (failed_) return;
+
+  if (async_context_.async_id != 0) {
+    AsyncWrap::EmitAfter(env_, async_context_.async_id);
+  }
+
+  if (env_->using_domains()) {
+    failed_ = DomainExit(env_, object_);
+    if (failed_) return;
+  }
+
+  if (IsInnerMakeCallback()) {
+    return;
+  }
+
+  Environment::TickInfo* tick_info = env_->tick_info();
+
+  if (tick_info->length() == 0) {
+    env_->isolate()->RunMicrotasks();
+  }
+
+  // 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(), 0);
+  CHECK_EQ(env_->trigger_id(), 0);
+
+  Local<Object> process = env_->process_object();
+
+  if (tick_info->length() == 0) {
+    tick_info->set_index(0);
+    return;
+  }
+
+  CHECK_EQ(env_->current_async_id(), 0);
+  CHECK_EQ(env_->trigger_id(), 0);
+
+  if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
+    failed_ = true;
+  }
+}
+
+MaybeLocal<Value> InternalMakeCallback(Environment* env,
+                                       Local<Object> recv,
+                                       const Local<Function> callback,
+                                       int argc,
+                                       Local<Value> argv[],
+                                       async_context asyncContext) {
+  InternalCallbackScope scope(env, recv, asyncContext);
+  if (scope.Failed()) {
+    return Undefined(env->isolate());
   }
 
   MaybeLocal<Value> ret;
 
   {
-    AsyncHooks::ExecScope exec_scope(env, asyncContext.async_id,
-                                     asyncContext.trigger_async_id);
-
-    if (asyncContext.async_id != 0) {
-      // No need to check a return value because the application will exit if
-      // an exception occurs.
-      AsyncWrap::EmitBefore(env, asyncContext.async_id);
-    }
-
     ret = callback->Call(env->context(), recv, argc, argv);
 
     if (ret.IsEmpty()) {
       // NOTE: For backwards compatibility with public API we return Undefined()
       // if the top level call threw.
-      return callback_scope.in_makecallback() ?
-          ret : Undefined(env->isolate());
-    }
-
-    if (asyncContext.async_id != 0) {
-      AsyncWrap::EmitAfter(env, asyncContext.async_id);
+      scope.MarkAsFailed();
+      return scope.IsInnerMakeCallback() ? ret : Undefined(env->isolate());
     }
   }
 
-  if (env->using_domains()) {
-    disposed_domain = DomainExit(env, object);
-    if (disposed_domain) return Undefined(env->isolate());
-  }
-
-  if (callback_scope.in_makecallback()) {
-    return ret;
-  }
-
-  Environment::TickInfo* tick_info = env->tick_info();
-
-  if (tick_info->length() == 0) {
-    env->isolate()->RunMicrotasks();
-  }
-
-  // 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(), asyncContext.async_id);
-  CHECK_EQ(env->trigger_id(), asyncContext.trigger_async_id);
-
-  Local<Object> process = env->process_object();
-
-  if (tick_info->length() == 0) {
-    tick_info->set_index(0);
-    return ret;
-  }
-
-  if (env->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
+  scope.Close();
+  if (scope.Failed()) {
     return Undefined(env->isolate());
   }
 
@@ -1442,8 +1511,8 @@ MaybeLocal<Value> MakeCallback(Isolate* isolate,
   // the two contexts need not be the same.
   Environment* env = Environment::GetCurrent(callback->CreationContext());
   Context::Scope context_scope(env->context());
-  return MakeCallback(env, recv.As<Value>(), callback, argc, argv,
-                      asyncContext);
+  return InternalMakeCallback(env, recv, callback,
+                              argc, argv, asyncContext);
 }
 
 

src/node.h

@@ -570,6 +570,36 @@ NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate,
 NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate,
                                   async_context asyncContext);
 
+class InternalCallbackScope;
+
+/* This class works like `MakeCallback()` in that it sets up a specific
+ * asyncContext as the current one and informs the async_hooks and domains
+ * modules that this context is currently active.
+ *
+ * `MakeCallback()` is a wrapper around this class as well as
+ * `Function::Call()`. Either one of these mechanisms needs to be used for
+ * top-level calls into JavaScript (i.e. without any existing JS stack).
+ *
+ * This object should be stack-allocated to ensure that it is contained in a
+ * valid HandleScope.
+ */
+class NODE_EXTERN CallbackScope {
+ public:
+  CallbackScope(v8::Isolate* isolate,
+                v8::Local<v8::Object> resource,
+                async_context asyncContext);
+  ~CallbackScope();
+
+ private:
+  InternalCallbackScope* private_;
+  v8::TryCatch try_catch_;
+
+  void operator=(const CallbackScope&) = delete;
+  void operator=(CallbackScope&&) = delete;
+  CallbackScope(const CallbackScope&) = delete;
+  CallbackScope(CallbackScope&&) = delete;
+};
+
 /* An API specific to emit before/after callbacks is unnecessary because
  * MakeCallback will automatically call them for you.
  *
@@ -659,6 +689,16 @@ class AsyncResource {
     async_id get_trigger_async_id() const {
       return async_context_.trigger_async_id;
     }
+
+  protected:
+    class CallbackScope : public node::CallbackScope {
+     public:
+      explicit CallbackScope(AsyncResource* res)
+        : node::CallbackScope(res->isolate_,
+                              res->resource_.Get(res->isolate_),
+                              res->async_context_) {}
+    };
+
   private:
     v8::Isolate* isolate_;
     v8::Persistent<v8::Object> resource_;

src/node_internals.h

@@ -286,6 +286,14 @@ static v8::MaybeLocal<v8::Object> New(Environment* env,
 }
 }  // namespace Buffer
 
+v8::MaybeLocal<v8::Value> InternalMakeCallback(
+    Environment* env,
+    v8::Local<v8::Object> recv,
+    const v8::Local<v8::Function> callback,
+    int argc,
+    v8::Local<v8::Value> argv[],
+    async_context asyncContext);
+
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

test/addons/callback-scope/binding.cc

@@ -0,0 +1,39 @@
+#include "node.h"
+#include "v8.h"
+
+#include <assert.h>
+#include <vector>
+
+namespace {
+
+void RunInCallbackScope(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  v8::Isolate* isolate = args.GetIsolate();
+
+  assert(args.Length() == 4);
+  assert(args[0]->IsObject());
+  assert(args[1]->IsNumber());
+  assert(args[2]->IsNumber());
+  assert(args[3]->IsFunction());
+
+  node::async_context asyncContext = {
+    args[1].As<v8::Number>()->Value(),
+    args[2].As<v8::Number>()->Value()
+  };
+
+  node::CallbackScope scope(isolate, args[0].As<v8::Object>(), asyncContext);
+  v8::Local<v8::Function> fn = args[3].As<v8::Function>();
+
+  v8::MaybeLocal<v8::Value> ret =
+      fn->Call(isolate->GetCurrentContext(), args[0], 0, nullptr);
+
+  if (!ret.IsEmpty())
+    args.GetReturnValue().Set(ret.ToLocalChecked());
+}
+
+void Initialize(v8::Local<v8::Object> exports) {
+  NODE_SET_METHOD(exports, "runInCallbackScope", RunInCallbackScope);
+}
+
+}  // namespace
+
+NODE_MODULE(binding, Initialize)

test/addons/callback-scope/binding.gyp

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

test/addons/callback-scope/test-async-hooks.js

@@ -0,0 +1,22 @@
+'use strict';
+
+const common = require('../../common');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+const { runInCallbackScope } = require(`./build/${common.buildType}/binding`);
+
+let insideHook = false;
+async_hooks.createHook({
+  before: common.mustCall((id) => {
+    assert.strictEqual(id, 1000);
+    insideHook = true;
+  }),
+  after: common.mustCall((id) => {
+    assert.strictEqual(id, 1000);
+    insideHook = false;
+  })
+}).enable();
+
+runInCallbackScope({}, 1000, 1000, () => {
+  assert(insideHook);
+});

test/addons/callback-scope/test.js

@@ -0,0 +1,17 @@
+'use strict';
+
+const common = require('../../common');
+const assert = require('assert');
+const { runInCallbackScope } = require(`./build/${common.buildType}/binding`);
+
+assert.strictEqual(runInCallbackScope({}, 0, 0, () => 42), 42);
+
+{
+  process.once('uncaughtException', common.mustCall((err) => {
+    assert.strictEqual(err.message, 'foo');
+  }));
+
+  runInCallbackScope({}, 0, 0, () => {
+    throw new Error('foo');
+  });
+}