Skip to content

Commit c07b12d

Browse files
joyeecheungBridgeAR
authored andcommitted
process: make tick callback and promise rejection callback more robust
- Rename `internalTickCallback` to `processTicksAndRejections`, make sure it does not get called if it's not set in C++. - Rename `emitPromiseRejectionWarnings` to `processPromiseRejections` since it also emit events that are not warnings. - Sets `SetPromiseRejectCallback` in the `Environment` constructor to make sure it only gets called once per-isolate, and make sure it does not get called if it's not set in C++. - Wrap promise rejection callback initialization into `listenForRejections()`. - Add comments. PR-URL: #25200 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent c5ab340 commit c07b12d

9 files changed

+50
-32
lines changed

lib/internal/process/next_tick.js

+9-11
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,14 @@ const {
66
tickInfo,
77
// Used to run V8's micro task queue.
88
runMicrotasks,
9-
setTickCallback,
10-
initializePromiseRejectCallback
9+
setTickCallback
1110
} = internalBinding('task_queue');
1211

1312
const {
1413
setHasRejectionToWarn,
1514
hasRejectionToWarn,
16-
promiseRejectHandler,
17-
emitPromiseRejectionWarnings
15+
listenForRejections,
16+
processPromiseRejections
1817
} = require('internal/process/promises');
1918

