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);