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>

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())
+  v8::Local<v8::Function> init_fn = env->async_hooks_init_function();
+
+  // No init callback exists, no reason to go on.
+  if (init_fn.IsEmpty())
     return;
 
-  // If callback hooks have not been enabled, and there is no parent, return.
-  if (!env->async_wrap_callbacks_enabled() && parent == nullptr)
-    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 (try_catch.HasCaught())
+  if (parent != nullptr)
+    argv[1] = parent->object();
+
+  v8::MaybeLocal<v8::Value> ret =
+      init_fn->Call(env->context(), object, ARRAY_SIZE(argv), argv);
+
+  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);
 }
 

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);

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

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_;
 }

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_;

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);

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);
+});

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);
+});