From 9b8e1c2e4f0a4254b295316b10136a28cc36db4c Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 29 Jan 2018 18:22:12 -0500 Subject: [PATCH] timers: refactor error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of using nextTick to process failed lists, just attempt to process them again from C++ if the process is still alive. This also allows the removal of domain specific code in timers. The current behaviour is not quite ideal as it means that all lists after the failed one will process on an arbitrary nextTick, even if they're — say — not due to fire for another 2 days... PR-URL: https://github.com/nodejs/node/pull/18486 Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater Reviewed-By: Jeremiah Senkpiel --- lib/timers.js | 61 ++++++------------- src/env-inl.h | 8 +++ src/env.h | 3 + src/node.cc | 5 ++ src/timer_wrap.cc | 9 ++- .../test-timers-unref-throw-then-ref.js | 17 ++++++ 6 files changed, 58 insertions(+), 45 deletions(-) create mode 100644 test/parallel/test-timers-unref-throw-then-ref.js diff --git a/lib/timers.js b/lib/timers.js index 8d061ab0147e3d..a91de76d7f95bd 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -212,7 +212,6 @@ function TimersList(msecs, unrefed) { this._idlePrev = this; // prevent any unnecessary hidden class changes. this._unrefed = unrefed; this.msecs = msecs; - this.nextTick = false; const timer = this._timer = new TimerWrap(); timer._list = this; @@ -228,12 +227,6 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout() { var list = this._list; var msecs = list.msecs; - if (list.nextTick) { - list.nextTick = false; - process.nextTick(listOnTimeoutNT, list); - return; - } - debug('timeout callback %d', msecs); var now = TimerWrap.now(); @@ -252,7 +245,7 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout() { } this.start(timeRemaining); debug('%d list wait because diff is %d', msecs, diff); - return; + return true; } // The actual logic for when a timeout happens. @@ -289,10 +282,10 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout() { // Do not close the underlying handle if its ownership has changed // (e.g it was unrefed in its callback). - if (this.owner) - return; + if (!this.owner) + this.close(); - this.close(); + return true; }; @@ -318,34 +311,10 @@ function tryOnTimeout(timer, list) { timer._destroyed = true; } } - - if (!threw) return; - - // Postpone all later list events to next tick. We need to do this - // so that the events are called in the order they were created. - const lists = list._unrefed === true ? unrefedLists : refedLists; - for (var key in lists) { - if (key > list.msecs) { - lists[key].nextTick = true; - } - } - // We need to continue processing after domain error handling - // is complete, but not by using whatever domain was left over - // when the timeout threw its exception. - const domain = process.domain; - process.domain = null; - // If we threw, we need to process the rest of the list in nextTick. - process.nextTick(listOnTimeoutNT, list); - process.domain = domain; } } -function listOnTimeoutNT(list) { - list._timer[kOnTimeout](); -} - - // A convenience function for re-using TimerWrap handles more easily. // // This mostly exists to fix https://github.com/nodejs/node/issues/1264. @@ -550,17 +519,21 @@ exports.clearInterval = function(timer) { function unrefdHandle() { - // Don't attempt to call the callback if it is not a function. - if (typeof this.owner._onTimeout === 'function') { - ontimeout(this.owner); + try { + // Don't attempt to call the callback if it is not a function. + if (typeof this.owner._onTimeout === 'function') { + ontimeout(this.owner); + } + } finally { + // Make sure we clean up if the callback is no longer a function + // even if the timer is an interval. + if (!this.owner._repeat || + typeof this.owner._onTimeout !== 'function') { + this.owner.close(); + } } - // Make sure we clean up if the callback is no longer a function - // even if the timer is an interval. - if (!this.owner._repeat || - typeof this.owner._onTimeout !== 'function') { - this.owner.close(); - } + return true; } diff --git a/src/env-inl.h b/src/env-inl.h index 545a92ba17f47c..5643fffb6f8b6a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -264,6 +264,10 @@ inline bool Environment::TickInfo::has_scheduled() const { return fields_[kHasScheduled] == 1; } +inline bool Environment::TickInfo::has_thrown() const { + return fields_[kHasThrown] == 1; +} + inline bool Environment::TickInfo::has_promise_rejections() const { return fields_[kHasPromiseRejections] == 1; } @@ -272,6 +276,10 @@ inline void Environment::TickInfo::promise_rejections_toggle_on() { fields_[kHasPromiseRejections] = 1; } +inline void Environment::TickInfo::set_has_thrown(bool state) { + fields_[kHasThrown] = state ? 1 : 0; +} + inline void Environment::AssignToContext(v8::Local context, const ContextInfo& info) { context->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, this); diff --git a/src/env.h b/src/env.h index 2ce7069f47f72e..7e0152916161bf 100644 --- a/src/env.h +++ b/src/env.h @@ -484,8 +484,10 @@ class Environment { inline AliasedBuffer& fields(); inline bool has_scheduled() const; inline bool has_promise_rejections() const; + inline bool has_thrown() const; inline void promise_rejections_toggle_on(); + inline void set_has_thrown(bool state); private: friend class Environment; // So we can call the constructor. @@ -494,6 +496,7 @@ class Environment { enum Fields { kHasScheduled, kHasPromiseRejections, + kHasThrown, kFieldsCount }; diff --git a/src/node.cc b/src/node.cc index dfe4125bd092fe..4e3638b886fcb8 100644 --- a/src/node.cc +++ b/src/node.cc @@ -937,6 +937,10 @@ InternalCallbackScope::InternalCallbackScope(Environment* env, AsyncWrap::EmitBefore(env, asyncContext.async_id); } + if (!IsInnerMakeCallback()) { + env->tick_info()->set_has_thrown(false); + } + env->async_hooks()->push_async_ids(async_context_.async_id, async_context_.trigger_async_id); pushed_ids_ = true; @@ -984,6 +988,7 @@ void InternalCallbackScope::Close() { Local process = env_->process_object(); if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { + env_->tick_info()->set_has_thrown(true); failed_ = true; } } diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index 1725cf30e71d04..8b596eb4352358 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -138,7 +138,14 @@ class TimerWrap : public HandleWrap { Environment* env = wrap->env(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); - wrap->MakeCallback(kOnTimeout, 0, nullptr); + Local ret; + do { + ret = wrap->MakeCallback(kOnTimeout, 0, nullptr).ToLocalChecked(); + } while (ret->IsUndefined() && + !env->tick_info()->has_thrown() && + wrap->object()->Get(env->context(), + env->owner_string()).ToLocalChecked() + ->IsUndefined()); } static void Now(const FunctionCallbackInfo& args) { diff --git a/test/parallel/test-timers-unref-throw-then-ref.js b/test/parallel/test-timers-unref-throw-then-ref.js new file mode 100644 index 00000000000000..d3ae27e83551c7 --- /dev/null +++ b/test/parallel/test-timers-unref-throw-then-ref.js @@ -0,0 +1,17 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +process.once('uncaughtException', common.expectsError({ + message: 'Timeout Error' +})); + +let called = false; +const t = setTimeout(() => { + assert(!called); + called = true; + t.ref(); + throw new Error('Timeout Error'); +}, 1).unref(); + +setTimeout(common.mustCall(), 1);