From cccb8fa7b2a02698d49a0da80e41a1033e477db5 Mon Sep 17 00:00:00 2001 From: Dan Fabulich Date: Thu, 23 Apr 2020 00:17:18 -0700 Subject: [PATCH] process: change default --unhandled-rejections=throw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a semver-major change that resolves DEP0018. 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. Refs: https://nodejs.org/dist/latest/docs/api/deprecations.html#deprecations_dep0018_unhandled_promise_rejections PR-URL: https://github.com/nodejs/node/pull/33021 Fixes: https://github.com/nodejs/node/issues/20392 Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: Mary Marchini Reviewed-By: Shelley Vohr Reviewed-By: Michael Dawson Reviewed-By: Anatoli Papirovski Reviewed-By: Сковорода Никита Андреевич --- doc/api/cli.md | 6 +-- lib/internal/process/promises.js | 27 +--------- test/common/index.js | 5 +- .../test-esm-cjs-load-error-note.mjs | 7 ++- .../promise_unhandled_warn_with_error.js | 4 +- .../unhandled_promise_trace_warnings.js | 5 +- .../unhandled_promise_trace_warnings.out | 4 -- .../test-promise-unhandled-default.js | 52 +++++++++++++++++++ test/parallel/test-promise-unhandled-throw.js | 2 - .../test-promise-unhandled-warn-no-hook.js | 5 +- ...est-promises-unhandled-proxy-rejections.js | 20 +------ .../test-promises-unhandled-rejections.js | 9 ++-- ...st-promises-unhandled-symbol-rejections.js | 7 +-- ...promises-warning-on-unhandled-rejection.js | 14 ++--- .../test-inspector-async-call-stack-abort.js | 3 +- 15 files changed, 73 insertions(+), 97 deletions(-) create mode 100644 test/parallel/test-promise-unhandled-default.js diff --git a/doc/api/cli.md b/doc/api/cli.md index 2eb189a743b49d..c92cd21e8ebe04 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -1007,15 +1007,11 @@ added: - v10.17.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 the following modes can be chosen: * `throw`: Emit [`unhandledRejection`][]. If this hook is not set, raise the - unhandled rejection as an uncaught exception. + 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 f2145d425c620f..2b806e7e8e41b4 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -57,11 +57,6 @@ const kThrowUnhandledRejections = 3; const kWarnWithErrorCodeUnhandledRejections = 4; -// --unhandled-rejections is unset: -// Emit 'unhandledRejection', if it's unhandled, emit -// 'UnhandledPromiseRejectionWarning', then emit deprecation warning. -const kDefaultUnhandledRejections = 5; - let unhandledRejectionsMode; function setHasRejectionToWarn(value) { @@ -86,7 +81,7 @@ function getUnhandledRejectionsMode() { case 'warn-with-error-code': return kWarnWithErrorCodeUnhandledRejections; default: - return kDefaultUnhandledRejections; + return kThrowUnhandledRejections; } } @@ -175,15 +170,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. @@ -241,17 +227,6 @@ function processPromiseRejections() { } break; } - case kDefaultUnhandledRejections: { - const handled = process.emit('unhandledRejection', reason, promise); - if (!handled) { - emitUnhandledRejectionWarning(uid, reason); - if (!deprecationWarned) { - emitDeprecationWarning(); - deprecationWarned = true; - } - } - break; - } } maybeScheduledTicksOrMicrotasks = true; } diff --git a/test/common/index.js b/test/common/index.js index 4c0c0f86d9820e..b1c25d7591ac9f 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -599,11 +599,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_unhandled_warn_with_error.js b/test/message/promise_unhandled_warn_with_error.js index 0f22e8deeba653..e07f52039101a3 100644 --- a/test/message/promise_unhandled_warn_with_error.js +++ b/test/message/promise_unhandled_warn_with_error.js @@ -1,10 +1,8 @@ // Flags: --unhandled-rejections=warn-with-error-code 'use strict'; -const common = require('../common'); +require('../common'); const assert = require('assert'); -common.disableCrashOnUnhandledRejection(); - Promise.reject(new Error('alas')); process.on('exit', assert.strictEqual.bind(null, 1)); diff --git a/test/message/unhandled_promise_trace_warnings.js b/test/message/unhandled_promise_trace_warnings.js index d51a2cd3da2106..66c1f39e5de091 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(); +require('../common'); 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-default.js b/test/parallel/test-promise-unhandled-default.js new file mode 100644 index 00000000000000..f69a039bed21da --- /dev/null +++ b/test/parallel/test-promise-unhandled-default.js @@ -0,0 +1,52 @@ +'use strict'; + +const common = require('../common'); +const Countdown = require('../common/countdown'); +const assert = require('assert'); + +// Verify that unhandled rejections always trigger uncaught exceptions instead +// of triggering unhandled rejections. + +const err1 = new Error('One'); +const err2 = new Error( + '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(). The promise rejected with the' + + ' reason "null".' +); +err2.code = 'ERR_UNHANDLED_REJECTION'; +Object.defineProperty(err2, 'name', { + value: 'UnhandledPromiseRejection', + writable: true, + configurable: true +}); + +const errors = [err1, err2]; +const identical = [true, false]; + +const ref = new Promise(() => { + throw err1; +}); +// Explicitly reject `null`. +Promise.reject(null); + +process.on('warning', common.mustNotCall('warning')); +// If we add an unhandledRejection handler, the exception won't be thrown +// process.on('unhandledRejection', common.mustCall(2)); +process.on('rejectionHandled', common.mustNotCall('rejectionHandled')); +process.on('exit', assert.strictEqual.bind(null, 0)); + +const timer = setTimeout(() => console.log(ref), 1000); + +const counter = new Countdown(2, () => { + clearTimeout(timer); +}); + +process.on('uncaughtException', common.mustCall((err, origin) => { + counter.dec(); + assert.strictEqual(origin, 'unhandledRejection', err); + const knownError = errors.shift(); + assert.deepStrictEqual(err, knownError); + // Check if the errors are reference equal. + assert(identical.shift() ? err === knownError : err !== knownError); +}, 2)); diff --git a/test/parallel/test-promise-unhandled-throw.js b/test/parallel/test-promise-unhandled-throw.js index 30c6b87135b49b..b9e57fb8bff412 100644 --- a/test/parallel/test-promise-unhandled-throw.js +++ b/test/parallel/test-promise-unhandled-throw.js @@ -5,8 +5,6 @@ const common = require('../common'); const Countdown = require('../common/countdown'); const assert = require('assert'); -common.disableCrashOnUnhandledRejection(); - // Verify that unhandled rejections always trigger uncaught exceptions instead // of triggering unhandled rejections. 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..bfde806b0572ec 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,17 @@ 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); + })); 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 a3d0944ce5ecbb..64eb2bd0ac8974 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); }