Skip to content

Commit

Permalink
process: change default --unhandled-rejections=throw
Browse files Browse the repository at this point in the history
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: #33021
Fixes: #20392
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
  • Loading branch information
dfabulich authored and mmarchini committed Sep 22, 2020
1 parent 25d8456 commit 3b10f7f
Show file tree
Hide file tree
Showing 15 changed files with 73 additions and 97 deletions.
6 changes: 1 addition & 5 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
27 changes: 1 addition & 26 deletions lib/internal/process/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -86,7 +81,7 @@ function getUnhandledRejectionsMode() {
case 'warn-with-error-code':
return kWarnWithErrorCodeUnhandledRejections;
default:
return kDefaultUnhandledRejections;
return kThrowUnhandledRejections;
}
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
Expand Down
5 changes: 1 addition & 4 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
7 changes: 3 additions & 4 deletions test/es-module/test-esm-cjs-load-error-note.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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}`);
}));
Expand Down Expand Up @@ -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}`);
}));
4 changes: 1 addition & 3 deletions test/message/promise_unhandled_warn_with_error.js
Original file line number Diff line number Diff line change
@@ -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));
5 changes: 2 additions & 3 deletions test/message/unhandled_promise_trace_warnings.js
Original file line number Diff line number Diff line change
@@ -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(() => {}));
4 changes: 0 additions & 4 deletions test/message/unhandled_promise_trace_warnings.out
Original file line number Diff line number Diff line change
Expand Up @@ -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:*)
Expand Down
52 changes: 52 additions & 0 deletions test/parallel/test-promise-unhandled-default.js
Original file line number Diff line number Diff line change
@@ -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));
2 changes: 0 additions & 2 deletions test/parallel/test-promise-unhandled-throw.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
5 changes: 1 addition & 4 deletions test/parallel/test-promise-unhandled-warn-no-hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
20 changes: 1 addition & 19 deletions test/parallel/test-promises-unhandled-proxy-rejections.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand All @@ -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);
9 changes: 4 additions & 5 deletions test/parallel/test-promises-unhandled-rejections.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
},
Expand Down
7 changes: 1 addition & 6 deletions test/parallel/test-promises-unhandled-symbol-rejections.js
Original file line number Diff line number Diff line change
@@ -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 ' +
Expand All @@ -20,7 +16,6 @@ const expectedPromiseWarning = ['Unhandled promise rejection. ' +
'(rejection id: 1)'];

common.expectWarning({
DeprecationWarning: expectedDeprecationWarning,
UnhandledPromiseRejectionWarning: [
expectedValueWarning,
expectedPromiseWarning
Expand Down
14 changes: 4 additions & 10 deletions test/parallel/test-promises-warning-on-unhandled-rejection.js
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -7,8 +7,6 @@
const common = require('../common');
const assert = require('assert');

common.disableCrashOnUnhandledRejection();

let b = 0;

process.on('warning', common.mustCall((warning) => {
Expand All @@ -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(
Expand All @@ -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(() => { })));
Expand Down
3 changes: 1 addition & 2 deletions test/sequential/test-inspector-async-call-stack-abort.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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);
}

0 comments on commit 3b10f7f

Please sign in to comment.