async_hooks: use scope for defaultTriggerAsyncId

Previously the getter would mutate the kDefaultTriggerAsncId value. This refactor changes the setter to bind the current kDefaultTriggerAsncId to a scope, such that the getter doesn't have to mutate its own value. PR-URL: nodejs#17273 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
maclover7 · Dec 19, 2017 · 3b8da4c · 3b8da4c
1 parent 0784b04
commit 3b8da4c
Show file tree

Hide file tree

Showing 12 changed files with 142 additions and 121 deletions.
diff --git a/lib/dgram.js b/lib/dgram.js
@@ -28,7 +28,7 @@ const dns = require('dns');
 const util = require('util');
 const { isUint8Array } = require('internal/util/types');
 const EventEmitter = require('events');
-const { setDefaultTriggerAsyncId } = require('internal/async_hooks');
+const { defaultTriggerAsyncIdScope } = require('internal/async_hooks');
 const { UV_UDP_REUSEADDR } = process.binding('constants').os;
 const { async_id_symbol } = process.binding('async_wrap');
 const { nextTick } = require('internal/process/next_tick');
@@ -450,21 +450,24 @@ Socket.prototype.send = function(buffer,
   }
 
   const afterDns = (ex, ip) => {
-    doSend(ex, this, ip, list, address, port, callback);
+    defaultTriggerAsyncIdScope(
+      this[async_id_symbol],
+      [ex, this, ip, list, address, port, callback],
+      doSend
+    );
   };
 
   this._handle.lookup(address, afterDns);
 };
 
