From c5ab3408b1fd87347c24ed6879e577fad9c602ca Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 23 Dec 2018 00:02:14 +0800 Subject: [PATCH] src: refactor tickInfo access - Wrap access to tickInfo fields in functions - Rename `kHasScheduled` to `kHasTickScheduled` and `kHasPromiseRejections` to `kHasRejectionToWarn` for clarity - note the latter will be set to false if the rejection does not lead to a warning so the previous description is not accurate. - Set `kHasRejectionToWarn` in JS land of relying on C++ to use an implict contract (return value of the promise rejection handler) to set it, as the decision is made entirely in JS land. - Destructure promise reject event constants. 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 | 22 +++++++++----- lib/internal/process/promises.js | 48 +++++++++++++++++++++++-------- src/callback_scope.cc | 4 +-- src/env-inl.h | 12 +++----- src/env.h | 10 +++---- src/node_task_queue.cc | 10 ++----- 6 files changed, 63 insertions(+), 43 deletions(-) diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index 00bba254718684..0bfe168975acb3 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -11,6 +11,8 @@ const { } = internalBinding('task_queue'); const { + setHasRejectionToWarn, + hasRejectionToWarn, promiseRejectHandler, emitPromiseRejectionWarnings } = require('internal/process/promises'); @@ -30,15 +32,21 @@ const { ERR_INVALID_CALLBACK } = require('internal/errors').codes; const FixedQueue = require('internal/fixed_queue'); // *Must* match Environment::TickInfo::Fields in src/env.h. -const kHasScheduled = 0; -const kHasPromiseRejections = 1; +const kHasTickScheduled = 0; + +function hasTickScheduled() { + return tickInfo[kHasTickScheduled] === 1; +} +function setHasTickScheduled(value) { + tickInfo[kHasTickScheduled] = value ? 1 : 0; +} const queue = new FixedQueue(); function runNextTicks() { - if (tickInfo[kHasScheduled] === 0 && tickInfo[kHasPromiseRejections] === 0) + if (!hasTickScheduled() && !hasRejectionToWarn()) runMicrotasks(); - if (tickInfo[kHasScheduled] === 0 && tickInfo[kHasPromiseRejections] === 0) + if (!hasTickScheduled() && !hasRejectionToWarn()) return; internalTickCallback(); @@ -70,10 +78,10 @@ function internalTickCallback() { emitAfter(asyncId); } - tickInfo[kHasScheduled] = 0; + setHasTickScheduled(false); runMicrotasks(); } while (!queue.isEmpty() || emitPromiseRejectionWarnings()); - tickInfo[kHasPromiseRejections] = 0; + setHasRejectionToWarn(false); } class TickObject { @@ -119,7 +127,7 @@ function nextTick(callback) { } if (queue.isEmpty()) - tickInfo[kHasScheduled] = 1; + setHasTickScheduled(true); queue.push(new TickObject(callback, args, getDefaultTriggerAsyncId())); } diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index df4e8188fe92b4..047a9d43aabf2b 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -2,24 +2,45 @@ const { safeToString } = internalBinding('util'); const { - promiseRejectEvents + tickInfo, + promiseRejectEvents: { + kPromiseRejectWithNoHandler, + kPromiseHandlerAddedAfterReject, + kPromiseResolveAfterResolved, + kPromiseRejectAfterResolved + } } = internalBinding('task_queue'); +// *Must* match Environment::TickInfo::Fields in src/env.h. +const kHasRejectionToWarn = 1; + const maybeUnhandledPromises = new WeakMap(); const pendingUnhandledRejections = []; const asyncHandledRejections = []; let lastPromiseId = 0; +function setHasRejectionToWarn(value) { + tickInfo[kHasRejectionToWarn] = value ? 1 : 0; +} + +function hasRejectionToWarn() { + return tickInfo[kHasRejectionToWarn] === 1; +} + function promiseRejectHandler(type, promise, reason) { switch (type) { - case promiseRejectEvents.kPromiseRejectWithNoHandler: - return unhandledRejection(promise, reason); - case promiseRejectEvents.kPromiseHandlerAddedAfterReject: - return handledRejection(promise); - case promiseRejectEvents.kPromiseResolveAfterResolved: - return resolveError('resolve', promise, reason); - case promiseRejectEvents.kPromiseRejectAfterResolved: - return resolveError('reject', promise, reason); + case kPromiseRejectWithNoHandler: + unhandledRejection(promise, reason); + break; + case kPromiseHandlerAddedAfterReject: + handledRejection(promise); + break; + case kPromiseResolveAfterResolved: + resolveError('resolve', promise, reason); + break; + case kPromiseRejectAfterResolved: + resolveError('reject', promise, reason); + break; } } @@ -38,7 +59,7 @@ function unhandledRejection(promise, reason) { warned: false }); pendingUnhandledRejections.push(promise); - return true; + setHasRejectionToWarn(true); } function handledRejection(promise) { @@ -54,10 +75,11 @@ function handledRejection(promise) { warning.name = 'PromiseRejectionHandledWarning'; warning.id = uid; asyncHandledRejections.push({ promise, warning }); - return true; + setHasRejectionToWarn(true); + return; } } - return false; + setHasRejectionToWarn(false); } const unhandledRejectionErrName = 'UnhandledPromiseRejectionWarning'; @@ -121,6 +143,8 @@ function emitPromiseRejectionWarnings() { } module.exports = { + hasRejectionToWarn, + setHasRejectionToWarn, promiseRejectHandler, emitPromiseRejectionWarnings }; diff --git a/src/callback_scope.cc b/src/callback_scope.cc index 0757b61061c3e7..89fd4d6dd433d0 100644 --- a/src/callback_scope.cc +++ b/src/callback_scope.cc @@ -95,7 +95,7 @@ void InternalCallbackScope::Close() { Environment::TickInfo* tick_info = env_->tick_info(); if (!env_->can_call_into_js()) return; - if (!tick_info->has_scheduled()) { + if (!tick_info->has_tick_scheduled()) { env_->isolate()->RunMicrotasks(); } @@ -106,7 +106,7 @@ void InternalCallbackScope::Close() { CHECK_EQ(env_->trigger_async_id(), 0); } - if (!tick_info->has_scheduled() && !tick_info->has_promise_rejections()) { + if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn()) { return; } diff --git a/src/env-inl.h b/src/env-inl.h index e323d8707e676f..e63d0d38aa2aea 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -265,16 +265,12 @@ inline AliasedBuffer& Environment::TickInfo::fields() { return fields_; } -inline bool Environment::TickInfo::has_scheduled() const { - return fields_[kHasScheduled] == 1; +inline bool Environment::TickInfo::has_tick_scheduled() const { + return fields_[kHasTickScheduled] == 1; } -inline bool Environment::TickInfo::has_promise_rejections() const { - return fields_[kHasPromiseRejections] == 1; -} - -inline void Environment::TickInfo::promise_rejections_toggle_on() { - fields_[kHasPromiseRejections] = 1; +inline bool Environment::TickInfo::has_rejection_to_warn() const { + return fields_[kHasRejectionToWarn] == 1; } inline void Environment::AssignToContext(v8::Local context, diff --git a/src/env.h b/src/env.h index 14a59980051eab..f75742297d551d 100644 --- a/src/env.h +++ b/src/env.h @@ -572,18 +572,16 @@ class Environment { class TickInfo { public: inline AliasedBuffer& fields(); - inline bool has_scheduled() const; - inline bool has_promise_rejections() const; - - inline void promise_rejections_toggle_on(); + inline bool has_tick_scheduled() const; + inline bool has_rejection_to_warn() const; private: friend class Environment; // So we can call the constructor. inline explicit TickInfo(v8::Isolate* isolate); enum Fields { - kHasScheduled, - kHasPromiseRejections, + kHasTickScheduled = 0, + kHasRejectionToWarn, kFieldsCount }; diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index f65f420081d6e8..5bfa281068e755 100644 --- a/src/node_task_queue.cc +++ b/src/node_task_queue.cc @@ -17,7 +17,6 @@ using v8::kPromiseRejectAfterResolved; using v8::kPromiseRejectWithNoHandler; using v8::kPromiseResolveAfterResolved; using v8::Local; -using v8::MaybeLocal; using v8::Number; using v8::Object; using v8::Promise; @@ -80,13 +79,8 @@ static void PromiseRejectCallback(PromiseRejectMessage message) { } Local args[] = { type, promise, value }; - MaybeLocal ret = callback->Call(env->context(), - Undefined(isolate), - arraysize(args), - args); - - if (!ret.IsEmpty() && ret.ToLocalChecked()->IsTrue()) - env->tick_info()->promise_rejections_toggle_on(); + USE(callback->Call( + env->context(), Undefined(isolate), arraysize(args), args)); } static void InitializePromiseRejectCallback(