From d5409f41ebf6bd6a957abda5be029cc1de4fea18 Mon Sep 17 00:00:00 2001
From: Dan Fabulich <dan.fabulich@redfin.com>
Date: Thu, 23 Apr 2020 00:17:18 -0700
Subject: [PATCH] process: Don't warn/throw on unhandled rejections when hook
 is set

--unhandled-rejections has three explicit modes (strict, warn, none)
plus one implicit "default" mode, which logs an additional deprecation
warning (DEP0018).

Prior to this commit, the default mode was subtly different from warn
mode. If the unhandledRejections hook is set, default mode suppresses
all warnings. In warn mode, unhandledRejections would always fire a
warning, regardless of whether the hook was set.

In addition, prior to this commit, strict mode would always throw an
exception, regardless of whether the hook was set.

In this commit, all modes honor the unhandledRejections hook. If the
user has set the hook, then the user has taken full responsibility over
the behavior of unhandled rejections. In that case, no additional
warnings or thrown exceptions will be fired, even in warn mode or
strict mode.

This commit is a stepping stone towards resolving DEP0018. After this
commit, any code that includes an unhandledRejection hook will behave
unchanged when we change the default mode.

Refs: https://github.com/nodejs/node/pull/26599
---
 doc/api/cli.md                                | 18 ++++--
 lib/internal/process/promises.js              | 63 +++++++++----------
 .../message/promise_always_throw_unhandled.js |  4 +-
 test/parallel/test-promise-unhandled-error.js |  1 -
 test/parallel/test-promise-unhandled-warn.js  |  2 +-
 ...est-promises-unhandled-proxy-rejections.js |  5 +-
 ...st-promises-unhandled-symbol-rejections.js |  5 +-
 7 files changed, 53 insertions(+), 45 deletions(-)

diff --git a/doc/api/cli.md b/doc/api/cli.md
index b56a77143863d4..076f4aa4495f43 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -890,12 +890,20 @@ for the very first unhandled rejection in case no [`unhandledRejection`][] hook
 is used.
 
 Using this flag allows to change what should happen when an unhandled rejection
-occurs. One of three modes can be chosen:
+occurs and the [`unhandledRejection`][] hook is unset.
 
-* `strict`: Raise the unhandled rejection as an uncaught exception.
-* `warn`: Always trigger a warning, no matter if the [`unhandledRejection`][]
-  hook is set or not but do not print the deprecation warning.
-* `none`: Silence all warnings.
+One of three modes can be chosen:
+
+* `strict`: Emit [`unhandledRejection`][]. If this hook is not set, raise the
+  unhandled rejection as an uncaught exception.
+* `warn`: Emit [`unhandledRejection`][]. If this hook is not set, trigger a
+  warning. This is the default.
+* `none`: Emit [`unhandledRejection`][] and do nothing further.
+
+In all three modes, setting the [`unhandledRejection`][] hook will suppress
+the exception/warning. The hook implementation can raise an exception (as in
+`strict` mode), log a warning (as in `warn` mode), or do nothing (as in `none`
+mode).
 
 ### `--use-bundled-ca`, `--use-openssl-ca`
 <!-- YAML
diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js
index 1a0ed06f213df8..36ed60d3468f50 100644
--- a/lib/internal/process/promises.js
+++ b/lib/internal/process/promises.js
@@ -30,17 +30,17 @@ const pendingUnhandledRejections = [];
 const asyncHandledRejections = [];
 let lastPromiseId = 0;
 
-// --unhandled-rejection=none:
+// --unhandled-rejections=none:
 // Emit 'unhandledRejection', but do not emit any warning.
 const kIgnoreUnhandledRejections = 0;
