From 3eaa593a320d62a18324f9e9ac36b1fd56042d0d Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 1 Oct 2015 18:57:36 -0600 Subject: [PATCH] async_wrap: correctly pass parent to init callback Previous logic didn't allow parent to propagate to the init callback properly. The fix now allows the init callback to be called and receive the parent if: - async wrap callbacks are enabled and parent exists - the init callback has been called on the parent and an init callback exists then it will be called regardless of whether async wrap callbacks are disabled. Change the init/pre/post callback checks to see if it has been properly set. This allows removal of the Environment "using_asyncwrap" variable. Pass Isolate to a TryCatch instance. Fixes: https://github.com/nodejs/node/issues/2986 PR-URL: https://github.com/nodejs/node/pull/3216 Reviewed-By: Rod Vagg --- src/async-wrap-inl.h | 33 +++++++------ src/async-wrap.cc | 18 +++++--- src/async-wrap.h | 2 +- src/env-inl.h | 9 ---- src/env.h | 4 -- src/node.cc | 16 ++++--- ...st-async-wrap-disabled-propagate-parent.js | 46 +++++++++++++++++++ .../test-async-wrap-propagate-parent.js | 43 +++++++++++++++++ 8 files changed, 131 insertions(+), 40 deletions(-) create mode 100644 test/parallel/test-async-wrap-disabled-propagate-parent.js create mode 100644 test/parallel/test-async-wrap-propagate-parent.js 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 e74d5133d526bf..90d10883acd807 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); +});