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 <rod@vagg.org>

diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h
index 841c723..cac8175 100644 (file)
--- 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<v8::Function> 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<v8::Value> n = v8::Int32::New(env->isolate(), provider);
-  env->async_hooks_init_function()->Call(object, 1, &n);
+  v8::Local<v8::Value> argv[] = {
+    v8::Int32::New(env->isolate(), provider),
+    Null(env->isolate())
+  };
+
+  if (parent != nullptr)
+    argv[1] = parent->object();
+
+  v8::MaybeLocal<v8::Value> 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<bool>(bits_ & 1);
 }
 

diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index ccf357b..4f27e51 100644 (file)
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -123,8 +123,6 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
   env->set_async_hooks_init_function(args[0].As<Function>());
   env->set_async_hooks_pre_function(args[1].As<Function>());
   env->set_async_hooks_post_function(args[2].As<Function>());
-
-  env->set_using_asyncwrap(true);
 }
 
 
@@ -146,6 +144,10 @@ static void Initialize(Local<Object> 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<Function>());
+  env->set_async_hooks_pre_function(Local<Function>());
+  env->set_async_hooks_post_function(Local<Function>());
 }
 
 
@@ -164,6 +166,8 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
                                       Local<Value>* argv) {
   CHECK(env()->context() == env()->isolate()->GetCurrentContext());
 
+  Local<Function> pre_fn = env()->async_hooks_pre_function();
+  Local<Function> post_fn = env()->async_hooks_post_function();
   Local<Object> context = object();
   Local<Object> process = env()->process_object();
   Local<Object> domain;
@@ -179,7 +183,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
     }
   }
 
-  TryCatch try_catch;
+  TryCatch try_catch(env()->isolate());
   try_catch.SetVerbose(true);
 
   if (has_domain) {
@@ -191,9 +195,9 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> 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<Value> AsyncWrap::MakeCallback(const Local<Function> 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 3999607..330f345 100644 (file)
--- 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 0b4b4a5..2d965f9 100644 (file)
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -208,7 +208,6 @@ inline Environment::Environment(v8::Local<v8::Context> 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 4a11c50..9765214 100644 (file)
--- 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 e74d513..90d1088 100644 (file)
--- a/src/node.cc
+++ b/src/node.cc
@@ -1040,15 +1040,19 @@ Local<Value> MakeCallback(Environment* env,
   // If you hit this assertion, you forgot to enter the v8::Context first.
   CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
 
+  Local<Function> pre_fn = env->async_hooks_pre_function();
+  Local<Function> post_fn = env->async_hooks_post_function();
   Local<Object> 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<Object>();
     Local<Value> 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<Value> 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<Value> MakeCallback(Environment* env,
 
   Local<Value> 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 (file)
index 0000000..de36071
--- /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 (file)
index 0000000..8074b00
--- /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);
+});