Skip to content

Commit

Permalink
domain: further abstract usage in C++
Browse files Browse the repository at this point in the history
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: #18291
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
apapirovski authored and MylesBorins committed Feb 21, 2018
1 parent d764039 commit 44bf0f4
Show file tree
Hide file tree
Showing 10 changed files with 72 additions and 90 deletions.
12 changes: 11 additions & 1 deletion lib/domain.js
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
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,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
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,6 @@ inline Environment::Environment(IsolateData* isolate_data,
: isolate_(context->GetIsolate()),
isolate_data_(isolate_data),
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 @@ -379,14 +378,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
7 changes: 1 addition & 6 deletions src/env.h
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 @@ -283,6 +281,7 @@ class ModuleWrap;
V(async_hooks_binding, 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 @@ -537,9 +536,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 @@ -693,7 +689,6 @@ class Environment {
AsyncHooks async_hooks_;
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
78 changes: 10 additions & 68 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1118,62 +1118,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 @@ -1294,11 +1238,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 @@ -1328,11 +1267,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 Down Expand Up @@ -1379,7 +1313,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()
Expand Down Expand Up @@ -3635,7 +3578,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
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
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
16 changes: 11 additions & 5 deletions test/addons/repl-domain-abort/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <node.h>
#include <v8.h>

using v8::Boolean;
using v8::Function;
using v8::FunctionCallbackInfo;
using v8::Local;
Expand All @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion test/addons/repl-domain-abort/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions test/cctest/node_module_reg.cc
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

0 comments on commit 44bf0f4

Please sign in to comment.