process: Throw exception on --unhandled-rejections=default

This is a semver-major change that resolves DEP0018. This PR defines a new mode for the --unhandled-rejections flag, called "default". The "default" mode first emits unhandledRejection. If this hook is not set, the "default" mode will raise the unhandled rejection as an uncaught exception. This mirrors the behavior of the current (unnamed) default mode for --unhandled-rejections, which behaves nearly like "warn" mode. The current default behavior is to emit unhandledRejection; if this hook is not set, the unnamed default mode logs a warning and a deprecation notice. (In comparison, "warn" mode always logs a warning, regardless of whether the hook is set.) All users that have set an unhandledRejection hook or set a non-default value for the --unhandled-rejections flag will see no change in behavior after this change. Fixes: #20392 Refs: https://nodejs.org/dist/latest/docs/api/deprecations.html#deprecations_dep0018_unhandled_promise_rejections
nodejs · Apr 23, 2020 · 3814c75 · 3814c75
1 parent 91ca221
commit 3814c75
Show file tree

Hide file tree

Showing 14 changed files with 34 additions and 85 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -885,13 +885,11 @@ Track heap object allocations for heap snapshots.
 added: v12.0.0
 -->
 
-By default all unhandled rejections trigger a warning plus a deprecation warning
-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:
 
+* `default`: Emit [`unhandledRejection`][]. If this hook is not set, raise the
+  unhandled rejection as an uncaught exception. This is the default.
 * `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.

diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js
@@ -30,21 +30,22 @@ 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:
+// --unhandled-rejections=warn:
 // Emit 'unhandledRejection', then emit 'UnhandledPromiseRejectionWarning'.
 const kAlwaysWarnUnhandledRejections = 1;
-// --unhandled-rejection=strict:
+// --unhandled-rejections=strict:
 // 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
-// 'UnhandledPromiseRejectionWarning', then emit deprecation warning.
+// Emit 'unhandledRejection', if it's unhandled, emit
+// 'uncaughtException'. If it's not handled, print the error to stderr
+// and exit the process.
 const kDefaultUnhandledRejections = 3;
 
 let unhandledRejectionsMode;
@@ -156,15 +157,6 @@ function emitUnhandledRejectionWarning(uid, reason) {
   process.emitWarning(warning);
 }
 
-let deprecationWarned = false;
-function emitDeprecationWarning() {
-  process.emitWarning(
-    'Unhandled promise rejections are deprecated. In the future, ' +
-    'promise rejections that are not handled will terminate the ' +
-    'Node.js process with a non-zero exit code.',
-    'DeprecationWarning', 'DEP0018');
-}
-
 // If this method returns true, we've executed user code or triggered
 // a warning to be emitted which requires the microtask and next tick
 // queues to be drained again.
@@ -208,11 +200,9 @@ function processPromiseRejections() {
       case kDefaultUnhandledRejections: {
         const handled = process.emit('unhandledRejection', reason, promise);
         if (!handled) {
-          emitUnhandledRejectionWarning(uid, reason);
-          if (!deprecationWarned) {
-            emitDeprecationWarning();
-            deprecationWarned = true;
-          }
+          const err = reason instanceof Error ?
+            reason : generateUnhandledRejectionError(reason);
+          triggerUncaughtException(err, true /* fromPromise */);
         }
         break;
       }

diff --git a/src/node_options.cc b/src/node_options.cc
@@ -119,6 +119,7 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
   }
 
   if (!unhandled_rejections.empty() &&
+      unhandled_rejections != "default" &&
       unhandled_rejections != "strict" &&
       unhandled_rejections != "warn" &&
       unhandled_rejections != "none") {

diff --git a/test/common/index.js b/test/common/index.js
@@ -596,11 +596,8 @@ function getBufferSources(buf) {
   return [...getArrayBufferViews(buf), new Uint8Array(buf).buffer];
 }
 
-// Crash the process on unhandled rejections.
-const crashOnUnhandledRejection = (err) => { throw err; };
-process.on('unhandledRejection', crashOnUnhandledRejection);
 function disableCrashOnUnhandledRejection() {
-  process.removeListener('unhandledRejection', crashOnUnhandledRejection);
+  process.on('unhandledRejection', () => {});
 }
 
 function getTTYfd() {

diff --git a/test/es-module/test-esm-cjs-load-error-note.mjs b/test/es-module/test-esm-cjs-load-error-note.mjs
@@ -63,8 +63,7 @@ pImport2.stderr.on('data', (data) => {
   pImport2Stderr += data;
 });
 pImport2.on('close', mustCall((code) => {
-  // As this exits normally we expect 0
-  assert.strictEqual(code, 0);
+  assert.strictEqual(code, expectedCode);
   assert.ok(!pImport2Stderr.includes(expectedNote),
             `${expectedNote} must not be included in ${pImport2Stderr}`);
 }));
@@ -94,15 +93,15 @@ pImport4.on('close', mustCall((code) => {
             `${expectedNote} not found in ${pImport4Stderr}`);
 }));
 
-// Must exit with zero and show note
+// Must exit non-zero and show note
 const pImport5 = spawn(process.execPath, [Import5]);
 let pImport5Stderr = '';
 pImport5.stderr.setEncoding('utf8');
 pImport5.stderr.on('data', (data) => {
   pImport5Stderr += data;
 });
 pImport5.on('close', mustCall((code) => {
-  assert.strictEqual(code, 0);
+  assert.strictEqual(code, expectedCode);
   assert.ok(!pImport5Stderr.includes(expectedNote),
             `${expectedNote} must not be included in ${pImport5Stderr}`);
 }));
diff --git a/test/message/promise_always_throw_unhandled.js b/test/message/promise_always_throw_unhandled.js
@@ -1,7 +1,7 @@
 // Flags: --unhandled-rejections=strict
 'use strict';
 
-require('../common');
+const common = require('../common');
 
 // 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/message/unhandled_promise_trace_warnings.js b/test/message/unhandled_promise_trace_warnings.js
@@ -1,6 +1,5 @@
-// Flags: --trace-warnings
+// Flags: --trace-warnings --unhandled-rejections=warn
 'use strict';
 const common = require('../common');
-common.disableCrashOnUnhandledRejection();
 const p = Promise.reject(new Error('This was rejected'));
 setImmediate(() => p.catch(() => {}));
diff --git a/test/message/unhandled_promise_trace_warnings.out b/test/message/unhandled_promise_trace_warnings.out
@@ -17,10 +17,6 @@
     at *
     at *
     at *
-(node:*) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
-    at *
-    at *
-    at *
 (node:*) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)
     at handledRejection (internal/process/promises.js:*)
     at promiseRejectHandler (internal/process/promises.js:*)

diff --git a/test/parallel/test-promise-unhandled-warn-no-hook.js b/test/parallel/test-promise-unhandled-warn-no-hook.js
@@ -3,10 +3,7 @@
 
 const common = require('../common');
 
-common.disableCrashOnUnhandledRejection();
-
-// Verify that ignoring unhandled rejection works fine and that no warning is
-// logged.
+// Verify that --unhandled-rejections=warn works fine
 
 new Promise(() => {
   throw new Error('One');

diff --git a/test/parallel/test-promises-unhandled-proxy-rejections.js b/test/parallel/test-promises-unhandled-proxy-rejections.js
@@ -3,21 +3,6 @@ const common = require('../common');
 
 common.disableCrashOnUnhandledRejection();
 
-const expectedDeprecationWarning = ['Unhandled promise rejections are ' +
-                                   'deprecated. In the future, promise ' +
-                                   'rejections that are not handled will ' +
-                                   'terminate the Node.js process with a ' +
-                                   'non-zero exit code.', 'DEP0018'];
-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, ' +
-  'use the CLI flag `--unhandled-rejections=strict` (see ' +
-  'https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). ' +
-  '(rejection id: 1)'];
-
 function throwErr() {
   throw new Error('Error from proxy');
 }
@@ -38,10 +23,7 @@ const thorny = new Proxy({}, {
   construct: throwErr
 });
 
-common.expectWarning({
-  DeprecationWarning: expectedDeprecationWarning,
-  UnhandledPromiseRejectionWarning: expectedPromiseWarning,
-});
+process.on('warning', common.mustNotCall());
 
 // Ensure this doesn't crash
 Promise.reject(thorny);
diff --git a/test/parallel/test-promises-unhandled-rejections.js b/test/parallel/test-promises-unhandled-rejections.js
@@ -668,6 +668,7 @@ asyncTest('Throwing an error inside a rejectionHandled handler goes to' +
           ' unhandledException, and does not cause .catch() to throw an ' +
           'exception', function(done) {
   clean();
+  common.disableCrashOnUnhandledRejection();
   const e = new Error();
   const e2 = new Error();
   const tearDownException = setupException(function(err) {
@@ -702,19 +703,20 @@ asyncTest('Rejected promise inside unhandledRejection allows nextTick loop' +
 });
 
 asyncTest(
-  'Unhandled promise rejection emits a warning immediately',
+  'Promise rejection triggers unhandledRejection immediately',
   function(done) {
     clean();
-    Promise.reject(0);
-    const { emitWarning } = process;
-    process.emitWarning = common.mustCall((...args) => {
+    process.on('unhandledRejection', common.mustCall((err) => {
       if (timer) {
         clearTimeout(timer);
         timer = null;
         done();
       }
-      emitWarning(...args);
-    }, 2);
+    }));
+    try {
+      Promise.reject(0);
+      assert(false);
+    } catch (e) {}
 
     let timer = setTimeout(common.mustNotCall(), 10000);
   },

diff --git a/test/parallel/test-promises-unhandled-symbol-rejections.js b/test/parallel/test-promises-unhandled-symbol-rejections.js
@@ -1,14 +1,10 @@
+// Flags: --unhandled-rejections=warn
 'use strict';
 const common = require('../common');
 
 common.disableCrashOnUnhandledRejection();
 
 const expectedValueWarning = ['Symbol()'];
-const expectedDeprecationWarning = ['Unhandled promise rejections are ' +
-                                   'deprecated. In the future, promise ' +
-                                   'rejections that are not handled will ' +
-                                   'terminate the Node.js process with a ' +
-                                   'non-zero exit code.', 'DEP0018'];
 const expectedPromiseWarning = ['Unhandled promise rejection. ' +
   'This error originated either by throwing ' +
   'inside of an async function without a catch ' +
@@ -20,7 +16,6 @@ const expectedPromiseWarning = ['Unhandled promise rejection. ' +
   '(rejection id: 1)'];
 
 common.expectWarning({
-  DeprecationWarning: expectedDeprecationWarning,
   UnhandledPromiseRejectionWarning: [
     expectedValueWarning,
     expectedPromiseWarning

diff --git a/test/parallel/test-promises-warning-on-unhandled-rejection.js b/test/parallel/test-promises-warning-on-unhandled-rejection.js
@@ -1,4 +1,4 @@
-// Flags: --no-warnings
+// Flags: --no-warnings --unhandled-rejections=warn
 'use strict';
 
 // Test that warnings are emitted when a Promise experiences an uncaught
@@ -7,8 +7,6 @@
 const common = require('../common');
 const assert = require('assert');
 
-common.disableCrashOnUnhandledRejection();
-
 let b = 0;
 
 process.on('warning', common.mustCall((warning) => {
@@ -27,14 +25,10 @@ process.on('warning', common.mustCall((warning) => {
       );
       break;
     case 2:
-      // One time deprecation warning, first unhandled rejection
-      assert.strictEqual(warning.name, 'DeprecationWarning');
-      break;
-    case 3:
       // Number rejection error displayed. Note it's been stringified
       assert.strictEqual(warning.message, '42');
       break;
-    case 4:
+    case 3:
       // Unhandled rejection warning (won't be handled next tick)
       assert.strictEqual(warning.name, 'UnhandledPromiseRejectionWarning');
       assert(
@@ -43,13 +37,13 @@ process.on('warning', common.mustCall((warning) => {
         `but did not. Had "${warning.message}" instead.`
       );
       break;
-    case 5:
+    case 4:
       // Rejection handled asynchronously.
       assert.strictEqual(warning.name, 'PromiseRejectionHandledWarning');
       assert(/Promise rejection was handled asynchronously/
         .test(warning.message));
   }
-}, 6));
+}, 5));
 
 const p = Promise.reject('This was rejected'); // Reject with a string
 setImmediate(common.mustCall(() => p.catch(() => { })));

diff --git a/test/sequential/test-inspector-async-call-stack-abort.js b/test/sequential/test-inspector-async-call-stack-abort.js
@@ -9,7 +9,6 @@ const { strictEqual } = require('assert');
 const eyecatcher = 'nou, houdoe he?';
 
 if (process.argv[2] === 'child') {
-  common.disableCrashOnUnhandledRejection();
   const { Session } = require('inspector');
   const { promisify } = require('util');
   const { internalBinding } = require('internal/test/binding');
@@ -31,7 +30,7 @@ if (process.argv[2] === 'child') {
   const options = { encoding: 'utf8' };
   const proc = spawnSync(
     process.execPath, ['--expose-internals', __filename, 'child'], options);
-  strictEqual(proc.status, 0);
+  strictEqual(proc.status, 1);
   strictEqual(proc.signal, null);
   strictEqual(proc.stderr.includes(eyecatcher), true);
 }