Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

domain, src: clean up domain-related code #18291

Closed
wants to merge 5 commits into from
Closed

domain, src: clean up domain-related code #18291

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fab0c98
src: remove unnecessary block scope
apapirovski Jan 21, 2018
0d66284
src: remove unnecessary async hooks check
apapirovski Jan 21, 2018
9fdf293
src: remove outdated domain reference
apapirovski Jan 19, 2018
c6cadb3
domain: further abstract usage in C++
apapirovski Jan 19, 2018
930afeb
fixup: bnoordhuis suggestion + test adjustment
apapirovski Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion lib/domain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
1 change: 1 addition & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
9 changes: 0 additions & 9 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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_;
}
Expand Down
8 changes: 1 addition & 7 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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") \
Expand Down Expand Up @@ -244,7 +242,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") \
Expand Down Expand Up @@ -281,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) \
Expand Down Expand Up @@ -568,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);

Expand Down Expand Up @@ -747,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_;
Expand Down
95 changes: 15 additions & 80 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
Expand All @@ -1042,11 +976,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<Object> process = env_->process_object();

if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
Expand All @@ -1066,17 +995,24 @@ MaybeLocal<Value> InternalMakeCallback(Environment* env,
return Undefined(env->isolate());
}

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);

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());
} else {
Local<Array> argv_array = Array::New(env->isolate(), argc);
for (int i = 0; i < argc; i++) {
argv_array->Set(env->context(), i, argv[i]).FromJust();
}
Local<Value> domain_argv[] = { callback, argv_array };
ret = domain_cb->Call(env->context(), recv, 2, domain_argv);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably faster to use spread in JS land and skip allocating the JS array here. I.e.:

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]);

Then in JS:

function topLevelDomainCallback(cb, ...args) {
  // ...
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bnoordhuis, will update!

}

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();
Expand Down Expand Up @@ -3348,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);
}


Expand Down
34 changes: 34 additions & 0 deletions src/node_domain.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
1 change: 1 addition & 0 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ struct sockaddr;
V(cares_wrap) \
V(config) \
V(contextify) \
V(domain) \
V(fs) \
V(fs_event_wrap) \
V(http2) \
Expand Down
11 changes: 6 additions & 5 deletions test/addons/repl-domain-abort/binding.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,12 @@ 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> ret = node::MakeCallback(isolate,
isolate->GetCurrentContext()->Global(),
args[0].As<Function>(),
0,
nullptr);
assert(ret->IsTrue());
}

void init(Local<Object> exports) {
Expand Down
2 changes: 1 addition & 1 deletion test/addons/repl-domain-abort/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions test/cctest/node_module_reg.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
Expand Down