From fab0c982253b882583fc2360e9507fe81cdb2fb1 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sun, 21 Jan 2018 10:34:36 -0500 Subject: [PATCH 1/5] src: remove unnecessary block scope --- src/node.cc | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/node.cc b/src/node.cc index 656d132ec9fb32..dd6a44bbae3ee0 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1066,17 +1066,13 @@ MaybeLocal InternalMakeCallback(Environment* env, return Undefined(env->isolate()); } - MaybeLocal ret; + MaybeLocal ret = callback->Call(env->context(), recv, argc, argv); - { - 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. - scope.MarkAsFailed(); - return scope.IsInnerMakeCallback() ? ret : Undefined(env->isolate()); - } + if (ret.IsEmpty()) { + // NOTE: For backwards compatibility with public API we return Undefined() + // if the top level call threw. + scope.MarkAsFailed(); + return scope.IsInnerMakeCallback() ? ret : Undefined(env->isolate()); } scope.Close(); From 0d66284a9eae1398d2c978596917afd911057965 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sun, 21 Jan 2018 10:47:02 -0500 Subject: [PATCH 2/5] src: remove unnecessary async hooks check --- src/node.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/node.cc b/src/node.cc index dd6a44bbae3ee0..6eb7696a6e9846 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1042,11 +1042,6 @@ void InternalCallbackScope::Close() { return; } - if (env_->async_hooks()->fields()[AsyncHooks::kTotals]) { - CHECK_EQ(env_->execution_async_id(), 0); - CHECK_EQ(env_->trigger_async_id(), 0); - } - Local process = env_->process_object(); if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { From 9fdf29316c0c2f94347cfdda3f379eedf1628e96 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 19 Jan 2018 14:23:37 -0500 Subject: [PATCH 3/5] src: remove outdated domain reference --- src/env.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/env.h b/src/env.h index f5a11dca07a20b..ce89c37694e433 100644 --- a/src/env.h +++ b/src/env.h @@ -244,7 +244,6 @@ class ModuleWrap; V(subject_string, "subject") \ V(subjectaltname_string, "subjectaltname") \ V(syscall_string, "syscall") \ - V(tick_domain_cb_string, "_tickDomainCallback") \ V(ticketkeycallback_string, "onticketkeycallback") \ V(timeout_string, "timeout") \ V(tls_ticket_string, "tlsTicket") \ From c6cadb354e7f67b093a837c0c52da85687500141 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 19 Jan 2018 15:42:59 -0500 Subject: [PATCH 4/5] 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. --- lib/domain.js | 12 +++- node.gyp | 1 + src/env-inl.h | 9 --- src/env.h | 7 +-- src/node.cc | 80 ++++-------------------- src/node_domain.cc | 34 ++++++++++ src/node_internals.h | 1 + test/addons/repl-domain-abort/binding.cc | 11 ++-- test/addons/repl-domain-abort/test.js | 2 +- test/cctest/node_module_reg.cc | 1 + 10 files changed, 68 insertions(+), 90 deletions(-) create mode 100644 src/node_domain.cc diff --git a/lib/domain.js b/lib/domain.js index 9670a53629e16b..558b67dca7dcba 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 54bdb87669f176..d47479f375b96f 100644 --- a/node.gyp +++ b/node.gyp @@ -299,6 +299,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 910bba69f7569f..8f34a56d91b3f2 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), @@ -427,14 +426,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 ce89c37694e433..b3e42a5ddce8a0 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); @@ -746,7 +742,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..630adc7c389757 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,18 @@ 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 { + Local argv_array = Array::New(env->isolate(), argc); + for (int i = 0; i < argc; i++) { + argv_array->Set(env->context(), i, argv[i]).FromJust(); + } + Local domain_argv[] = { callback, argv_array }; + ret = domain_cb->Call(env->context(), recv, 2, domain_argv); + } if (ret.IsEmpty()) { // NOTE: For backwards compatibility with public API we return Undefined() @@ -3339,7 +3284,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 bb03d490f83067..8a19df0a80150b 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..7a04d0bcef8271 100644 --- a/test/addons/repl-domain-abort/binding.cc +++ b/test/addons/repl-domain-abort/binding.cc @@ -31,11 +31,12 @@ using v8::Value; void Method(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); - node::MakeCallback(isolate, - isolate->GetCurrentContext()->Global(), - args[0].As(), - 0, - nullptr); + Local ret = node::MakeCallback(isolate, + isolate->GetCurrentContext()->Global(), + args[0].As(), + 0, + nullptr); + 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..331d7aabb7b3e4 100644 --- a/test/addons/repl-domain-abort/test.js +++ b/test/addons/repl-domain-abort/test.js @@ -40,7 +40,7 @@ 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() { console.log(\'cb_ran\'); return true; });', ]; 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() {} From 930afebc766068b038c4e03e7de78c9fd4e25c37 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 22 Jan 2018 13:19:18 -0500 Subject: [PATCH 5/5] fixup: bnoordhuis suggestion + test adjustment --- lib/domain.js | 2 +- src/node.cc | 10 ++++------ test/addons/repl-domain-abort/binding.cc | 9 +++++++-- test/addons/repl-domain-abort/test.js | 3 ++- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/domain.js b/lib/domain.js index 558b67dca7dcba..08fbd207f171d3 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -94,7 +94,7 @@ process.setUncaughtExceptionCaptureCallback = function(fn) { throw err; }; -function topLevelDomainCallback(cb, args) { +function topLevelDomainCallback(cb, ...args) { const domain = this.domain; if (domain) domain.enter(); diff --git a/src/node.cc b/src/node.cc index 630adc7c389757..c9af7c7dcd1efc 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1000,12 +1000,10 @@ MaybeLocal InternalMakeCallback(Environment* env, if (asyncContext.async_id != 0 || domain_cb.IsEmpty() || recv.IsEmpty()) { ret = callback->Call(env->context(), recv, argc, argv); } else { - Local argv_array = Array::New(env->isolate(), argc); - for (int i = 0; i < argc; i++) { - argv_array->Set(env->context(), i, argv[i]).FromJust(); - } - Local domain_argv[] = { callback, argv_array }; - ret = domain_cb->Call(env->context(), recv, 2, domain_argv); + 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()) { diff --git a/test/addons/repl-domain-abort/binding.cc b/test/addons/repl-domain-abort/binding.cc index 7a04d0bcef8271..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,15 @@ using v8::Value; void Method(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); + Local params[] = { + Boolean::New(isolate, true), + Boolean::New(isolate, false) + }; Local ret = node::MakeCallback(isolate, isolate->GetCurrentContext()->Global(), args[0].As(), - 0, - nullptr); + 2, + params); assert(ret->IsTrue()); } diff --git a/test/addons/repl-domain-abort/test.js b/test/addons/repl-domain-abort/test.js index 331d7aabb7b3e4..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\'); return true; });', + '.method(function(v1, v2) {' + + 'console.log(\'cb_ran\'); return v1 === true && v2 === false; });', ]; const dInput = new stream.Readable();