async_hooks: run after() in case of exception

In the case of an exception being caught by _fatalException(), make sure to always run the after() callbacks to let the user know the callback has completed. Include test to make sure after() is triggered correctly, including having the correct currentId.
nodejs · trevnorris · Oct 31, 2016 · Oct 31, 2016 · Sep 28, 2016 · Nov 2, 2016
commit ad382c5a8258f5e7fdcb05435771f2fb1cd88354
diff --git a/lib/async_hooks.js b/lib/async_hooks.js
@@ -50,6 +50,11 @@ const { kInit, kBefore, kAfter, kDestroy, kActiveHooks, kAsyncUidCntr,
         kCurrentId, kTriggerId, kInitTriggerId, kScopedTriggerId } =
             async_wrap.constants;
 
+// Expose the current_trigger_id_stack via the async_wrap binding for internal
+// usage. Specifically so that _fatalException() can empty it if the exception
+// is caught.
+async_wrap.current_trigger_id_stack = current_trigger_id_stack;
+
 // Setup the callbacks that node::AsyncWrap will call when there are hooks to
 // process. They use the same functions as the JS embedder API.
 async_wrap.setupHooks({ init,
@@ -294,6 +299,7 @@ function emitInitS(id, type, triggerId, handle) {
     }
   }
 
+  // Isn't null if hooks were added/removed while the hooks were running.
   if (tmp_active_hooks_array !== null) {
     restoreTmpHooks();
   }
@@ -342,10 +348,6 @@ function emitAfterS(id) {
   }
 
   // Remove state after the call has completed.
-  // TODO(trevnorris): Clearing the native counters is easy, but also must
-  // track the length of the array manually so the native side can empty it
-  // in case there's an error. Or, export the array in such a way that
-  // fatalException can empty it as well.
   // TODO(trevnorris): Add a check that kCurrentId === id to make sure the
   // stack hasn't been corrupted.
   if (current_trigger_id_stack.length > 0) {

diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js
@@ -281,6 +281,9 @@
   }
 
   function setupProcessFatal() {
+    const async_wrap = process.binding('async_wrap');
+    const async_uid_fields = async_wrap.async_uid_fields;
+    const { kCurrentId, kTriggerId } = async_wrap.constants;
 
     process._fatalException = function(er) {
       var caught;
@@ -306,6 +309,16 @@
       // if we handled an error, then make sure any ticks get processed
       } else {
         NativeModule.require('timers').setImmediate(process._tickCallback);
+
+        // Emit the after() hooks now that the exception has been delt with.
+        NativeModule.require('async_hooks').emitAfter(
+          async_uid_fields[kCurrentId]);
+        async_uid_fields[kCurrentId] = 0;
+        async_uid_fields[kTriggerId] = 0;
+        async_wrap.current_trigger_id_stack.length = 0;
+        // No need to worry about running restoreTmpHooks() because that's
+        // only used while other hooks are running and if a hook throws then
+        // the application is forced to shut down.
       }
 
       return caught;

diff --git a/src/async-wrap.cc b/src/async-wrap.cc
@@ -435,9 +435,8 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
     return Local<Value>();
   }
 
-  // TODO(trevnorris): It will be confusing for developers if there's a caught
-  // uncaught exception. Which leads to none of their after() callbacks being
-  // called.
+  // If the callback failed then the after() hooks will be called at the end
+  // of _fatalException().
   if (async_hooks->fields()[AsyncHooks::kAfter] > 0) {
     if (uid.IsEmpty())
       uid = Number::New(env()->isolate(), get_id());

diff --git a/test/parallel/test-async-wrap-after-uncaughtexception.js b/test/parallel/test-async-wrap-after-uncaughtexception.js
@@ -0,0 +1,50 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+const thrown_ids = {};
+
+process.on('exit', () => {
+  process.removeAllListeners('uncaughtException');
+  assert.strictEqual(Object.keys(thrown_ids).length, 0, 'ids remain');
+});
+
+process.on('uncaughtException', common.mustCall((err) => {
+  assert.strictEqual(err.message, thrown_ids[async_hooks.currentId()]);
+}, 5));
+
+async_hooks.createHook({
+  after(id) {
+    delete thrown_ids[id];
+  },
+}).enable();
+
+const simId = setImmediate(() => {
+  throw new Error('setImmediate');
+})._asyncId;
+thrown_ids[simId] = 'setImmediate';
+
+const stId = setTimeout(() => {
+  throw new Error('setTimeout');
+}, 10)._asyncId;
+thrown_ids[stId] = 'setTimeout';
+
+const sinId = setInterval(function() {
+  clearInterval(this);
+  throw new Error('setInterval');
+}, 10)._asyncId;
+thrown_ids[sinId] = 'setInterval';
+
+const rbId = require('crypto').randomBytes(1, () => {
+  throw new Error('RANDOMBYTESREQUEST');
+}).getAsyncId();
+thrown_ids[rbId] = 'RANDOMBYTESREQUEST';
+
+const tcpId = require('net').createServer(function(c) {
+  c.end();
+  setImmediate(() => this.close());
+  throw new Error('TCPWRAP');
+}).listen(common.PORT)._handle.getAsyncId();
+thrown_ids[tcpId] = 'TCPWRAP';
+require('net').connect(common.PORT, () => {}).resume();