2019
const {
@@ -49,10 +48,10 @@ function runNextTicks() {
4948
if (!hasTickScheduled() && !hasRejectionToWarn())
5049
return;
5150

52-
internalTickCallback();
51+
processTicksAndRejections();
5352
}
5453

55-
function internalTickCallback() {
54+
function processTicksAndRejections() {
5655
let tock;
5756
do {
5857
while (tock = queue.shift()) {
@@ -80,7 +79,7 @@ function internalTickCallback() {
8079
}
8180
setHasTickScheduled(false);
8281
runMicrotasks();
83-
} while (!queue.isEmpty() || emitPromiseRejectionWarnings());
82+
} while (!queue.isEmpty() || processPromiseRejections());
8483
setHasRejectionToWarn(false);
8584
}
8685

@@ -134,11 +133,10 @@ function nextTick(callback) {
134133
// TODO(joyeecheung): make this a factory class so that node.js can
135134
// control the side effects caused by the initializers.
136135
exports.setup = function() {
137-
// Initializes the per-isolate promise rejection callback which
138-
// will call the handler being passed into this function.
139-
initializePromiseRejectCallback(promiseRejectHandler);
136+
// Sets the per-isolate promise rejection callback
137+
listenForRejections();
140138
// Sets the callback to be run in every tick.
141-
setTickCallback(internalTickCallback);
139+
setTickCallback(processTicksAndRejections);
142140
return {
143141
nextTick,
144142
runNextTicks

lib/internal/process/promises.js

+11-4
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ const {
88
kPromiseHandlerAddedAfterReject,
99
kPromiseResolveAfterResolved,
1010
kPromiseRejectAfterResolved
11-
}
11+
},
12+
setPromiseRejectCallback
1213
} = internalBinding('task_queue');
1314

1415
// *Must* match Environment::TickInfo::Fields in src/env.h.
@@ -117,7 +118,9 @@ function emitDeprecationWarning() {
117118
}
118119
}
119120

120-
function emitPromiseRejectionWarnings() {
121+
// If this method returns true, at least one more tick need to be
122+
// scheduled to process any potential pending rejections
123+
function processPromiseRejections() {
121124
while (asyncHandledRejections.length > 0) {
122125
const { promise, warning } = asyncHandledRejections.shift();
123126
if (!process.emit('rejectionHandled', promise)) {
@@ -142,9 +145,13 @@ function emitPromiseRejectionWarnings() {
142145
return maybeScheduledTicks || pendingUnhandledRejections.length !== 0;
143146
}
144147

148+
function listenForRejections() {
149+
setPromiseRejectCallback(promiseRejectHandler);
150+
}
151+
145152
module.exports = {
146153
hasRejectionToWarn,
147154
setHasRejectionToWarn,
148-
promiseRejectHandler,
149-
emitPromiseRejectionWarnings
155+
listenForRejections,
156+
processPromiseRejections
150157
};

src/callback_scope.cc

+8-2
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
namespace node {
77

8+
using v8::Function;
89
using v8::HandleScope;
910
using v8::Isolate;
1011
using v8::Local;
@@ -114,8 +115,13 @@ void InternalCallbackScope::Close() {
114115

115116
if (!env_->can_call_into_js()) return;
116117

117-
if (env_->tick_callback_function()
118-
->Call(env_->context(), process, 0, nullptr).IsEmpty()) {
118+
Local<Function> tick_callback = env_->tick_callback_function();
119+
120+
// The tick is triggered before JS land calls SetTickCallback
121+
// to initializes the tick callback during bootstrap.
122+
CHECK(!tick_callback.IsEmpty());
123+
124+
if (tick_callback->Call(env_->context(), process, 0, nullptr).IsEmpty()) {
119125
failed_ = true;
120126
}
121127
}

src/env.cc

+4
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,10 @@ Environment::Environment(IsolateData* isolate_data,
235235
if (options_->no_force_async_hooks_checks) {
236236
async_hooks_.no_force_checks();
237237
}
238+
239+
// TODO(addaleax): the per-isolate state should not be controlled by
240+
// a single Environment.
241+
isolate()->SetPromiseRejectCallback(task_queue::PromiseRejectCallback);
238242
}
239243

240244
Environment::~Environment() {

src/node_internals.h

+4
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,10 @@ SlicedArguments::SlicedArguments(
180180
size_ = size;
181181
}
182182

183+
namespace task_queue {
184+
void PromiseRejectCallback(v8::PromiseRejectMessage message);
185+
} // namespace task_queue
186+
183187
v8::Maybe<bool> ProcessEmitWarning(Environment* env, const char* fmt, ...);
184188
v8::Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
185189
const char* warning,

src/node_task_queue.cc

+8-9
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ static void SetTickCallback(const FunctionCallbackInfo<Value>& args) {
3636
env->set_tick_callback_function(args[0].As<Function>());
3737
}
3838

39-
static void PromiseRejectCallback(PromiseRejectMessage message) {
39+
void PromiseRejectCallback(PromiseRejectMessage message) {
4040
static std::atomic<uint64_t> unhandledRejections{0};
4141
static std::atomic<uint64_t> rejectionsHandledAfter{0};
4242

@@ -49,6 +49,10 @@ static void PromiseRejectCallback(PromiseRejectMessage message) {
4949
if (env == nullptr) return;
5050

5151
Local<Function> callback = env->promise_reject_callback();
52+
// The promise is rejected before JS land calls SetPromiseRejectCallback
53+
// to initializes the promise reject callback during bootstrap.
54+
CHECK(!callback.IsEmpty());
55+
5256
Local<Value> value;
5357
Local<Value> type = Number::New(env->isolate(), event);
5458

@@ -83,17 +87,12 @@ static void PromiseRejectCallback(PromiseRejectMessage message) {
8387
env->context(), Undefined(isolate), arraysize(args), args));
8488
}
8589

86-
static void InitializePromiseRejectCallback(
90+
static void SetPromiseRejectCallback(
8791
const FunctionCallbackInfo<Value>& args) {
8892
Environment* env = Environment::GetCurrent(args);
8993
Isolate* isolate = env->isolate();
9094

9195
CHECK(args[0]->IsFunction());
92-
93-
// TODO(joyeecheung): this may be moved to somewhere earlier in the bootstrap
94-
// to make sure it's only called once
95-
isolate->SetPromiseRejectCallback(PromiseRejectCallback);
96-
9796
env->set_promise_reject_callback(args[0].As<Function>());
9897
}
9998

@@ -120,8 +119,8 @@ static void Initialize(Local<Object> target,
120119
FIXED_ONE_BYTE_STRING(isolate, "promiseRejectEvents"),
121120
events).FromJust();
122121
env->SetMethod(target,
123-
"initializePromiseRejectCallback",
124-
InitializePromiseRejectCallback);
122+
"setPromiseRejectCallback",
123+
SetPromiseRejectCallback);
125124
}
126125

127126
} // namespace task_queue

test/message/events_unhandled_error_nexttick.out

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ Error
1515
at startup (internal/bootstrap/node.js:*:*)
1616
Emitted 'error' event at:
1717
at process.nextTick (*events_unhandled_error_nexttick.js:*:*)
18-
at internalTickCallback (internal/process/next_tick.js:*:*)
18+
at processTicksAndRejections (internal/process/next_tick.js:*:*)
1919
at process.runNextTicks [as _tickCallback] (internal/process/next_tick.js:*:*)
2020
at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
2121
at executeUserCode (internal/bootstrap/node.js:*:*)

test/message/nexttick_throw.out

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
^
55
ReferenceError: undefined_reference_error_maker is not defined
66
at *test*message*nexttick_throw.js:*:*
7-
at internalTickCallback (internal/process/next_tick.js:*:*)
7+
at processTicksAndRejections (internal/process/next_tick.js:*:*)
88
at process.runNextTicks [as _tickCallback] (internal/process/next_tick.js:*:*)
99
at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
1010
at executeUserCode (internal/bootstrap/node.js:*:*)

test/message/stdin_messages.out

+4-4
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ SyntaxError: Strict mode code may not include a with statement
1212
at Socket.process.stdin.on (internal/bootstrap/node.js:*:*)
1313
at Socket.emit (events.js:*:*)
1414
at endReadableNT (_stream_readable.js:*:*)
15-
at process.internalTickCallback (internal/process/next_tick.js:*:*)
15+
at processTicksAndRejections (internal/process/next_tick.js:*:*)
1616
42
1717
42
1818
[stdin]:1
@@ -29,7 +29,7 @@ Error: hello
2929
at Socket.process.stdin.on (internal/bootstrap/node.js:*:*)
3030
at Socket.emit (events.js:*:*)
3131
at endReadableNT (_stream_readable.js:*:*)
32-
at process.internalTickCallback (internal/process/next_tick.js:*:*)
32+
at processTicksAndRejections (internal/process/next_tick.js:*:*)
3333
[stdin]:1
3434
throw new Error("hello")
3535
^
@@ -44,7 +44,7 @@ Error: hello
4444
at Socket.process.stdin.on (internal/bootstrap/node.js:*:*)
4545
at Socket.emit (events.js:*:*)
4646
at endReadableNT (_stream_readable.js:*:*)
47-
at process.internalTickCallback (internal/process/next_tick.js:*:*)
47+
at processTicksAndRejections (internal/process/next_tick.js:*:*)
4848
100
4949
[stdin]:1
5050
var x = 100; y = x;
@@ -60,7 +60,7 @@ ReferenceError: y is not defined
6060
at Socket.process.stdin.on (internal/bootstrap/node.js:*:*)
6161
at Socket.emit (events.js:*:*)
6262
at endReadableNT (_stream_readable.js:*:*)
63-
at process.internalTickCallback (internal/process/next_tick.js:*:*)
63+
at processTicksAndRejections (internal/process/next_tick.js:*:*)
6464

6565
[stdin]:1
6666
var ______________________________________________; throw 10

0 commit comments

Comments
 (0)