From eeede3b19c8bdb78605764eec75bea327c9014ff Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 19 Jan 2018 15:42:59 -0500 Subject: [PATCH] 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 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- lib/domain.js | 12 +++- node.gyp | 1 + src/env-inl.h | 9 --- src/env.h | 7 +-- src/node.cc | 78 +++--------------------- src/node_domain.cc | 34 +++++++++++ src/node_internals.h | 1 + test/addons/repl-domain-abort/binding.cc | 16 +++-- test/addons/repl-domain-abort/test.js | 3 +- test/cctest/node_module_reg.cc | 1 + 10 files changed, 72 insertions(+), 90 deletions(-) create mode 100644 src/node_domain.cc diff --git a/lib/domain.js b/lib/domain.js index 9670a53629e16b..08fbd207f171d3 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 80418b3a8a6f70..36efe80f7a2107 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 2a004f58153c21..202393f679e219 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 a6fe29b79b964a..b24ada5740dd7b 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 6eb7696a6e9846..c9af7c7dcd1efc 100644 --- a/src/node.cc +++ b/src/node.cc @@ -790,62 +790,6 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) { } -Local GetDomainProperty(Environment* env, Local object) { - Local 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) { - Local domain_v = GetDomainProperty(env, object); - if (domain_v->IsObject()) { - Local domain = domain_v.As(); - Local enter_v = domain->Get(env->enter_string()); - if (enter_v->IsFunction()) { - if (enter_v.As()->Call(domain, 0, nullptr).IsEmpty()) { - FatalError("node::AsyncWrap::MakeCallback", - "domain enter callback threw, please report this"); - } - } - } -} - - -void DomainExit(Environment* env, v8::Local object) { - Local domain_v = GetDomainProperty(env, object); - if (domain_v->IsObject()) { - Local domain = domain_v.As(); - Local exit_v = domain->Get(env->exit_string()); - if (exit_v->IsFunction()) { - if (exit_v.As()->Call(domain, 0, nullptr).IsEmpty()) { - FatalError("node::AsyncWrap::MakeCallback", - "domain exit callback threw, please report this"); - } - } - } -} - -void SetupDomainUse(const FunctionCallbackInfo& 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& 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 InternalMakeCallback(Environment* env, return Undefined(env->isolate()); } - MaybeLocal ret = callback->Call(env->context(), recv, argc, argv); + Local domain_cb = env->domain_callback(); + MaybeLocal ret; + if (asyncContext.async_id != 0 || domain_cb.IsEmpty() || recv.IsEmpty()) { + ret = callback->Call(env->context(), recv, argc, argv); + } else { + std::vector> 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 00000000000000..f4f585ac4f43e2 --- /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& args) { + Environment* env = Environment::GetCurrent(args); + + CHECK(args[0]->IsFunction()); + + env->set_domain_callback(args[0].As()); +} + +void Initialize(Local target, + Local unused, + Local 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 0f693527cceeb1..0001d15172f39e 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 1b4dbfa84e5054..d2f7560048fd46 100644 --- a/test/addons/repl-domain-abort/binding.cc +++ b/test/addons/repl-domain-abort/binding.cc @@ -22,6 +22,7 @@ #include #include +using v8::Boolean; using v8::Function; using v8::FunctionCallbackInfo; using v8::Local; @@ -31,11 +32,16 @@ using v8::Value; void Method(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); - node::MakeCallback(isolate, - isolate->GetCurrentContext()->Global(), - args[0].As(), - 0, - nullptr); + Local params[] = { + Boolean::New(isolate, true), + Boolean::New(isolate, false) + }; + Local ret = node::MakeCallback(isolate, + isolate->GetCurrentContext()->Global(), + args[0].As(), + 2, + params); + assert(ret->IsTrue()); } void init(Local exports) { diff --git a/test/addons/repl-domain-abort/test.js b/test/addons/repl-domain-abort/test.js index 1d6116159c85b6..2049fe6e6a23f5 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 a0736d2cc3e692..bd4f20bc9f823d 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() {}