From 75311dbc2f5fbd1c81dbab94e1372b55e0dbb1ac Mon Sep 17 00:00:00 2001
From: legendecas <legendecas@gmail.com>
Date: Fri, 14 Feb 2020 11:30:33 +0800
Subject: [PATCH] async_hooks: ensure event after been emitted on
 runInAsyncScope

The exception handler user-defined will not automatically emit after
for the async resource.

Also removes a duplicated case
`test-emit-after-uncaught-exception-runInAsyncScope.js`
which is identical to test-emit-after-uncaught-exception.js.

Refs: https://github.com/nodejs/node/pull/30965
PR-URL: https://github.com/nodejs/node/pull/31784
Fixes: https://github.com/nodejs/node/issues/31783
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 lib/async_hooks.js                            | 15 ++++---
 ...oks-run-in-async-scope-caught-exception.js |  9 +++++
 ...fter-uncaught-exception-runInAsyncScope.js | 40 -------------------
 3 files changed, 19 insertions(+), 45 deletions(-)
 create mode 100644 test/parallel/test-async-hooks-run-in-async-scope-caught-exception.js
 delete mode 100644 test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js

diff --git a/lib/async_hooks.js b/lib/async_hooks.js
index 923b9d675818a3..3ebc9af473d5c8 100644
--- a/lib/async_hooks.js
+++ b/lib/async_hooks.js
@@ -22,6 +22,7 @@ const {
   executionAsyncId,
   triggerAsyncId,
   // Private API
+  hasAsyncIdStack,
   getHookArrays,
   enableHooks,
   disableHooks,
@@ -179,12 +180,16 @@ class AsyncResource {
     const asyncId = this[async_id_symbol];
     emitBefore(asyncId, this[trigger_async_id_symbol], this);
 
-    const ret = thisArg === undefined ?
-      fn(...args) :
-      ReflectApply(fn, thisArg, args);
+    try {
+      const ret = thisArg === undefined ?
+        fn(...args) :
+        ReflectApply(fn, thisArg, args);
 
-    emitAfter(asyncId);
-    return ret;
+      return ret;
+    } finally {
+      if (hasAsyncIdStack())
+        emitAfter(asyncId);
+    }
   }
 
   emitDestroy() {
diff --git a/test/parallel/test-async-hooks-run-in-async-scope-caught-exception.js b/test/parallel/test-async-hooks-run-in-async-scope-caught-exception.js
new file mode 100644
index 00000000000000..78e38c1e9312d7
--- /dev/null
+++ b/test/parallel/test-async-hooks-run-in-async-scope-caught-exception.js
@@ -0,0 +1,9 @@
+'use strict';
+
+require('../common');
+const { AsyncResource } = require('async_hooks');
+
+try {
+  new AsyncResource('foo').runInAsyncScope(() => { throw new Error('bar'); });
+} catch {}
+// Should abort (fail the case) if async id is not matching.
diff --git a/test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js b/test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js
deleted file mode 100644
index 5003972e9984aa..00000000000000
--- a/test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js
+++ /dev/null
@@ -1,40 +0,0 @@
-'use strict';
-
-const common = require('../common');
-const assert = require('assert');
-const async_hooks = require('async_hooks');
-
-const id_obj = {};
-let collect = true;
-
-const hook = async_hooks.createHook({
-  before(id) { if (collect) id_obj[id] = true; },
-  after(id) { delete id_obj[id]; },
-}).enable();
-
-process.once('uncaughtException', common.mustCall((er) => {
-  assert.strictEqual(er.message, 'bye');
-  collect = false;
-}));
-
-setImmediate(common.mustCall(() => {
-  process.nextTick(common.mustCall(() => {
-    assert.strictEqual(Object.keys(id_obj).length, 0);
-    hook.disable();
-  }));
-
-  // Create a stack of async ids that will need to be emitted in the case of
-  // an uncaught exception.
-  const ar1 = new async_hooks.AsyncResource('Mine');
-  ar1.runInAsyncScope(() => {
-    const ar2 = new async_hooks.AsyncResource('Mine');
-    ar2.runInAsyncScope(() => {
-      throw new Error('bye');
-    });
-  });
-
-  // TODO(trevnorris): This test shows that the after() hooks are always called
-  // correctly, but it doesn't solve where the emitDestroy() is missed because
-  // of the uncaught exception. Simple solution is to always call emitDestroy()
-  // before the emitAfter(), but how to codify this?
-}));