-// --unhandled-rejection=warn:
-// Emit 'unhandledRejection', then emit 'UnhandledPromiseRejectionWarning'.
-const kAlwaysWarnUnhandledRejections = 1;
-// --unhandled-rejection=strict:
-// Emit 'uncaughtException'. If it's not handled, print the error to stderr
+// --unhandled-rejections=warn:
+// Emit 'unhandledRejection', if it's unhandled, emit
+// 'UnhandledPromiseRejectionWarning'.
+const kWarnUnhandledRejections = 1;
+// --unhandled-rejections=strict:
+// Emit 'unhandledRejection', if it's unhandled, emit
+// 'uncaughtException'. If it's not handled, print the error to stderr
 // and exit the process.
-// Otherwise, emit 'unhandledRejection'. If 'unhandledRejection' is not
-// handled, emit 'UnhandledPromiseRejectionWarning'.
 const kThrowUnhandledRejections = 2;
 // --unhandled-rejection is unset:
 // Emit 'unhandledRejection', if it's handled, emit
@@ -62,10 +62,10 @@ function getUnhandledRejectionsMode() {
   switch (getOptionValue('--unhandled-rejections')) {
     case 'none':
       return kIgnoreUnhandledRejections;
-    case 'warn':
-      return kAlwaysWarnUnhandledRejections;
     case 'strict':
       return kThrowUnhandledRejections;
+    case 'warn':
+      return kWarnUnhandledRejections;
     default:
       return kDefaultUnhandledRejections;
   }
@@ -138,7 +138,8 @@ function emitUnhandledRejectionWarning(uid, reason) {
     'Unhandled promise rejection. This error originated either by ' +
       'throwing inside of an async function without a catch block, ' +
       'or by rejecting a promise which was not handled with .catch(). ' +
-      'To terminate the node process on unhandled promise ' +
+      'To suppress this warning, use process.on("unhandledRejection"). ' +
+      'Or, to terminate the node process on unhandled promise ' +
       'rejection, use the CLI flag `--unhandled-rejections=strict` (see ' +
       'https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). ' +
       `(rejection id: ${uid})`
@@ -187,34 +188,30 @@ function processPromiseRejections() {
     }
     promiseInfo.warned = true;
     const { reason, uid } = promiseInfo;
-    switch (unhandledRejectionsMode) {
-      case kThrowUnhandledRejections: {
-        const err = reason instanceof Error ?
-          reason : generateUnhandledRejectionError(reason);
-        triggerUncaughtException(err, true /* fromPromise */);
-        const handled = process.emit('unhandledRejection', reason, promise);
-        if (!handled) emitUnhandledRejectionWarning(uid, reason);
-        break;
-      }
-      case kIgnoreUnhandledRejections: {
-        process.emit('unhandledRejection', reason, promise);
-        break;
-      }
-      case kAlwaysWarnUnhandledRejections: {
-        process.emit('unhandledRejection', reason, promise);
-        emitUnhandledRejectionWarning(uid, reason);
-        break;
-      }
-      case kDefaultUnhandledRejections: {
-        const handled = process.emit('unhandledRejection', reason, promise);
-        if (!handled) {
+    const handled = process.emit('unhandledRejection', reason, promise);
+    if (!handled) {
+      switch (unhandledRejectionsMode) {
+        case kThrowUnhandledRejections: {
+          const err = reason instanceof Error ?
+            reason : generateUnhandledRejectionError(reason);
+          triggerUncaughtException(err, true /* fromPromise */);
+          break;
+        }
+        case kIgnoreUnhandledRejections: {
+          break;
+        }
+        case kWarnUnhandledRejections: {
+          emitUnhandledRejectionWarning(uid, reason);
+          break;
+        }
+        case kDefaultUnhandledRejections: {
           emitUnhandledRejectionWarning(uid, reason);
           if (!deprecationWarned) {
             emitDeprecationWarning();
             deprecationWarned = true;
           }
+          break;
         }
-        break;
       }
     }
     maybeScheduledTicksOrMicrotasks = true;
diff --git a/test/message/promise_always_throw_unhandled.js b/test/message/promise_always_throw_unhandled.js
index d9128f34a5c9f1..c648022171cbfa 100644
--- a/test/message/promise_always_throw_unhandled.js
+++ b/test/message/promise_always_throw_unhandled.js
@@ -1,7 +1,9 @@
 // Flags: --unhandled-rejections=strict
 'use strict';
 
-require('../common');
+const common = require('../common');
+
+common.disableCrashOnUnhandledRejection();
 
 // Check that the process will exit on the first unhandled rejection in case the
 // unhandled rejections mode is set to `'strict'`.
diff --git a/test/parallel/test-promise-unhandled-error.js b/test/parallel/test-promise-unhandled-error.js
index bb906fdcbe7591..f2d2ced53a2a9b 100644
--- a/test/parallel/test-promise-unhandled-error.js
+++ b/test/parallel/test-promise-unhandled-error.js
@@ -34,7 +34,6 @@ const ref = new Promise(() => {
 Promise.reject(null);
 
 process.on('warning', common.mustNotCall('warning'));
-process.on('unhandledRejection', common.mustCall(2));
 process.on('rejectionHandled', common.mustNotCall('rejectionHandled'));
 process.on('exit', assert.strictEqual.bind(null, 0));
 
diff --git a/test/parallel/test-promise-unhandled-warn.js b/test/parallel/test-promise-unhandled-warn.js
index 62ddc69d8cd34b..7055e3cbea961e 100644
--- a/test/parallel/test-promise-unhandled-warn.js
+++ b/test/parallel/test-promise-unhandled-warn.js
@@ -16,7 +16,7 @@ Promise.reject('test');
 
 // Unhandled rejections trigger two warning per rejection. One is the rejection
 // reason and the other is a note where this warning is coming from.
-process.on('warning', common.mustCall(4));
+process.on('warning', common.mustNotCall());
 process.on('uncaughtException', common.mustNotCall('uncaughtException'));
 process.on('rejectionHandled', common.mustCall(2));
 
diff --git a/test/parallel/test-promises-unhandled-proxy-rejections.js b/test/parallel/test-promises-unhandled-proxy-rejections.js
index 3a4f2d1ba8b3ac..6d3ef46befbdef 100644
--- a/test/parallel/test-promises-unhandled-proxy-rejections.js
+++ b/test/parallel/test-promises-unhandled-proxy-rejections.js
@@ -12,8 +12,9 @@ const expectedPromiseWarning = ['Unhandled promise rejection. ' +
   'This error originated either by throwing ' +
   'inside of an async function without a catch ' +
   'block, or by rejecting a promise which was ' +
-  'not handled with .catch(). To terminate the ' +
-  'node process on unhandled promise rejection, ' +
+  'not handled with .catch(). To suppress this warning, ' +
+  'use process.on("unhandledRejection"). Or, to terminate ' +
+  'the node process on unhandled promise rejection, ' +
   'use the CLI flag `--unhandled-rejections=strict` (see ' +
   'https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). ' +
   '(rejection id: 1)'];
diff --git a/test/parallel/test-promises-unhandled-symbol-rejections.js b/test/parallel/test-promises-unhandled-symbol-rejections.js
index 2851ce977fc4d0..b64776236b0375 100644
--- a/test/parallel/test-promises-unhandled-symbol-rejections.js
+++ b/test/parallel/test-promises-unhandled-symbol-rejections.js
@@ -13,8 +13,9 @@ const expectedPromiseWarning = ['Unhandled promise rejection. ' +
   'This error originated either by throwing ' +
   'inside of an async function without a catch ' +
   'block, or by rejecting a promise which was ' +
-  'not handled with .catch(). To terminate the ' +
-  'node process on unhandled promise rejection, ' +
+  'not handled with .catch(). To suppress this warning, ' +
+  'use process.on("unhandledRejection"). Or, to terminate ' +
+  'the node process on unhandled promise rejection, ' +
   'use the CLI flag `--unhandled-rejections=strict` (see ' +
   'https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). ' +
   '(rejection id: 1)'];