async_wrap: correctly pass parent to init callback

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

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,19 +1078,19 @@ 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);
   }
 
   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);
+});