-
 function doSend(ex, self, ip, list, address, port, callback) {
   if (ex) {
     if (typeof callback === 'function') {
-      callback(ex);
+      process.nextTick(callback, ex);
       return;
     }
 
-    self.emit('error', ex);
+    process.nextTick(() => self.emit('error', ex));
     return;
   } else if (!self._handle) {
     return;
@@ -478,20 +481,18 @@ function doSend(ex, self, ip, list, address, port, callback) {
     req.callback = callback;
     req.oncomplete = afterSend;
   }
-  // node::SendWrap isn't instantiated and attached to the JS instance of
-  // SendWrap above until send() is called. So don't set the init trigger id
-  // until now.
-  setDefaultTriggerAsyncId(self[async_id_symbol]);
+
   var err = self._handle.send(req,
                               list,
                               list.length,
                               port,
                               ip,
                               !!callback);
+
   if (err && callback) {
     // don't emit as error, dgram_legacy.js compatibility
     const ex = exceptionWithHostPort(err, 'send', address, port);
-    nextTick(self[async_id_symbol], callback, ex);
+    process.nextTick(callback, ex);
   }
 }
 

diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js
@@ -250,20 +250,27 @@ function newUid() {
 // constructor is complete.
 function getDefaultTriggerAsyncId() {
   var defaultTriggerAsyncId = async_id_fields[kDefaultTriggerAsyncId];
-  // Reset value after it's been called so the next constructor doesn't
-  // inherit it by accident.
-  async_id_fields[kDefaultTriggerAsyncId] = -1;
   // If defaultTriggerAsyncId isn't set, use the executionAsyncId
   if (defaultTriggerAsyncId < 0)
     defaultTriggerAsyncId = async_id_fields[kExecutionAsyncId];
   return defaultTriggerAsyncId;
 }
 
 
-function setDefaultTriggerAsyncId(triggerAsyncId) {
+function defaultTriggerAsyncIdScope(triggerAsyncId, opaque, block) {
   // CHECK(Number.isSafeInteger(triggerAsyncId))
   // CHECK(triggerAsyncId > 0)
+  const oldDefaultTriggerAsyncId = async_id_fields[kDefaultTriggerAsyncId];
   async_id_fields[kDefaultTriggerAsyncId] = triggerAsyncId;
+
+  var ret;
+  try {
+    ret = Reflect.apply(block, null, opaque);
+  } finally {
+    async_id_fields[kDefaultTriggerAsyncId] = oldDefaultTriggerAsyncId;
+  }
+
+  return ret;
 }
 
 
@@ -285,10 +292,6 @@ function emitInitScript(asyncId, type, triggerAsyncId, resource) {
   // manually means that the embedder must have used getDefaultTriggerAsyncId().
   if (triggerAsyncId === null) {
     triggerAsyncId = getDefaultTriggerAsyncId();
-  } else {
-    // If a triggerAsyncId was passed, any kDefaultTriggerAsyncId still must be
-    // null'd.
-    async_id_fields[kDefaultTriggerAsyncId] = -1;
   }
 
   emitInitNative(asyncId, type, triggerAsyncId, resource);
@@ -341,7 +344,7 @@ module.exports = {
   // Internal Embedder API
   newUid,
   getDefaultTriggerAsyncId,
-  setDefaultTriggerAsyncId,
+  defaultTriggerAsyncIdScope,
   emitInit: emitInitScript,
   emitBefore: emitBeforeScript,
   emitAfter: emitAfterScript,

diff --git a/lib/net.js b/lib/net.js
@@ -43,7 +43,7 @@ const { TCPConnectWrap } = process.binding('tcp_wrap');
 const { PipeConnectWrap } = process.binding('pipe_wrap');
 const { ShutdownWrap, WriteWrap } = process.binding('stream_wrap');
 const { async_id_symbol } = process.binding('async_wrap');
-const { newUid, setDefaultTriggerAsyncId } = require('internal/async_hooks');
+const { newUid, defaultTriggerAsyncIdScope } = require('internal/async_hooks');
 const { nextTick } = require('internal/process/next_tick');
 const errors = require('internal/errors');
 const dns = require('dns');
@@ -277,6 +277,14 @@ Socket.prototype._unrefTimer = function _unrefTimer() {
     timers._unrefActive(s);
 };
 
+
+function shutdownSocket(self, callback) {
+  var req = new ShutdownWrap();
+  req.oncomplete = callback;
+  req.handle = self._handle;
+  return self._handle.shutdown(req);
+}
+
 // the user has called .end(), and all the bytes have been
 // sent out to the other side.
 function onSocketFinish() {
@@ -298,14 +306,9 @@ function onSocketFinish() {
   if (!this._handle || !this._handle.shutdown)
     return this.destroy();
 
-  var req = new ShutdownWrap();
-  req.oncomplete = afterShutdown;
-  req.handle = this._handle;
-  // node::ShutdownWrap isn't instantiated and attached to the JS instance of
-  // ShutdownWrap above until shutdown() is called. So don't set the init
-  // trigger id until now.
-  setDefaultTriggerAsyncId(this[async_id_symbol]);
-  var err = this._handle.shutdown(req);
+  var err = defaultTriggerAsyncIdScope(
+    this[async_id_symbol], [this, afterShutdown], shutdownSocket
+  );
 
   if (err)
     return this.destroy(errnoException(err, 'shutdown'));
@@ -945,23 +948,15 @@ function internalConnect(
     req.localAddress = localAddress;
     req.localPort = localPort;
 
-    // node::TCPConnectWrap isn't instantiated and attached to the JS instance
-    // of TCPConnectWrap above until connect() is called. So don't set the init
-    // trigger id until now.
-    setDefaultTriggerAsyncId(self[async_id_symbol]);
     if (addressType === 4)
       err = self._handle.connect(req, address, port);
     else
       err = self._handle.connect6(req, address, port);
-
   } else {
     const req = new PipeConnectWrap();
     req.address = address;
     req.oncomplete = afterConnect;
-    // node::PipeConnectWrap isn't instantiated and attached to the JS instance
-    // of PipeConnectWrap above until connect() is called. So don't set the
-    // init trigger id until now.
-    setDefaultTriggerAsyncId(self[async_id_symbol]);
+
     err = self._handle.connect(req, address, afterConnect);
   }
 
@@ -1030,7 +1025,9 @@ Socket.prototype.connect = function(...args) {
                                  'string',
                                  path);
     }
-    internalConnect(this, path);
+    defaultTriggerAsyncIdScope(
+      this[async_id_symbol], [this, path], internalConnect
+    );
   } else {
     lookupAndConnect(this, options);
   }
@@ -1073,7 +1070,11 @@ function lookupAndConnect(self, options) {
   if (addressType) {
     nextTick(self[async_id_symbol], function() {
       if (self.connecting)
-        internalConnect(self, host, port, addressType, localAddress, localPort);
+        defaultTriggerAsyncIdScope(
+          self[async_id_symbol],
+          [self, host, port, addressType, localAddress, localPort],
+          internalConnect
+        );
     });
     return;
   }
@@ -1097,33 +1098,33 @@ function lookupAndConnect(self, options) {
   debug('connect: dns options', dnsopts);
   self._host = host;
   var lookup = options.lookup || dns.lookup;
-  setDefaultTriggerAsyncId(self[async_id_symbol]);
-  lookup(host, dnsopts, function emitLookup(err, ip, addressType) {
-    self.emit('lookup', err, ip, addressType, host);
+  defaultTriggerAsyncIdScope(self[async_id_symbol], [], function() {
+    lookup(host, dnsopts, function emitLookup(err, ip, addressType) {
+      self.emit('lookup', err, ip, addressType, host);
 
-    // It's possible we were destroyed while looking this up.
-    // XXX it would be great if we could cancel the promise returned by
-    // the look up.
-    if (!self.connecting) return;
+      // It's possible we were destroyed while looking this up.
+      // XXX it would be great if we could cancel the promise returned by
+      // the look up.
+      if (!self.connecting) return;
 
-    if (err) {
-      // net.createConnection() creates a net.Socket object and
-      // immediately calls net.Socket.connect() on it (that's us).
-      // There are no event listeners registered yet so defer the
-      // error event to the next tick.
-      err.host = options.host;
-      err.port = options.port;
-      err.message = err.message + ' ' + options.host + ':' + options.port;
-      process.nextTick(connectErrorNT, self, err);
-    } else {
-      self._unrefTimer();
-      internalConnect(self,
-                      ip,
-                      port,
-                      addressType,
-                      localAddress,
-                      localPort);
-    }
+      if (err) {
+        // net.createConnection() creates a net.Socket object and
+        // immediately calls net.Socket.connect() on it (that's us).
+        // There are no event listeners registered yet so defer the
+        // error event to the next tick.
+        err.host = options.host;
+        err.port = options.port;
+        err.message = err.message + ' ' + options.host + ':' + options.port;
+        process.nextTick(connectErrorNT, self, err);
+      } else {
+        self._unrefTimer();
+        defaultTriggerAsyncIdScope(
+          self[async_id_symbol],
+          [self, ip, port, addressType, localAddress, localPort],
+          internalConnect
+        );
+      }
+    });
   });
 }
 

diff --git a/src/async_wrap.cc b/src/async_wrap.cc
@@ -308,12 +308,13 @@ static void PromiseHook(PromiseHookType type, Local<Promise> promise,
       if (parent_wrap == nullptr) {
         parent_wrap = PromiseWrap::New(env, parent_promise, nullptr, true);
       }
-      // get id from parentWrap
-      double trigger_async_id = parent_wrap->get_async_id();
-      env->set_default_trigger_async_id(trigger_async_id);
-    }
 
-    wrap = PromiseWrap::New(env, promise, parent_wrap, silent);
+      AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(
+        env, parent_wrap->get_async_id());
+      wrap = PromiseWrap::New(env, promise, parent_wrap, silent);
+    } else {
+      wrap = PromiseWrap::New(env, promise, nullptr, silent);
+    }
   }
 
   CHECK_NE(wrap, nullptr);

diff --git a/src/connection_wrap.cc b/src/connection_wrap.cc
@@ -49,7 +49,6 @@ void ConnectionWrap<WrapType, UVType>::OnConnection(uv_stream_t* handle,
   };
 
   if (status == 0) {
-    env->set_default_trigger_async_id(wrap_data->get_async_id());
     // Instantiate the client javascript object and handle.
     Local<Object> client_obj = WrapType::Instantiate(env,
                                                      wrap_data,

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -176,23 +176,27 @@ inline void Environment::AsyncHooks::clear_async_id_stack() {
   async_id_fields_[kTriggerAsyncId] = 0;
 }
 
-inline Environment::AsyncHooks::InitScope::InitScope(
-    Environment* env, double init_trigger_async_id)
-        : env_(env),
-          async_id_fields_ref_(env->async_hooks()->async_id_fields()) {
-  if (env_->async_hooks()->fields()[AsyncHooks::kCheck] > 0) {
-    CHECK_GE(init_trigger_async_id, -1);
+inline Environment::AsyncHooks::DefaultTriggerAsyncIdScope
+  ::DefaultTriggerAsyncIdScope(Environment* env,
+                               double default_trigger_async_id)
+    : async_id_fields_ref_(env->async_hooks()->async_id_fields()) {
+  if (env->async_hooks()->fields()[AsyncHooks::kCheck] > 0) {
+    CHECK_GE(default_trigger_async_id, 0);
   }
-  env->async_hooks()->push_async_ids(
-    async_id_fields_ref_[AsyncHooks::kExecutionAsyncId],
-    init_trigger_async_id);
+
+  old_default_trigger_async_id_ =
+    async_id_fields_ref_[AsyncHooks::kDefaultTriggerAsyncId];
+  async_id_fields_ref_[AsyncHooks::kDefaultTriggerAsyncId] =
+    default_trigger_async_id;
 }
 
-inline Environment::AsyncHooks::InitScope::~InitScope() {
-  env_->async_hooks()->pop_async_id(
-    async_id_fields_ref_[AsyncHooks::kExecutionAsyncId]);
+inline Environment::AsyncHooks::DefaultTriggerAsyncIdScope
+  ::~DefaultTriggerAsyncIdScope() {
+  async_id_fields_ref_[AsyncHooks::kDefaultTriggerAsyncId] =
+    old_default_trigger_async_id_;
 }
 
+
 inline Environment::AsyncCallbackScope::AsyncCallbackScope(Environment* env)
     : env_(env) {
   env_->makecallback_cntr_++;
@@ -456,17 +460,12 @@ inline double Environment::trigger_async_id() {
 inline double Environment::get_default_trigger_async_id() {
   double default_trigger_async_id =
     async_hooks()->async_id_fields()[AsyncHooks::kDefaultTriggerAsyncId];
-  async_hooks()->async_id_fields()[AsyncHooks::kDefaultTriggerAsyncId] = -1;
   // If defaultTriggerAsyncId isn't set, use the executionAsyncId
   if (default_trigger_async_id < 0)
     default_trigger_async_id = execution_async_id();
   return default_trigger_async_id;
 }
 
-inline void Environment::set_default_trigger_async_id(const double id) {
-  async_hooks()->async_id_fields()[AsyncHooks::kDefaultTriggerAsyncId] = id;
-}
-
 inline double* Environment::heap_statistics_buffer() const {
   CHECK_NE(heap_statistics_buffer_, nullptr);
   return heap_statistics_buffer_;

diff --git a/src/env.h b/src/env.h
@@ -402,22 +402,23 @@ class Environment {
     inline size_t stack_size();
     inline void clear_async_id_stack();  // Used in fatal exceptions.
 
-    // Used to propagate the trigger_async_id to the constructor of any newly
-    // created resources using RAII. Instead of needing to pass the
-    // trigger_async_id along with other constructor arguments.
-    class InitScope {
+    // Used to set the kDefaultTriggerAsyncId in a scope. This is instead of
+    // passing the trigger_async_id along with other constructor arguments.
+    class DefaultTriggerAsyncIdScope {
      public:
-      InitScope() = delete;
-      explicit InitScope(Environment* env, double init_trigger_async_id);
-      ~InitScope();
+      DefaultTriggerAsyncIdScope() = delete;
+      explicit DefaultTriggerAsyncIdScope(Environment* env,
+                                          double init_trigger_async_id);
+      ~DefaultTriggerAsyncIdScope();
 
      private:
-      Environment* env_;
       AliasedBuffer<double, v8::Float64Array> async_id_fields_ref_;
+      double old_default_trigger_async_id_;
 
-      DISALLOW_COPY_AND_ASSIGN(InitScope);
+      DISALLOW_COPY_AND_ASSIGN(DefaultTriggerAsyncIdScope);
     };
 
+
    private:
     friend class Environment;  // So we can call the constructor.
     inline explicit AsyncHooks(v8::Isolate* isolate);
@@ -559,7 +560,6 @@ class Environment {
   inline double execution_async_id();
   inline double trigger_async_id();
   inline double get_default_trigger_async_id();
-  inline void set_default_trigger_async_id(const double id);
 
   // List of id's that have been destroyed and need the destroy() cb called.
   inline std::vector<double>* destroy_async_id_list();

diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc
@@ -53,7 +53,8 @@ Local<Object> PipeWrap::Instantiate(Environment* env,
                                     AsyncWrap* parent,
                                     PipeWrap::SocketType type) {
   EscapableHandleScope handle_scope(env->isolate());
-  AsyncHooks::InitScope init_scope(env, parent->get_async_id());
+  AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(env,
+                                                       parent->get_async_id());
   CHECK_EQ(false, env->pipe_constructor_template().IsEmpty());
   Local<Function> constructor = env->pipe_constructor_template()->GetFunction();
   CHECK_EQ(false, constructor.IsEmpty());