domain: further abstract usage in C++

Move the majority of C++ domain-related code into JS land by introducing
a top level domain callback which handles entering & exiting the domain.

Move the rest of the domain necessities into their own file that creates
an internal binding, to avoid exposing domain-related code on the
process object.

Modify an existing test slightly to better test domain-related code.

PR-URL: https://github.com/nodejs/node/pull/18291
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

10 files changed, 72 insertions, 90 deletions

diff --git a/lib/domain.js b/lib/domain.js
index 9670a53629..08fbd207f1 100644
--- a/lib/domain.js
+++ b/lib/domain.js
@@ -94,11 +94,21 @@ process.setUncaughtExceptionCaptureCallback = function(fn) {
   throw err;
 };
 
+function topLevelDomainCallback(cb, ...args) {
+  const domain = this.domain;
+  if (domain)
+    domain.enter();
+  const ret = Reflect.apply(cb, this, args);
+  if (domain)
+    domain.exit();
+  return ret;
+}
+
 // It's possible to enter one domain while already inside
 // another one. The stack is each entered domain.
 const stack = [];
 exports._stack = stack;
-process._setupDomainUse();
+internalBinding('domain').enable(topLevelDomainCallback);
 
 function updateExceptionCapture() {
   if (stack.every((domain) => domain.listenerCount('error') === 0)) {
diff --git a/node.gyp b/node.gyp
index 80418b3a8a..36efe80f7a 100644
--- a/node.gyp
+++ b/node.gyp
@@ -300,6 +300,7 @@
         'src/node_constants.cc',
         'src/node_contextify.cc',
         'src/node_debug_options.cc',
+        'src/node_domain.cc',
         'src/node_file.cc',
         'src/node_http2.cc',
         'src/node_http_parser.cc',
diff --git a/src/env-inl.h b/src/env-inl.h
index 2a004f5815..202393f679 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -310,7 +310,6 @@ inline Environment::Environment(IsolateData* isolate_data,
       immediate_info_(context->GetIsolate()),
       tick_info_(context->GetIsolate()),
       timer_base_(uv_now(isolate_data->event_loop())),
-      using_domains_(false),
       printed_error_(false),
       trace_sync_io_(false),
       abort_on_uncaught_exception_(false),
@@ -426,14 +425,6 @@ inline uint64_t Environment::timer_base() const {
   return timer_base_;
 }
 
-inline bool Environment::using_domains() const {
-  return using_domains_;
-}
-
-inline void Environment::set_using_domains(bool value) {
-  using_domains_ = value;
-}
-
 inline bool Environment::printed_error() const {
   return printed_error_;
 }
diff --git a/src/env.h b/src/env.h
index a6fe29b79b..b24ada5740 100644
--- a/src/env.h
+++ b/src/env.h
@@ -91,7 +91,6 @@ class ModuleWrap;
   V(decorated_private_symbol, "node:decorated")                               \
   V(npn_buffer_private_symbol, "node:npnBuffer")                              \
   V(selected_npn_buffer_private_symbol, "node:selectedNpnBuffer")             \
-  V(domain_private_symbol, "node:domain")                                     \
 
 // Strings are per-isolate primitives but Environment proxies them
 // for the sake of convenience.  Strings should be ASCII-only.
@@ -128,7 +127,6 @@ class ModuleWrap;
   V(dns_soa_string, "SOA")                                                    \
   V(dns_srv_string, "SRV")                                                    \
   V(dns_txt_string, "TXT")                                                    \
-  V(domain_string, "domain")                                                  \
   V(emit_warning_string, "emitWarning")                                       \
   V(exchange_string, "exchange")                                              \
   V(encoding_string, "encoding")                                              \
@@ -280,6 +278,7 @@ class ModuleWrap;
   V(internal_binding_cache_object, v8::Object)                                \
   V(buffer_prototype_object, v8::Object)                                      \
   V(context, v8::Context)                                                     \
+  V(domain_callback, v8::Function)                                            \
   V(host_import_module_dynamically_callback, v8::Function)                    \
   V(http2ping_constructor_template, v8::ObjectTemplate)                       \
   V(http2stream_constructor_template, v8::ObjectTemplate)                     \
@@ -567,9 +566,6 @@ class Environment {
 
   inline IsolateData* isolate_data() const;
 
-  inline bool using_domains() const;
-  inline void set_using_domains(bool value);
-
   inline bool printed_error() const;
   inline void set_printed_error(bool value);
 
@@ -745,7 +741,6 @@ class Environment {
   ImmediateInfo immediate_info_;
   TickInfo tick_info_;
   const uint64_t timer_base_;
-  bool using_domains_;
   bool printed_error_;
   bool trace_sync_io_;
   bool abort_on_uncaught_exception_;
diff --git a/src/node.cc b/src/node.cc
index 6eb7696a6e..c9af7c7dcd 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -790,62 +790,6 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) {
 }
 
 
-Local<Value> GetDomainProperty(Environment* env, Local<Object> object) {
-  Local<Value> domain_v =
-      object->GetPrivate(env->context(), env->domain_private_symbol())
-          .ToLocalChecked();
-  if (domain_v->IsObject()) {
-    return domain_v;
-  }
-  return object->Get(env->context(), env->domain_string()).ToLocalChecked();
-}
-
-
-void DomainEnter(Environment* env, Local<Object> object) {
-  Local<Value> domain_v = GetDomainProperty(env, object);
-  if (domain_v->IsObject()) {
-    Local<Object> domain = domain_v.As<Object>();
-    Local<Value> enter_v = domain->Get(env->enter_string());
-    if (enter_v->IsFunction()) {
-      if (enter_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
-        FatalError("node::AsyncWrap::MakeCallback",
-                   "domain enter callback threw, please report this");
-      }
-    }
-  }
-}
-
-
-void DomainExit(Environment* env, v8::Local<v8::Object> object) {
-  Local<Value> domain_v = GetDomainProperty(env, object);
-  if (domain_v->IsObject()) {
-    Local<Object> domain = domain_v.As<Object>();
-    Local<Value> exit_v = domain->Get(env->exit_string());
-    if (exit_v->IsFunction()) {
-      if (exit_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
-        FatalError("node::AsyncWrap::MakeCallback",
-                  "domain exit callback threw, please report this");
-      }
-    }
-  }
-}
-
-void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
-
-  if (env->using_domains())
-    return;
-  env->set_using_domains(true);
-
-  HandleScope scope(env->isolate());
-
-  // Do a little housekeeping.
-  env->process_object()->Delete(
-      env->context(),
-      FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupDomainUse")).FromJust();
-}
-
-
 void RunMicrotasks(const FunctionCallbackInfo<Value>& args) {
   args.GetIsolate()->RunMicrotasks();
 }
@@ -982,11 +926,6 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
   // If you hit this assertion, you forgot to enter the v8::Context first.
   CHECK_EQ(Environment::GetCurrent(env->isolate()), env);
 
-  if (asyncContext.async_id == 0 && env->using_domains() &&
-      !object_.IsEmpty()) {
-    DomainEnter(env, object_);
-  }
-
   if (asyncContext.async_id != 0) {
     // No need to check a return value because the application will exit if
     // an exception occurs.
@@ -1016,11 +955,6 @@ void InternalCallbackScope::Close() {
     AsyncWrap::EmitAfter(env_, async_context_.async_id);
   }
 
-  if (async_context_.async_id == 0 && env_->using_domains() &&
-      !object_.IsEmpty()) {
-    DomainExit(env_, object_);
-  }
-
   if (IsInnerMakeCallback()) {
     return;
   }
@@ -1061,7 +995,16 @@ MaybeLocal<Value> InternalMakeCallback(Environment* env,
     return Undefined(env->isolate());
   }
 
-  MaybeLocal<Value> ret = callback->Call(env->context(), recv, argc, argv);
+  Local<Function> domain_cb = env->domain_callback();
+  MaybeLocal<Value> ret;
+  if (asyncContext.async_id != 0 || domain_cb.IsEmpty() || recv.IsEmpty()) {
+    ret = callback->Call(env->context(), recv, argc, argv);
+  } else {
+    std::vector<Local<Value>> args(1 + argc);
+    args[0] = callback;
+    std::copy(&argv[0], &argv[argc], &args[1]);
+    ret = domain_cb->Call(env->context(), recv, args.size(), &args[0]);
+  }
 
   if (ret.IsEmpty()) {
     // NOTE: For backwards compatibility with public API we return Undefined()
@@ -3339,7 +3282,6 @@ void SetupProcessObject(Environment* env,
   env->SetMethod(process, "_setupProcessObject", SetupProcessObject);
   env->SetMethod(process, "_setupNextTick", SetupNextTick);
   env->SetMethod(process, "_setupPromises", SetupPromises);
-  env->SetMethod(process, "_setupDomainUse", SetupDomainUse);
 }
 
 
diff --git a/src/node_domain.cc b/src/node_domain.cc
new file mode 100644
index 0000000000..f4f585ac4f
--- /dev/null
+++ b/src/node_domain.cc
@@ -0,0 +1,34 @@
+#include "v8.h"
+#include "node_internals.h"
+
+namespace node {
+namespace domain {
+
+using v8::Context;
+using v8::Function;
+using v8::FunctionCallbackInfo;
+using v8::Local;
+using v8::Object;
+using v8::Value;
+
+
+void Enable(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+
+  CHECK(args[0]->IsFunction());
+
+  env->set_domain_callback(args[0].As<Function>());
+}
+
+void Initialize(Local<Object> target,
+                Local<Value> unused,
+                Local<Context> context) {
+  Environment* env = Environment::GetCurrent(context);
+
+  env->SetMethod(target, "enable", Enable);
+}
+
+}  // namespace domain
+}  // namespace node
+
+NODE_MODULE_CONTEXT_AWARE_INTERNAL(domain, node::domain::Initialize)
diff --git a/src/node_internals.h b/src/node_internals.h
index 0f693527cc..0001d15172 100644
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -104,6 +104,7 @@ struct sockaddr;
     V(cares_wrap)                                                             \
     V(config)                                                                 \
     V(contextify)                                                             \
+    V(domain)                                                                 \
     V(fs)                                                                     \
     V(fs_event_wrap)                                                          \
     V(http2)                                                                  \
diff --git a/test/addons/repl-domain-abort/binding.cc b/test/addons/repl-domain-abort/binding.cc
index 1b4dbfa84e..d2f7560048 100644
--- a/test/addons/repl-domain-abort/binding.cc
+++ b/test/addons/repl-domain-abort/binding.cc
@@ -22,6 +22,7 @@
 #include <node.h>
 #include <v8.h>
 
+using v8::Boolean;
 using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::Local;
@@ -31,11 +32,16 @@ using v8::Value;
 
 void Method(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = args.GetIsolate();
-  node::MakeCallback(isolate,
-                     isolate->GetCurrentContext()->Global(),
-                     args[0].As<Function>(),
-                     0,
-                     nullptr);
+  Local<Value> params[] = {
+    Boolean::New(isolate, true),
+    Boolean::New(isolate, false)
+  };
+  Local<Value> ret = node::MakeCallback(isolate,
+                                        isolate->GetCurrentContext()->Global(),
+                                        args[0].As<Function>(),
+                                        2,
+                                        params);
+  assert(ret->IsTrue());
 }
 
 void init(Local<Object> exports) {
diff --git a/test/addons/repl-domain-abort/test.js b/test/addons/repl-domain-abort/test.js
index 1d6116159c..2049fe6e6a 100644
--- a/test/addons/repl-domain-abort/test.js
+++ b/test/addons/repl-domain-abort/test.js
@@ -40,7 +40,8 @@ const lines = [
   // This line shouldn't cause an assertion error.
   `require('${buildPath}')` +
   // Log output to double check callback ran.
-  '.method(function() { console.log(\'cb_ran\'); });',
+  '.method(function(v1, v2) {' +
+  'console.log(\'cb_ran\'); return v1 === true && v2 === false; });',
 ];
 
 const dInput = new stream.Readable();
diff --git a/test/cctest/node_module_reg.cc b/test/cctest/node_module_reg.cc
index a0736d2cc3..bd4f20bc9f 100644
--- a/test/cctest/node_module_reg.cc
+++ b/test/cctest/node_module_reg.cc
@@ -5,6 +5,7 @@
 void _register_cares_wrap() {}
 void _register_config() {}
 void _register_contextify() {}
+void _register_domain() {}
 void _register_fs() {}
 void _register_fs_event_wrap() {}
 void _register_http2() {}