From 0755912d25e67871d3b41f8466a92b98ad5cd5d6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 9 Aug 2018 21:06:32 +0200 Subject: [PATCH 1/3] process: add `multipleResolves` event This adds the `multipleResolves` event to track promises that resolve more than once or that reject after resolving. It is important to expose this to the user to make sure the application runs as expected. Without such warnings it would be very hard to debug these situations. Fixes: https://github.com/nodejs/promises-debugging/issues/8 --- doc/api/process.md | 54 ++++++++++++++++++ lib/internal/process/promises.js | 40 +++++++++----- src/bootstrapper.cc | 55 ++++++++++++------- src/env.h | 3 +- .../unhandled_promise_trace_warnings.out | 1 + test/parallel/test-promise-swallowed-event.js | 32 +++++++++++ 6 files changed, 150 insertions(+), 35 deletions(-) create mode 100644 test/parallel/test-promise-swallowed-event.js diff --git a/doc/api/process.md b/doc/api/process.md index a2e052e4779d97..169e58ccf5f5fb 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -97,6 +97,60 @@ the child process. The message goes through serialization and parsing. The resulting message might not be the same as what is originally sent. +### Event: 'multipleResolves' + + +* `type` {string} The error type. One of `'resolveAfterResolved'` or + `'rejectAfterResolved'`. +* `promise` {Promise} The promise that resolved or rejected more than once. +* `value` {any} The value with which the promise was either resolved or + rejected after the original resolve. +* `message` {string} A short description of what happened. + +The `'multipleResolves'` event is emitted whenever a `Promise` has been either: + +* Resolved more than once. +* Rejected after resolve. + +This is useful for tracking errors in your application while using the promise +constructor. Otherwise such mistakes are silently swallowed due to being in a +dead zone. + +It is recommended to end the process on such errors, since the process could be +in an undefined state. + +```js +process.on('multipleResolves', (type, promise, reason, message) => { + console.error(`${type}: ${message}`); + console.error(promise, reason); + process.exit(1); +}); + +async function main() { + try { + return await new Promise((resolve, reject) => { + resolve('First call'); + resolve('Swallowed resolve'); + reject(new Error('Swallowed reject')); + }); + } catch { + throw new Error('Failed'); + } +} + +main().then(console.log); +// resolveAfterResolved: Resolve was called more than once. +// Promise { 'First call' } 'Swallowed resolve' +// rejectAfterResolved: Reject was called after resolve. +// Promise { 'First call' } Error: Swallowed reject +// at Promise (*) +// at new Promise () +// at main (*) +// First call +``` + ### Event: 'rejectionHandled' -* `type` {string} The error type. One of `'resolveAfterResolved'` or - `'rejectAfterResolved'`. +* `type` {string} The error type. One of `'resolve'` or `'reject'`. * `promise` {Promise} The promise that resolved or rejected more than once. * `value` {any} The value with which the promise was either resolved or rejected after the original resolve. -* `message` {string} A short description of what happened. The `'multipleResolves'` event is emitted whenever a `Promise` has been either: * Resolved more than once. +* Rejected more than once. * Rejected after resolve. +* Resolved after reject. This is useful for tracking errors in your application while using the promise constructor. Otherwise such mistakes are silently swallowed due to being in a @@ -122,10 +122,9 @@ It is recommended to end the process on such errors, since the process could be in an undefined state. ```js -process.on('multipleResolves', (type, promise, reason, message) => { - console.error(`${type}: ${message}`); - console.error(promise, reason); - process.exit(1); +process.on('multipleResolves', (type, promise, reason) => { + console.error(type, promise, reason); + setImmediate(() => process.exit(1)); }); async function main() { @@ -141,10 +140,8 @@ async function main() { } main().then(console.log); -// resolveAfterResolved: Resolve was called more than once. -// Promise { 'First call' } 'Swallowed resolve' -// rejectAfterResolved: Reject was called after resolve. -// Promise { 'First call' } Error: Swallowed reject +// resolve: Promise { 'First call' } 'Swallowed resolve' +// reject: Promise { 'First call' } Error: Swallowed reject // at Promise (*) // at new Promise () // at main (*) diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index f386007b99c207..695baefefae3e5 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -23,16 +23,18 @@ function handler(type, promise, reason) { case promiseRejectEvents.kPromiseHandlerAddedAfterReject: return handledRejection(promise); case promiseRejectEvents.kPromiseResolveAfterResolved: - return resolveError('resolveAfterResolved', promise, reason, - 'Resolve was called more than once.'); + return resolveError('resolve', promise, reason); case promiseRejectEvents.kPromiseRejectAfterResolved: - return resolveError('rejectAfterResolved', promise, reason, - 'Reject was called after resolve.'); + return resolveError('reject', promise, reason); } } -function resolveError(type, promise, reason, message) { - process.emit('multipleResolves', type, promise, reason, message); +function resolveError(type, promise, reason) { + // We have to wrap this in a next tick. Otherwise the error could be caught by + // the executed promise. + process.nextTick(() => { + process.emit('multipleResolves', type, promise, reason); + }); } function unhandledRejection(promise, reason) { diff --git a/test/parallel/test-promise-swallowed-event.js b/test/parallel/test-promise-swallowed-event.js index cd9a0b3738bf77..144d732c20668b 100644 --- a/test/parallel/test-promise-swallowed-event.js +++ b/test/parallel/test-promise-swallowed-event.js @@ -3,30 +3,56 @@ const common = require('../common'); const assert = require('assert'); +const rejection = new Error('Swallowed reject'); +const rejection2 = new TypeError('Weird'); const resolveMessage = 'First call'; +const rejectPromise = new Promise((r) => setTimeout(r, 10, rejection2)); const swallowedResolve = 'Swallowed resolve'; -const rejection = new Error('Swallowed reject'); +const swallowedResolve2 = 'Foobar'; + +process.on('multipleResolves', common.mustCall(handler, 4)); + +const p1 = new Promise((resolve, reject) => { + resolve(resolveMessage); + resolve(swallowedResolve); + reject(rejection); +}); + +const p2 = new Promise((resolve, reject) => { + reject(rejectPromise); + resolve(swallowedResolve2); + reject(rejection2); +}).catch(common.mustCall((exception) => { + assert.strictEqual(exception, rejectPromise); +})); const expected = [ - 'resolveAfterResolved', + 'resolve', + p1, swallowedResolve, - 'Resolve was called more than once.', - 'rejectAfterResolved', + 'reject', + p1, rejection, - 'Reject was called after resolve.' + 'resolve', + p2, + swallowedResolve2, + 'reject', + p2, + rejection2 ]; -function handler(type, p, reason, message) { +let count = 0; + +function handler(type, promise, reason) { assert.strictEqual(type, expected.shift()); - assert.deepStrictEqual(p, Promise.resolve(resolveMessage)); + // In the first two cases the promise is identical because it's not delayed. + // The other two cases are not identical, because the `promise` is caught in a + // state when it has no knowledge about the `.catch()` handler that is + // attached to it right afterwards. + if (count++ < 2) { + assert.strictEqual(promise, expected.shift()); + } else { + assert.notStrictEqual(promise, expected.shift()); + } assert.strictEqual(reason, expected.shift()); - assert.strictEqual(message, expected.shift()); } - -process.on('multipleResolves', common.mustCall(handler, 2)); - -new Promise((resolve, reject) => { - resolve(resolveMessage); - resolve(swallowedResolve); - reject(rejection); -}); From 065054bb82a4f41ee59582284d427a9d30095af6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 19 Sep 2018 17:54:10 +0200 Subject: [PATCH 3/3] fixup --- doc/api/process.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/api/process.md b/doc/api/process.md index bebe0e49d71f69..cb264cb59b4ca5 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -119,7 +119,9 @@ constructor. Otherwise such mistakes are silently swallowed due to being in a dead zone. It is recommended to end the process on such errors, since the process could be -in an undefined state. +in an undefined state. While using the promise constructor make sure that it is +guaranteed to trigger the `resolve()` or `reject()` functions exactly once per +call and never call both functions in the same call. ```js process.on('multipleResolves', (type, promise, reason) => {