From a29fe914f116b290c85927777740663043014b9a Mon Sep 17 00:00:00 2001 From: Madara Uchiha Date: Mon, 20 Nov 2017 21:22:37 +0200 Subject: [PATCH 1/6] Improved unhandled rejection message Fixes #16768 --- lib/internal/process/promises.js | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index 663e6f5fec6985..7710bfaf5f118f 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -1,7 +1,5 @@ 'use strict'; -const { safeToString } = process.binding('util'); - const promiseRejectEvent = process._promiseRejectEvent; const hasBeenNotifiedProperty = new WeakMap(); const promiseToGuidProperty = new WeakMap(); @@ -60,11 +58,23 @@ function setupPromises(scheduleMicrotasks) { } function emitWarning(uid, reason) { + try { + if (reason instanceof Error) { + process.emitWarning(reason.stack, 'UnhandledPromiseRejectionWarning'); + } else { + process.emitWarning(reason, 'UnhandledPromiseRejectionWarning'); + } + } catch (e) { + // ignored + } + const warning = new Error( - `Unhandled promise rejection (rejection id: ${uid}): ` + - safeToString(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(). ' + + `(rejection id: ${uid})` + ); warning.name = 'UnhandledPromiseRejectionWarning'; - warning.id = uid; try { if (reason instanceof Error) { warning.stack = reason.stack; From ba4adc55f8c5a7ee2879ba9f544bb336e755276d Mon Sep 17 00:00:00 2001 From: Madara Uchiha Date: Mon, 20 Nov 2017 21:23:06 +0200 Subject: [PATCH 2/6] Make tests pass For unhandled rejection error message change. --- .../unhandled_promise_trace_warnings.out | 18 ++++++++++++++++++ ...test-promises-unhandled-proxy-rejections.js | 7 +++++-- ...est-promises-unhandled-symbol-rejections.js | 7 +++++-- ...-promises-warning-on-unhandled-rejection.js | 11 +++++++---- 4 files changed, 35 insertions(+), 8 deletions(-) diff --git a/test/message/unhandled_promise_trace_warnings.out b/test/message/unhandled_promise_trace_warnings.out index 77e7edcf4c6d88..410b3bf4e36d41 100644 --- a/test/message/unhandled_promise_trace_warnings.out +++ b/test/message/unhandled_promise_trace_warnings.out @@ -1,3 +1,21 @@ +(node:*) UnhandledPromiseRejectionWarning: Error: This was rejected + at * (*test*message*unhandled_promise_trace_warnings.js:*) + at * + at * + at * + at * + at * + at * + at * + at * + at * + at * + at * + at * + at * + at * + at * + at * (node:*) Error: This was rejected at * (*test*message*unhandled_promise_trace_warnings.js:*) at * diff --git a/test/parallel/test-promises-unhandled-proxy-rejections.js b/test/parallel/test-promises-unhandled-proxy-rejections.js index 91808af14588d3..f632857d6373ba 100644 --- a/test/parallel/test-promises-unhandled-proxy-rejections.js +++ b/test/parallel/test-promises-unhandled-proxy-rejections.js @@ -6,8 +6,11 @@ const expectedDeprecationWarning = 'Unhandled promise rejections are ' + 'rejections that are not handled will ' + 'terminate the Node.js process with a ' + 'non-zero exit code.'; -const expectedPromiseWarning = 'Unhandled promise rejection (rejection id: ' + - '1): [object Object]'; +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(). (rejection id: 1)'; function throwErr() { throw new Error('Error from proxy'); diff --git a/test/parallel/test-promises-unhandled-symbol-rejections.js b/test/parallel/test-promises-unhandled-symbol-rejections.js index a60a5f2e8aac30..3e06d7edad27b4 100644 --- a/test/parallel/test-promises-unhandled-symbol-rejections.js +++ b/test/parallel/test-promises-unhandled-symbol-rejections.js @@ -6,8 +6,11 @@ const expectedDeprecationWarning = 'Unhandled promise rejections are ' + 'rejections that are not handled will ' + 'terminate the Node.js process with a ' + 'non-zero exit code.'; -const expectedPromiseWarning = 'Unhandled promise rejection (rejection id: ' + - '1): Symbol()'; +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(). (rejection id: 1)'; common.expectWarning({ DeprecationWarning: expectedDeprecationWarning, diff --git a/test/parallel/test-promises-warning-on-unhandled-rejection.js b/test/parallel/test-promises-warning-on-unhandled-rejection.js index 10f95162a09597..0e16ef58438f10 100644 --- a/test/parallel/test-promises-warning-on-unhandled-rejection.js +++ b/test/parallel/test-promises-warning-on-unhandled-rejection.js @@ -12,18 +12,21 @@ let b = 0; process.on('warning', common.mustCall((warning) => { switch (b++) { case 0: - assert.strictEqual(warning.name, 'UnhandledPromiseRejectionWarning'); - assert(/Unhandled promise rejection/.test(warning.message)); + assert.strictEqual(warning.message, 'This was rejected'); break; case 1: - assert.strictEqual(warning.name, 'DeprecationWarning'); + assert.strictEqual(warning.name, 'UnhandledPromiseRejectionWarning'); + assert(/Unhandled promise rejection/.test(warning.message), 'Expected warning message to contain "Unhandled promise rejection" but did not. Had "' + warning.message + '" instead.'); break; case 2: + assert.strictEqual(warning.name, 'DeprecationWarning'); + break; + case 3: assert.strictEqual(warning.name, 'PromiseRejectionHandledWarning'); assert(/Promise rejection was handled asynchronously/ .test(warning.message)); } -}, 3)); +}, 4)); const p = Promise.reject('This was rejected'); setImmediate(common.mustCall(() => p.catch(() => {}))); From ab45e60a9a4bb8a03380e8616ed23a4a79133a35 Mon Sep 17 00:00:00 2001 From: Madara Uchiha Date: Tue, 21 Nov 2017 00:38:18 +0200 Subject: [PATCH 3/6] Wrap reason with unsafeToString() if it's a string --- lib/internal/process/promises.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index 7710bfaf5f118f..82f15f9f9df51e 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -1,5 +1,7 @@ 'use strict'; +const { safeToString } = process.binding('util'); + const promiseRejectEvent = process._promiseRejectEvent; const hasBeenNotifiedProperty = new WeakMap(); const promiseToGuidProperty = new WeakMap(); @@ -62,7 +64,9 @@ function setupPromises(scheduleMicrotasks) { if (reason instanceof Error) { process.emitWarning(reason.stack, 'UnhandledPromiseRejectionWarning'); } else { - process.emitWarning(reason, 'UnhandledPromiseRejectionWarning'); + process.emitWarning( + safeToString(reason), 'UnhandledPromiseRejectionWarning' + ); } } catch (e) { // ignored From bf06851232ab25edf5bb753efd8b6c6231b73045 Mon Sep 17 00:00:00 2001 From: Madara Uchiha Date: Tue, 21 Nov 2017 21:41:51 +0200 Subject: [PATCH 4/6] Add test for checking warnings about rejected numbers. Also clean up the test a bit and add comments --- ...promises-warning-on-unhandled-rejection.js | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-promises-warning-on-unhandled-rejection.js b/test/parallel/test-promises-warning-on-unhandled-rejection.js index 0e16ef58438f10..eade8ed4ff01a6 100644 --- a/test/parallel/test-promises-warning-on-unhandled-rejection.js +++ b/test/parallel/test-promises-warning-on-unhandled-rejection.js @@ -12,21 +12,35 @@ let b = 0; process.on('warning', common.mustCall((warning) => { switch (b++) { case 0: + // String rejection error displayed assert.strictEqual(warning.message, 'This was rejected'); break; case 1: + // Warning about rejection not being handled (will be next tick) assert.strictEqual(warning.name, 'UnhandledPromiseRejectionWarning'); assert(/Unhandled promise rejection/.test(warning.message), 'Expected warning message to contain "Unhandled promise rejection" but did not. Had "' + warning.message + '" instead.'); 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: + // Unhandled rejection warning (won't be handled next tick) + assert.strictEqual(warning.name, 'UnhandledPromiseRejectionWarning'); + assert(/Unhandled promise rejection/.test(warning.message), 'Expected warning message to contain "Unhandled promise rejection" but did not. Had "' + warning.message + '" instead.'); + break; + case 5: + // Rejection handled asynchronously. assert.strictEqual(warning.name, 'PromiseRejectionHandledWarning'); assert(/Promise rejection was handled asynchronously/ .test(warning.message)); } -}, 4)); +}, 6)); -const p = Promise.reject('This was rejected'); -setImmediate(common.mustCall(() => p.catch(() => {}))); +const p = Promise.reject('This was rejected'); // Reject with a string +setImmediate(common.mustCall(() => p.catch(() => { }))); +Promise.reject(42); // Reject with a number From 9d29f0c12d29d868b1543608aa51fc12f9a405bb Mon Sep 17 00:00:00 2001 From: Madara Uchiha Date: Tue, 21 Nov 2017 21:49:10 +0200 Subject: [PATCH 5/6] Fix failing symbol rejection test. Now checks correctly that Symbol() is outputted when rejection happens. --- test/parallel/test-promises-unhandled-symbol-rejections.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-promises-unhandled-symbol-rejections.js b/test/parallel/test-promises-unhandled-symbol-rejections.js index 3e06d7edad27b4..3e687d4e49bf08 100644 --- a/test/parallel/test-promises-unhandled-symbol-rejections.js +++ b/test/parallel/test-promises-unhandled-symbol-rejections.js @@ -1,6 +1,7 @@ 'use strict'; const common = require('../common'); +const expectedValueWarning = 'Symbol()'; const expectedDeprecationWarning = 'Unhandled promise rejections are ' + 'deprecated. In the future, promise ' + 'rejections that are not handled will ' + @@ -14,7 +15,10 @@ const expectedPromiseWarning = 'Unhandled promise rejection. ' + common.expectWarning({ DeprecationWarning: expectedDeprecationWarning, - UnhandledPromiseRejectionWarning: expectedPromiseWarning, + UnhandledPromiseRejectionWarning: [ + expectedPromiseWarning, + expectedValueWarning + ], }); // ensure this doesn't crash From 481c7f11edb797e39dddb153c8aa42af105c2a94 Mon Sep 17 00:00:00 2001 From: Madara Uchiha Date: Wed, 22 Nov 2017 22:22:51 +0200 Subject: [PATCH 6/6] Make test lines wrap at 80 characters or less. --- .../test-promises-warning-on-unhandled-rejection.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-promises-warning-on-unhandled-rejection.js b/test/parallel/test-promises-warning-on-unhandled-rejection.js index eade8ed4ff01a6..3ac7d8698beb37 100644 --- a/test/parallel/test-promises-warning-on-unhandled-rejection.js +++ b/test/parallel/test-promises-warning-on-unhandled-rejection.js @@ -18,7 +18,11 @@ process.on('warning', common.mustCall((warning) => { case 1: // Warning about rejection not being handled (will be next tick) assert.strictEqual(warning.name, 'UnhandledPromiseRejectionWarning'); - assert(/Unhandled promise rejection/.test(warning.message), 'Expected warning message to contain "Unhandled promise rejection" but did not. Had "' + warning.message + '" instead.'); + assert( + /Unhandled promise rejection/.test(warning.message), + 'Expected warning message to contain "Unhandled promise rejection" ' + + 'but did not. Had "' + warning.message + '" instead.' + ); break; case 2: // One time deprecation warning, first unhandled rejection @@ -31,7 +35,11 @@ process.on('warning', common.mustCall((warning) => { case 4: // Unhandled rejection warning (won't be handled next tick) assert.strictEqual(warning.name, 'UnhandledPromiseRejectionWarning'); - assert(/Unhandled promise rejection/.test(warning.message), 'Expected warning message to contain "Unhandled promise rejection" but did not. Had "' + warning.message + '" instead.'); + assert( + /Unhandled promise rejection/.test(warning.message), + 'Expected warning message to contain "Unhandled promise rejection" ' + + 'but did not. Had "' + warning.message + '" instead.' + ); break; case 5: // Rejection handled asynchronously.