From f322d289fa66b92a0d6e14bb155067f52b0a795e Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 23 Dec 2018 08:45:07 +0800 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/25200 Reviewed-By: Anna Henningsen Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- lib/internal/process/next_tick.js | 20 +++++++++---------- lib/internal/process/promises.js | 15 ++++++++++---- src/callback_scope.cc | 10 ++++++++-- src/env.cc | 4 ++++ src/node_internals.h | 4 ++++ src/node_task_queue.cc | 17 ++++++++-------- .../events_unhandled_error_nexttick.out | 2 +- test/message/nexttick_throw.out | 2 +- test/message/stdin_messages.out | 8 ++++---- 9 files changed, 50 insertions(+), 32 deletions(-) diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index 0bfe168975acb3..24104ec34abaee 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -6,15 +6,14 @@ const { tickInfo, // Used to run V8's micro task queue. runMicrotasks, - setTickCallback, - initializePromiseRejectCallback + setTickCallback } = internalBinding('task_queue'); const { setHasRejectionToWarn, hasRejectionToWarn, - promiseRejectHandler, - emitPromiseRejectionWarnings + listenForRejections, + processPromiseRejections } = require('internal/process/promises'); const { @@ -49,10 +48,10 @@ function runNextTicks() { if (!hasTickScheduled() && !hasRejectionToWarn()) return; - internalTickCallback(); + processTicksAndRejections(); } -function internalTickCallback() { +function processTicksAndRejections() { let tock; do { while (tock = queue.shift()) { @@ -80,7 +79,7 @@ function internalTickCallback() { } setHasTickScheduled(false); runMicrotasks(); - } while (!queue.isEmpty() || emitPromiseRejectionWarnings()); + } while (!queue.isEmpty() || processPromiseRejections()); setHasRejectionToWarn(false); } @@ -134,11 +133,10 @@ function nextTick(callback) { // TODO(joyeecheung): make this a factory class so that node.js can // control the side effects caused by the initializers. exports.setup = function() { - // Initializes the per-isolate promise rejection callback which - // will call the handler being passed into this function. - initializePromiseRejectCallback(promiseRejectHandler); + // Sets the per-isolate promise rejection callback + listenForRejections(); // Sets the callback to be run in every tick. - setTickCallback(internalTickCallback); + setTickCallback(processTicksAndRejections); return { nextTick, runNextTicks diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index 047a9d43aabf2b..f3e118e2ce7509 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -8,7 +8,8 @@ const { kPromiseHandlerAddedAfterReject, kPromiseResolveAfterResolved, kPromiseRejectAfterResolved - } + }, + setPromiseRejectCallback } = internalBinding('task_queue'); // *Must* match Environment::TickInfo::Fields in src/env.h. @@ -117,7 +118,9 @@ function emitDeprecationWarning() { } } -function emitPromiseRejectionWarnings() { +// If this method returns true, at least one more tick need to be +// scheduled to process any potential pending rejections +function processPromiseRejections() { while (asyncHandledRejections.length > 0) { const { promise, warning } = asyncHandledRejections.shift(); if (!process.emit('rejectionHandled', promise)) { @@ -142,9 +145,13 @@ function emitPromiseRejectionWarnings() { return maybeScheduledTicks || pendingUnhandledRejections.length !== 0; } +function listenForRejections() { + setPromiseRejectCallback(promiseRejectHandler); +} + module.exports = { hasRejectionToWarn, setHasRejectionToWarn, - promiseRejectHandler, - emitPromiseRejectionWarnings + listenForRejections, + processPromiseRejections }; diff --git a/src/callback_scope.cc b/src/callback_scope.cc index 89fd4d6dd433d0..fee7417fd37187 100644 --- a/src/callback_scope.cc +++ b/src/callback_scope.cc @@ -5,6 +5,7 @@ namespace node { +using v8::Function; using v8::HandleScope; using v8::Isolate; using v8::Local; @@ -114,8 +115,13 @@ void InternalCallbackScope::Close() { if (!env_->can_call_into_js()) return; - if (env_->tick_callback_function() - ->Call(env_->context(), process, 0, nullptr).IsEmpty()) { + Local tick_callback = env_->tick_callback_function(); + + // The tick is triggered before JS land calls SetTickCallback + // to initializes the tick callback during bootstrap. + CHECK(!tick_callback.IsEmpty()); + + if (tick_callback->Call(env_->context(), process, 0, nullptr).IsEmpty()) { failed_ = true; } } diff --git a/src/env.cc b/src/env.cc index d8c4dce2058b03..3d06ebe2082b08 100644 --- a/src/env.cc +++ b/src/env.cc @@ -241,6 +241,10 @@ Environment::Environment(IsolateData* isolate_data, if (options_->no_force_async_hooks_checks) { async_hooks_.no_force_checks(); } + + // TODO(addaleax): the per-isolate state should not be controlled by + // a single Environment. + isolate()->SetPromiseRejectCallback(task_queue::PromiseRejectCallback); } Environment::~Environment() { diff --git a/src/node_internals.h b/src/node_internals.h index acc97633ce9e78..4ed75385f9aa48 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -180,6 +180,10 @@ SlicedArguments::SlicedArguments( size_ = size; } +namespace task_queue { +void PromiseRejectCallback(v8::PromiseRejectMessage message); +} // namespace task_queue + v8::Maybe ProcessEmitWarning(Environment* env, const char* fmt, ...); v8::Maybe ProcessEmitDeprecationWarning(Environment* env, const char* warning, diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index 5bfa281068e755..43d8e1cc246b8c 100644 --- a/src/node_task_queue.cc +++ b/src/node_task_queue.cc @@ -36,7 +36,7 @@ static void SetTickCallback(const FunctionCallbackInfo& args) { env->set_tick_callback_function(args[0].As()); } -static void PromiseRejectCallback(PromiseRejectMessage message) { +void PromiseRejectCallback(PromiseRejectMessage message) { static std::atomic unhandledRejections{0}; static std::atomic rejectionsHandledAfter{0}; @@ -49,6 +49,10 @@ static void PromiseRejectCallback(PromiseRejectMessage message) { if (env == nullptr) return; Local callback = env->promise_reject_callback(); + // The promise is rejected before JS land calls SetPromiseRejectCallback + // to initializes the promise reject callback during bootstrap. + CHECK(!callback.IsEmpty()); + Local value; Local type = Number::New(env->isolate(), event); @@ -83,17 +87,12 @@ static void PromiseRejectCallback(PromiseRejectMessage message) { env->context(), Undefined(isolate), arraysize(args), args)); } -static void InitializePromiseRejectCallback( +static void SetPromiseRejectCallback( const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); CHECK(args[0]->IsFunction()); - - // TODO(joyeecheung): this may be moved to somewhere earlier in the bootstrap - // to make sure it's only called once - isolate->SetPromiseRejectCallback(PromiseRejectCallback); - env->set_promise_reject_callback(args[0].As()); } @@ -120,8 +119,8 @@ static void Initialize(Local target, FIXED_ONE_BYTE_STRING(isolate, "promiseRejectEvents"), events).FromJust(); env->SetMethod(target, - "initializePromiseRejectCallback", - InitializePromiseRejectCallback); + "setPromiseRejectCallback", + SetPromiseRejectCallback); } } // namespace task_queue diff --git a/test/message/events_unhandled_error_nexttick.out b/test/message/events_unhandled_error_nexttick.out index cd098b64bfb5f1..e661b32d8fb17d 100644 --- a/test/message/events_unhandled_error_nexttick.out +++ b/test/message/events_unhandled_error_nexttick.out @@ -14,7 +14,7 @@ Error at startExecution (internal/bootstrap/node.js:*:*) Emitted 'error' event at: at process.nextTick (*events_unhandled_error_nexttick.js:*:*) - at internalTickCallback (internal/process/next_tick.js:*:*) + at processTicksAndRejections (internal/process/next_tick.js:*:*) at process.runNextTicks [as _tickCallback] (internal/process/next_tick.js:*:*) at Function.Module.runMain (internal/modules/cjs/loader.js:*:*) at executeUserCode (internal/bootstrap/node.js:*:*) diff --git a/test/message/nexttick_throw.out b/test/message/nexttick_throw.out index 6a4d7ed2dabcb0..a2169b2f056fe1 100644 --- a/test/message/nexttick_throw.out +++ b/test/message/nexttick_throw.out @@ -4,7 +4,7 @@ ^ ReferenceError: undefined_reference_error_maker is not defined at *test*message*nexttick_throw.js:*:* - at internalTickCallback (internal/process/next_tick.js:*:*) + at processTicksAndRejections (internal/process/next_tick.js:*:*) at process.runNextTicks [as _tickCallback] (internal/process/next_tick.js:*:*) at Function.Module.runMain (internal/modules/cjs/loader.js:*:*) at executeUserCode (internal/bootstrap/node.js:*:*) diff --git a/test/message/stdin_messages.out b/test/message/stdin_messages.out index ac1a4df02816df..2bf935d7cbd16f 100644 --- a/test/message/stdin_messages.out +++ b/test/message/stdin_messages.out @@ -12,7 +12,7 @@ SyntaxError: Strict mode code may not include a with statement at Socket.process.stdin.on (internal/bootstrap/node.js:*:*) at Socket.emit (events.js:*:*) at endReadableNT (_stream_readable.js:*:*) - at process.internalTickCallback (internal/process/next_tick.js:*:*) + at processTicksAndRejections (internal/process/next_tick.js:*:*) 42 42 [stdin]:1 @@ -29,7 +29,7 @@ Error: hello at Socket.process.stdin.on (internal/bootstrap/node.js:*:*) at Socket.emit (events.js:*:*) at endReadableNT (_stream_readable.js:*:*) - at process.internalTickCallback (internal/process/next_tick.js:*:*) + at processTicksAndRejections (internal/process/next_tick.js:*:*) [stdin]:1 throw new Error("hello") ^ @@ -44,7 +44,7 @@ Error: hello at Socket.process.stdin.on (internal/bootstrap/node.js:*:*) at Socket.emit (events.js:*:*) at endReadableNT (_stream_readable.js:*:*) - at process.internalTickCallback (internal/process/next_tick.js:*:*) + at processTicksAndRejections (internal/process/next_tick.js:*:*) 100 [stdin]:1 var x = 100; y = x; @@ -60,7 +60,7 @@ ReferenceError: y is not defined at Socket.process.stdin.on (internal/bootstrap/node.js:*:*) at Socket.emit (events.js:*:*) at endReadableNT (_stream_readable.js:*:*) - at process.internalTickCallback (internal/process/next_tick.js:*:*) + at processTicksAndRejections (internal/process/next_tick.js:*:*) [stdin]:1 var ______________________________________________; throw 10