diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index 841c7235f52d33..cac8175889dfbf 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -24,32 +24,39 @@ inline AsyncWrap::AsyncWrap(Environment* env, // Shift provider value over to prevent id collision. persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider); - // Check user controlled flag to see if the init callback should run. - if (!env->using_asyncwrap()) - return; + v8::Local init_fn = env->async_hooks_init_function(); - // If callback hooks have not been enabled, and there is no parent, return. - if (!env->async_wrap_callbacks_enabled() && parent == nullptr) + // No init callback exists, no reason to go on. + if (init_fn.IsEmpty()) return; - // If callback hooks have not been enabled and parent has no queue, return. - if (!env->async_wrap_callbacks_enabled() && !parent->has_async_queue()) + // If async wrap callbacks are disabled and no parent was passed that has + // run the init callback then return. + if (!env->async_wrap_callbacks_enabled() && + (parent == nullptr || !parent->ran_init_callback())) return; v8::HandleScope scope(env->isolate()); - v8::TryCatch try_catch; - v8::Local n = v8::Int32::New(env->isolate(), provider); - env->async_hooks_init_function()->Call(object, 1, &n); + v8::Local argv[] = { + v8::Int32::New(env->isolate(), provider), + Null(env->isolate()) + }; + + if (parent != nullptr) + argv[1] = parent->object(); + + v8::MaybeLocal ret = + init_fn->Call(env->context(), object, ARRAY_SIZE(argv), argv); - if (try_catch.HasCaught()) + if (ret.IsEmpty()) FatalError("node::AsyncWrap::AsyncWrap", "init hook threw"); - bits_ |= 1; // has_async_queue() is true now. + bits_ |= 1; // ran_init_callback() is true now. } -inline bool AsyncWrap::has_async_queue() const { +inline bool AsyncWrap::ran_init_callback() const { return static_cast(bits_ & 1); } diff --git a/src/async-wrap.cc b/src/async-wrap.cc index ccf357ba24a21c..4f27e5116dca8d 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -123,8 +123,6 @@ static void SetupHooks(const FunctionCallbackInfo& args) { env->set_async_hooks_init_function(args[0].As()); env->set_async_hooks_pre_function(args[1].As()); env->set_async_hooks_post_function(args[2].As()); - - env->set_using_asyncwrap(true); } @@ -146,6 +144,10 @@ static void Initialize(Local target, NODE_ASYNC_PROVIDER_TYPES(V) #undef V target->Set(FIXED_ONE_BYTE_STRING(isolate, "Providers"), async_providers); + + env->set_async_hooks_init_function(Local()); + env->set_async_hooks_pre_function(Local()); + env->set_async_hooks_post_function(Local()); } @@ -164,6 +166,8 @@ Local AsyncWrap::MakeCallback(const Local cb, Local* argv) { CHECK(env()->context() == env()->isolate()->GetCurrentContext()); + Local pre_fn = env()->async_hooks_pre_function(); + Local post_fn = env()->async_hooks_post_function(); Local context = object(); Local process = env()->process_object(); Local domain; @@ -179,7 +183,7 @@ Local AsyncWrap::MakeCallback(const Local cb, } } - TryCatch try_catch; + TryCatch try_catch(env()->isolate()); try_catch.SetVerbose(true); if (has_domain) { @@ -191,9 +195,9 @@ Local AsyncWrap::MakeCallback(const Local cb, } } - if (has_async_queue()) { + if (ran_init_callback() && !pre_fn.IsEmpty()) { try_catch.SetVerbose(false); - env()->async_hooks_pre_function()->Call(context, 0, nullptr); + pre_fn->Call(context, 0, nullptr); if (try_catch.HasCaught()) FatalError("node::AsyncWrap::MakeCallback", "pre hook threw"); try_catch.SetVerbose(true); @@ -205,9 +209,9 @@ Local AsyncWrap::MakeCallback(const Local cb, return Undefined(env()->isolate()); } - if (has_async_queue()) { + if (ran_init_callback() && !post_fn.IsEmpty()) { try_catch.SetVerbose(false); - env()->async_hooks_post_function()->Call(context, 0, nullptr); + post_fn->Call(context, 0, nullptr); if (try_catch.HasCaught()) FatalError("node::AsyncWrap::MakeCallback", "post hook threw"); try_catch.SetVerbose(true); diff --git a/src/async-wrap.h b/src/async-wrap.h index 3999607bb293c2..330f3454f42d2c 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -70,7 +70,7 @@ class AsyncWrap : public BaseObject { private: inline AsyncWrap(); - inline bool has_async_queue() const; + inline bool ran_init_callback() const; // When the async hooks init JS function is called from the constructor it is // expected the context object will receive a _asyncQueue object property diff --git a/src/env-inl.h b/src/env-inl.h index 0b4b4a5a4a5ef2..2d965f9a519232 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -208,7 +208,6 @@ inline Environment::Environment(v8::Local context, isolate_data_(IsolateData::GetOrCreate(context->GetIsolate(), loop)), timer_base_(uv_now(loop)), using_domains_(false), - using_asyncwrap_(false), printed_error_(false), trace_sync_io_(false), debugger_agent_(this), @@ -348,14 +347,6 @@ inline void Environment::set_using_domains(bool value) { using_domains_ = value; } -inline bool Environment::using_asyncwrap() const { - return using_asyncwrap_; -} - -inline void Environment::set_using_asyncwrap(bool value) { - using_asyncwrap_ = value; -} - inline bool Environment::printed_error() const { return printed_error_; } diff --git a/src/env.h b/src/env.h index 4a11c5064cadad..97652146fabf0d 100644 --- a/src/env.h +++ b/src/env.h @@ -435,9 +435,6 @@ class Environment { inline bool using_domains() const; inline void set_using_domains(bool value); - inline bool using_asyncwrap() const; - inline void set_using_asyncwrap(bool value); - inline bool printed_error() const; inline void set_printed_error(bool value); @@ -537,7 +534,6 @@ class Environment { ares_channel cares_channel_; ares_task_list cares_task_list_; bool using_domains_; - bool using_asyncwrap_; bool printed_error_; bool trace_sync_io_; debugger::Agent debugger_agent_; diff --git a/src/node.cc b/src/node.cc index a7800f8b4c54fd..e5a87e711bd84b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1040,15 +1040,19 @@ Local MakeCallback(Environment* env, // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); + Local pre_fn = env->async_hooks_pre_function(); + Local post_fn = env->async_hooks_post_function(); Local object, domain; - bool has_async_queue = false; + bool ran_init_callback = false; bool has_domain = false; + // TODO(trevnorris): Adding "_asyncQueue" to the "this" in the init callback + // is a horrible way to detect usage. Rethink how detection should happen. if (recv->IsObject()) { object = recv.As(); Local async_queue_v = object->Get(env->async_queue_string()); if (async_queue_v->IsObject()) - has_async_queue = true; + ran_init_callback = true; } if (env->using_domains()) { @@ -1074,9 +1078,9 @@ Local MakeCallback(Environment* env, } } - if (has_async_queue) { + if (ran_init_callback && !pre_fn.IsEmpty()) { try_catch.SetVerbose(false); - env->async_hooks_pre_function()->Call(object, 0, nullptr); + pre_fn->Call(object, 0, nullptr); if (try_catch.HasCaught()) FatalError("node::MakeCallback", "pre hook threw"); try_catch.SetVerbose(true); @@ -1084,9 +1088,9 @@ Local MakeCallback(Environment* env, Local ret = callback->Call(recv, argc, argv); - if (has_async_queue) { + if (ran_init_callback && !post_fn.IsEmpty()) { try_catch.SetVerbose(false); - env->async_hooks_post_function()->Call(object, 0, nullptr); + post_fn->Call(object, 0, nullptr); if (try_catch.HasCaught()) FatalError("node::MakeCallback", "post hook threw"); try_catch.SetVerbose(true); diff --git a/test/parallel/test-async-wrap-disabled-propagate-parent.js b/test/parallel/test-async-wrap-disabled-propagate-parent.js new file mode 100644 index 00000000000000..de36071524c4fe --- /dev/null +++ b/test/parallel/test-async-wrap-disabled-propagate-parent.js @@ -0,0 +1,46 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const net = require('net'); +const async_wrap = process.binding('async_wrap'); +const providers = Object.keys(async_wrap.Providers); + +let cntr = 0; +let server; +let client; + +function init(type, parent) { + if (parent) { + cntr++; + // Cannot assert in init callback or will abort. + process.nextTick(() => { + assert.equal(providers[type], 'TCPWRAP'); + assert.equal(parent, server._handle, 'server doesn\'t match parent'); + assert.equal(this, client._handle, 'client doesn\'t match context'); + }); + } +} + +function noop() { } + +async_wrap.setupHooks(init, noop, noop); +async_wrap.enable(); + +server = net.createServer(function(c) { + client = c; + // Allow init callback to run before closing. + setImmediate(() => { + c.end(); + this.close(); + }); +}).listen(common.PORT, function() { + net.connect(common.PORT, noop); +}); + +async_wrap.disable(); + +process.on('exit', function() { + // init should have only been called once with a parent. + assert.equal(cntr, 1); +}); diff --git a/test/parallel/test-async-wrap-propagate-parent.js b/test/parallel/test-async-wrap-propagate-parent.js new file mode 100644 index 00000000000000..8074b0062e0db2 --- /dev/null +++ b/test/parallel/test-async-wrap-propagate-parent.js @@ -0,0 +1,43 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const net = require('net'); +const async_wrap = process.binding('async_wrap'); + +let cntr = 0; +let server; +let client; + +function init(type, parent) { + if (parent) { + cntr++; + // Cannot assert in init callback or will abort. + process.nextTick(() => { + assert.equal(parent, server._handle, 'server doesn\'t match parent'); + assert.equal(this, client._handle, 'client doesn\'t match context'); + }); + } +} + +function noop() { } + +async_wrap.setupHooks(init, noop, noop); +async_wrap.enable(); + +server = net.createServer(function(c) { + client = c; + // Allow init callback to run before closing. + setImmediate(() => { + c.end(); + this.close(); + }); +}).listen(common.PORT, function() { + net.connect(common.PORT, noop); +}); + + +process.on('exit', function() { + // init should have only been called once with a parent. + assert.equal(cntr, 1); +});