diff --git a/lib/internal/timers.js b/lib/internal/timers.js index aa061be3dbf845..df2a88558fd6e3 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -85,7 +85,6 @@ Timeout.prototype[refreshFnSymbol] = function refresh() { // Would be more ideal with uv_timer_again(), however that API does not // cause libuv's sorted timers data structure (a binary heap at the time // of writing) to re-sort itself. This causes ordering inconsistencies. - this._handle.stop(); this._handle.start(this._idleTimeout); } else if (this[unrefedSymbol]) { getTimers()._unrefActive(this); diff --git a/lib/timers.js b/lib/timers.js index db43a7491de72f..798ffe7d82dfe8 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -210,7 +210,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; @@ -222,19 +221,11 @@ function TimersList(msecs, unrefed) { // adds listOnTimeout to the C++ object prototype, as // V8 would not inline it otherwise. -TimerWrap.prototype[kOnTimeout] = function listOnTimeout() { - var list = this._list; - var msecs = list.msecs; - - if (list.nextTick) { - list.nextTick = false; - process.nextTick(listOnTimeoutNT, list); - return; - } +TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) { + const list = this._list; + const msecs = list.msecs; debug('timeout callback %d', msecs); - - var now = TimerWrap.now(); debug('now: %d', now); var diff, timer; @@ -245,12 +236,12 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout() { // This happens if there are more timers scheduled for later in the list. if (diff < msecs) { var timeRemaining = msecs - (TimerWrap.now() - timer._idleStart); - if (timeRemaining < 0) { + if (timeRemaining <= 0) { timeRemaining = 1; } this.start(timeRemaining); debug('%d list wait because diff is %d', msecs, diff); - return; + return true; } // The actual logic for when a timeout happens. @@ -287,10 +278,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; }; @@ -316,34 +307,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. @@ -353,7 +320,7 @@ function listOnTimeoutNT(list) { function reuse(item) { L.remove(item); - var list = refedLists[item._idleTimeout]; + const list = refedLists[item._idleTimeout]; // if empty - reuse the watcher if (list !== undefined && L.isEmpty(list)) { debug('reuse hit'); @@ -377,7 +344,7 @@ const unenroll = exports.unenroll = function(item) { item._destroyed = true; } - var handle = reuse(item); + const handle = reuse(item); if (handle !== null) { debug('unenroll: list empty'); handle.close(); @@ -449,11 +416,12 @@ setTimeout[internalUtil.promisify.custom] = function(after, value) { exports.setTimeout = setTimeout; -function ontimeout(timer) { - var args = timer._timerArgs; +function ontimeout(timer, start) { + const args = timer._timerArgs; if (typeof timer._onTimeout !== 'function') return promiseResolve(timer._onTimeout, args[0]); - const start = TimerWrap.now(); + if (start === undefined && timer._repeat) + start = TimerWrap.now(); if (!args) timer._onTimeout(); else @@ -537,18 +505,22 @@ 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); +function unrefdHandle(now) { + try { + // Don't attempt to call the callback if it is not a function. + if (typeof this.owner._onTimeout === 'function') { + ontimeout(this.owner, now); + } + } 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; } @@ -556,7 +528,7 @@ Timeout.prototype.unref = function() { if (this._handle) { this._handle.unref(); } else if (typeof this._onTimeout === 'function') { - var now = TimerWrap.now(); + const now = TimerWrap.now(); if (!this._idleStart) this._idleStart = now; var delay = this._idleStart + this._idleTimeout - now; if (delay < 0) delay = 0; @@ -567,7 +539,7 @@ Timeout.prototype.unref = function() { return; } - var handle = reuse(this); + const handle = reuse(this); if (handle !== null) { handle._list = undefined; } 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 d73be8156ecebf..3ed9677131903f 100644 --- a/src/env.h +++ b/src/env.h @@ -479,8 +479,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. @@ -489,6 +491,7 @@ class Environment { enum Fields { kHasScheduled, kHasPromiseRejections, + kHasThrown, kFieldsCount }; diff --git a/src/node.cc b/src/node.cc index bcb764e400aea7..af2dcc56000b5e 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..f1b423d3669ed5 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -37,6 +37,7 @@ using v8::FunctionTemplate; using v8::HandleScope; using v8::Integer; using v8::Local; +using v8::Number; using v8::Object; using v8::String; using v8::Value; @@ -138,19 +139,32 @@ 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; + Local args[1]; + do { + args[0] = GetNow(env); + ret = wrap->MakeCallback(kOnTimeout, 1, args).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) { Environment* env = Environment::GetCurrent(args); + args.GetReturnValue().Set(GetNow(env)); + } + + static Local GetNow(Environment* env) { uv_update_time(env->event_loop()); uint64_t now = uv_now(env->event_loop()); CHECK(now >= env->timer_base()); now -= env->timer_base(); if (now <= 0xfffffff) - args.GetReturnValue().Set(static_cast(now)); + return Integer::New(env->isolate(), static_cast(now)); else - args.GetReturnValue().Set(static_cast(now)); + return Number::New(env->isolate(), static_cast(now)); } uv_timer_t handle_; 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); diff --git a/test/sequential/test-timers-set-interval-excludes-callback-duration.js b/test/sequential/test-timers-set-interval-excludes-callback-duration.js index d47659d7b395ab..cb60d7e45270d6 100644 --- a/test/sequential/test-timers-set-interval-excludes-callback-duration.js +++ b/test/sequential/test-timers-set-interval-excludes-callback-duration.js @@ -9,9 +9,16 @@ const t = setInterval(() => { cntr++; if (cntr === 1) { common.busyLoop(100); + // ensure that the event loop passes before the second interval + setImmediate(() => assert.strictEqual(cntr, 1)); first = Timer.now(); } else if (cntr === 2) { assert(Timer.now() - first < 100); clearInterval(t); } }, 100); +const t2 = setInterval(() => { + if (cntr === 2) { + clearInterval(t2); + } +}, 100);