Skip to content

Commit 5605cec

Browse files
BridgeARBridgeAR
committed
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. PR-URL: nodejs#22218 Fixes: nodejs/promises-debugging#8 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
1 parent ea3bb9a commit 5605cec

6 files changed

Lines changed: 177 additions & 35 deletions

File tree

‎doc/api/process.md‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,59 @@ the child process.
9797
The message goes through serialization and parsing. The resulting message might
9898
not be the same as what is originally sent.
9999

100+
### Event: 'multipleResolves'
101+
<!-- YAML
102+
added: REPLACEME
103+
-->
104+
105+
* `type` {string} The error type. One of `'resolve'` or `'reject'`.
106+
* `promise` {Promise} The promise that resolved or rejected more than once.
107+
* `value` {any} The value with which the promise was either resolved or
108+
rejected after the original resolve.
109+
110+
The `'multipleResolves'` event is emitted whenever a `Promise` has been either:
111+
112+
* Resolved more than once.
113+
* Rejected more than once.
114+
* Rejected after resolve.
115+
* Resolved after reject.
116+
117+
This is useful for tracking errors in your application while using the promise
118+
constructor. Otherwise such mistakes are silently swallowed due to being in a
119+
dead zone.
120+
121+
It is recommended to end the process on such errors, since the process could be
122+
in an undefined state. While using the promise constructor make sure that it is
123+
guaranteed to trigger the `resolve()` or `reject()` functions exactly once per
124+
call and never call both functions in the same call.
125+
126+
```js
127+
process.on('multipleResolves', (type, promise, reason) => {
128+
console.error(type, promise, reason);
129+
setImmediate(() => process.exit(1));
130+
});
131+
132+
async function main() {
133+
try {
134+
return await new Promise((resolve, reject) => {
135+
resolve('First call');
136+
resolve('Swallowed resolve');
137+
reject(new Error('Swallowed reject'));
138+
});
139+
} catch {
140+
throw new Error('Failed');
141+
}
142+
}
143+
144+
main().then(console.log);
145+
// resolve: Promise { 'First call' } 'Swallowed resolve'
146+
// reject: Promise { 'First call' } Error: Swallowed reject
147+
// at Promise (*)
148+
// at new Promise (<anonymous>)
149+
// at main (*)
150+
// First call
151+
```
152+
100153
### Event: 'rejectionHandled'
101154
<!-- YAML
102155
added: v1.4.1

‎lib/internal/process/promises.js‎

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,37 @@ const { safeToString } = internalBinding('util');
66
const maybeUnhandledPromises = new WeakMap();
77
const pendingUnhandledRejections = [];
88
const asyncHandledRejections = [];
9+
const promiseRejectEvents = {};
910
let lastPromiseId = 0;
1011

1112
exports.setup = setupPromises;
1213

1314
function setupPromises(_setupPromises) {
14-
_setupPromises(unhandledRejection, handledRejection);
15+
_setupPromises(handler, promiseRejectEvents);
1516
return emitPromiseRejectionWarnings;
1617
}
1718

