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.

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

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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 @@ -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
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
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
93 changes: 13 additions & 80 deletions src/node.cc
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,22 @@ 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);
} 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()
// 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();
Expand Down Expand Up @@ -3348,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);
}


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