From 3814c75b15226b0c38b53b1985366e560876e04e Mon Sep 17 00:00:00 2001 From: Dan Fabulich Date: Thu, 23 Apr 2020 00:17:18 -0700 Subject: [PATCH] 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 --- doc/api/cli.md | 6 ++-- lib/internal/process/promises.js | 28 ++++++------------- src/node_options.cc | 1 + test/common/index.js | 5 +--- .../test-esm-cjs-load-error-note.mjs | 7 ++--- .../message/promise_always_throw_unhandled.js | 2 +- .../unhandled_promise_trace_warnings.js | 3 +- .../unhandled_promise_trace_warnings.out | 4 --- .../test-promise-unhandled-warn-no-hook.js | 5 +--- ...est-promises-unhandled-proxy-rejections.js | 20 +------------ .../test-promises-unhandled-rejections.js | 14 ++++++---- ...st-promises-unhandled-symbol-rejections.js | 7 +---- ...promises-warning-on-unhandled-rejection.js | 14 +++------- .../test-inspector-async-call-stack-abort.js | 3 +- 14 files changed, 34 insertions(+), 85 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index b56a77143863d4..9ea7a625d39714 100644 --- 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 index 1a0ed06f213df8..a5da2b7797c6ca 100644 --- 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 index 3b9142c19e98a8..edc025de7de18e 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -119,6 +119,7 @@ void EnvironmentOptions::CheckOptions(std::vector* 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 index e77e7b95059947..40f7bed14d2c47 100644 --- 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 index c0ac9393a8ddd5..0d0b1f37fbd995 100644 --- 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,7 +93,7 @@ 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'); @@ -102,7 +101,7 @@ 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 index d9128f34a5c9f1..ab3dd815e7434d 100644 --- 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 index d51a2cd3da2106..35f533135ed3e3 100644 --- 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 index c22dfbe0fcc8fa..5100eecc7e55e7 100644 --- 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 index 91784448ad01ce..850f327b0f0302 100644 --- 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 index 3a4f2d1ba8b3ac..099df95691d466 100644 --- 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 index 86a4370c518c56..9f07749ebe27f1 100644 --- 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 index 2851ce977fc4d0..cfcce5b148163d 100644 --- 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 index 596bf27ad1917d..0d6a74854217c8 100644 --- 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 index ce2b43cdf02e82..cf4693be8dd73a 100644 --- 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); }