19+
function handler(type, promise, reason) {
20+
switch (type) {
21+
case promiseRejectEvents.kPromiseRejectWithNoHandler:
22+
return unhandledRejection(promise, reason);
23+
case promiseRejectEvents.kPromiseHandlerAddedAfterReject:
24+
return handledRejection(promise);
25+
case promiseRejectEvents.kPromiseResolveAfterResolved:
26+
return resolveError('resolve', promise, reason);
27+
case promiseRejectEvents.kPromiseRejectAfterResolved:
28+
return resolveError('reject', promise, reason);
29+
}
30+
}
31+
32+
function resolveError(type, promise, reason) {
33+
// We have to wrap this in a next tick. Otherwise the error could be caught by
34+
// the executed promise.
35+
process.nextTick(() => {
36+
process.emit('multipleResolves', type, promise, reason);
37+
});
38+
}
39+
1840
function unhandledRejection(promise, reason) {
1941
maybeUnhandledPromises.set(promise, {
2042
reason,
@@ -46,16 +68,6 @@ function handledRejection(promise) {
4668

4769
const unhandledRejectionErrName = 'UnhandledPromiseRejectionWarning';
4870
function emitWarning(uid, reason) {
49-
try {
50-
if (reason instanceof Error) {
51-
process.emitWarning(reason.stack, unhandledRejectionErrName);
52-
} else {
53-
process.emitWarning(safeToString(reason), unhandledRejectionErrName);
54-
}
55-
} catch (e) {
56-
// ignored
57-
}
58-
5971
// eslint-disable-next-line no-restricted-syntax
6072
const warning = new Error(
6173
'Unhandled promise rejection. This error originated either by ' +
@@ -67,10 +79,12 @@ function emitWarning(uid, reason) {
6779
try {
6880
if (reason instanceof Error) {
6981
warning.stack = reason.stack;
82+
process.emitWarning(reason.stack, unhandledRejectionErrName);
83+
} else {
84+
process.emitWarning(safeToString(reason), unhandledRejectionErrName);
7085
}
71-
} catch (err) {
72-
// ignored
73-
}
86+
} catch {}
87+
7488
process.emitWarning(warning);
7589
emitDeprecationWarning();
7690
}

‎src/bootstrapper.cc‎

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,13 @@ using v8::Context;
1212
using v8::Function;
1313
using v8::FunctionCallbackInfo;
1414
using v8::Isolate;
15+
using v8::kPromiseHandlerAddedAfterReject;
16+
using v8::kPromiseRejectAfterResolved;
17+
using v8::kPromiseRejectWithNoHandler;
18+
using v8::kPromiseResolveAfterResolved;
1519
using v8::Local;
1620
using v8::MaybeLocal;
21+
using v8::Number;
1722
using v8::Object;
1823
using v8::Promise;
1924
using v8::PromiseRejectEvent;
@@ -67,34 +72,40 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
6772
PromiseRejectEvent event = message.GetEvent();
6873

6974
Environment* env = Environment::GetCurrent(isolate);
75+
7076
if (env == nullptr) return;
71-
Local<Function> callback;
77+
78+
Local<Function> callback = env->promise_handler_function();
7279
Local<Value> value;
80+
Local<Value> type = Number::New(env->isolate(), event);
7381

74-
if (event == v8::kPromiseRejectWithNoHandler) {
75-
callback = env->promise_reject_unhandled_function();
82+
if (event == kPromiseRejectWithNoHandler) {
7683
value = message.GetValue();
77-
78-
if (value.IsEmpty())
79-
value = Undefined(isolate);
80-
8184
unhandledRejections++;
82-
} else if (event == v8::kPromiseHandlerAddedAfterReject) {
83-
callback = env->promise_reject_handled_function();
85+
TRACE_COUNTER2(TRACING_CATEGORY_NODE2(promises, rejections),
86+
"rejections",
87+
"unhandled", unhandledRejections,
88+
"handledAfter", rejectionsHandledAfter);
89+
} else if (event == kPromiseHandlerAddedAfterReject) {
8490
value = Undefined(isolate);
85-
8691
rejectionsHandledAfter++;
92+
TRACE_COUNTER2(TRACING_CATEGORY_NODE2(promises, rejections),
93+
"rejections",
94+
"unhandled", unhandledRejections,
95+
"handledAfter", rejectionsHandledAfter);
96+
} else if (event == kPromiseResolveAfterResolved) {
97+
value = message.GetValue();
98+
} else if (event == kPromiseRejectAfterResolved) {
99+
value = message.GetValue();
87100
} else {
88101
return;
89102
}
90103

91-
TRACE_COUNTER2(TRACING_CATEGORY_NODE2(promises, rejections),
92-
"rejections",
93-
"unhandled", unhandledRejections,
94-
"handledAfter", rejectionsHandledAfter);
95-
104+
if (value.IsEmpty()) {
105+
value = Undefined(isolate);
106+
}
96107

97-
Local<Value> args[] = { promise, value };
108+
Local<Value> args[] = { type, promise, value };
98109
MaybeLocal<Value> ret = callback->Call(env->context(),
99110
Undefined(isolate),
100111
arraysize(args),
@@ -109,11 +120,17 @@ void SetupPromises(const FunctionCallbackInfo<Value>& args) {
109120
Isolate* isolate = env->isolate();
110121

111122
CHECK(args[0]->IsFunction());
112-
CHECK(args[1]->IsFunction());
123+
CHECK(args[1]->IsObject());
124+
125+
Local<Object> constants = args[1].As<Object>();
126+
127+
NODE_DEFINE_CONSTANT(constants, kPromiseRejectWithNoHandler);
128+
NODE_DEFINE_CONSTANT(constants, kPromiseHandlerAddedAfterReject);
129+
NODE_DEFINE_CONSTANT(constants, kPromiseResolveAfterResolved);
130+
NODE_DEFINE_CONSTANT(constants, kPromiseRejectAfterResolved);
113131

114132
isolate->SetPromiseRejectCallback(PromiseRejectCallback);
115-
env->set_promise_reject_unhandled_function(args[0].As<Function>());
116-
env->set_promise_reject_handled_function(args[1].As<Function>());
133+
env->set_promise_handler_function(args[0].As<Function>());
117134
}
118135

119136
#define BOOTSTRAP_METHOD(name, fn) env->SetMethod(bootstrapper, #name, fn)

‎src/env.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -342,8 +342,7 @@ struct PackageConfig {
342342
V(performance_entry_callback, v8::Function) \
343343
V(performance_entry_template, v8::Function) \
344344
V(process_object, v8::Object) \
345-
V(promise_reject_handled_function, v8::Function) \
346-
V(promise_reject_unhandled_function, v8::Function) \
345+
V(promise_handler_function, v8::Function) \
347346
V(promise_wrap_template, v8::ObjectTemplate) \
348347
V(push_values_to_array_function, v8::Function) \
349348
V(sab_lifetimepartner_constructor_template, v8::FunctionTemplate) \

‎test/message/unhandled_promise_trace_warnings.out‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
at *
3535
(node:*) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)
3636
at handledRejection (internal/process/promises.js:*)
37+
at handler (internal/process/promises.js:*)
3738
at Promise.then *
3839
at Promise.catch *
3940
at Immediate.setImmediate (*test*message*unhandled_promise_trace_warnings.js:*)

‎test/parallel/test-promise-swallowed-event.js‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
6+
const rejection = new Error('Swallowed reject');
7+
const rejection2 = new TypeError('Weird');
8+
const resolveMessage = 'First call';
9+
const rejectPromise = new Promise((r) => setTimeout(r, 10, rejection2));
10+
const swallowedResolve = 'Swallowed resolve';
11+
const swallowedResolve2 = 'Foobar';
12+
13+
process.on('multipleResolves', common.mustCall(handler, 4));
14+
15+
const p1 = new Promise((resolve, reject) => {
16+
resolve(resolveMessage);
17+
resolve(swallowedResolve);
18+
reject(rejection);
19+
});
20+
21+
const p2 = new Promise((resolve, reject) => {
22+
reject(rejectPromise);
23+
resolve(swallowedResolve2);
24+
reject(rejection2);
25+
}).catch(common.mustCall((exception) => {
26+
assert.strictEqual(exception, rejectPromise);
27+
}));
28+
29+
const expected = [
30+
'resolve',
31+
p1,
32+
swallowedResolve,
33+
'reject',
34+
p1,
35+
rejection,
36+
'resolve',
37+
p2,
38+
swallowedResolve2,
39+
'reject',
40+
p2,
41+
rejection2
42+
];
43+
44+
let count = 0;
45+
46+
function handler(type, promise, reason) {
47+
assert.strictEqual(type, expected.shift());
48+
// In the first two cases the promise is identical because it's not delayed.
49+
// The other two cases are not identical, because the `promise` is caught in a
50+
// state when it has no knowledge about the `.catch()` handler that is
51+
// attached to it right afterwards.
52+
if (count++ < 2) {
53+
assert.strictEqual(promise, expected.shift());
54+
} else {
55+
assert.notStrictEqual(promise, expected.shift());
56+
}
57+
assert.strictEqual(reason, expected.shift());
58+
}

0 commit comments

Comments
 (0)