diff options
| -rw-r--r-- | benchmark/timers/timers-cancel-unpooled.js | 14 | ||||
| -rw-r--r-- | benchmark/timers/timers-insert-unpooled.js | 16 | ||||
| -rw-r--r-- | lib/internal/timers.js | 22 | ||||
| -rw-r--r-- | lib/timers.js | 375 | ||||
| -rw-r--r-- | src/timer_wrap.cc | 20 | ||||
| -rw-r--r-- | test/async-hooks/test-timerwrap.setInterval.js | 2 | ||||
| -rw-r--r-- | test/async-hooks/test-timerwrap.setTimeout.js | 23 | ||||
| -rw-r--r-- | test/parallel/test-handle-wrap-isrefed.js | 20 | ||||
| -rw-r--r-- | test/parallel/test-timers-next-tick.js | 15 | ||||
| -rw-r--r-- | test/parallel/test-timers-same-timeout-wrong-list-deleted.js | 34 | ||||
| -rw-r--r-- | test/parallel/test-timers-timeout-to-interval.js | 2 | ||||
| -rw-r--r-- | test/parallel/test-timers-unref-leak.js | 14 | ||||
| -rw-r--r-- | test/parallel/test-timers-unref-reuse-no-exposed-list.js | 14 |
13 files changed, 242 insertions, 329 deletions
diff --git a/benchmark/timers/timers-cancel-unpooled.js b/benchmark/timers/timers-cancel-unpooled.js index 7b46bad719..1daf68bde0 100644 --- a/benchmark/timers/timers-cancel-unpooled.js +++ b/benchmark/timers/timers-cancel-unpooled.js @@ -4,18 +4,26 @@ const assert = require('assert'); const bench = common.createBenchmark(main, { n: [1e6], + direction: ['start', 'end'] }); -function main({ n }) { +function main({ n, direction }) { const timersList = []; for (var i = 0; i < n; i++) { timersList.push(setTimeout(cb, i + 1)); } + var j; bench.start(); - for (var j = 0; j < n + 1; j++) { - clearTimeout(timersList[j]); + if (direction === 'start') { + for (j = 0; j < n; j++) { + clearTimeout(timersList[j]); + } + } else { + for (j = n - 1; j >= 0; j--) { + clearTimeout(timersList[j]); + } } bench.end(n); } diff --git a/benchmark/timers/timers-insert-unpooled.js b/benchmark/timers/timers-insert-unpooled.js index 1f1c5155a7..232cc7c31a 100644 --- a/benchmark/timers/timers-insert-unpooled.js +++ b/benchmark/timers/timers-insert-unpooled.js @@ -4,18 +4,26 @@ const assert = require('assert'); const bench = common.createBenchmark(main, { n: [1e6], + direction: ['start', 'end'] }); -function main({ n }) { +function main({ direction, n }) { const timersList = []; + var i; bench.start(); - for (var i = 0; i < n; i++) { - timersList.push(setTimeout(cb, i + 1)); + if (direction === 'start') { + for (i = 1; i <= n; i++) { + timersList.push(setTimeout(cb, i)); + } + } else { + for (i = n; i > 0; i--) { + timersList.push(setTimeout(cb, i)); + } } bench.end(n); - for (var j = 0; j < n + 1; j++) { + for (var j = 0; j < n; j++) { clearTimeout(timersList[j]); } } diff --git a/lib/internal/timers.js b/lib/internal/timers.js index 801fce1cc3..44ebc65dc1 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -19,7 +19,7 @@ const { // Timeout values > TIMEOUT_MAX are set to 1. const TIMEOUT_MAX = 2 ** 31 - 1; -const unrefedSymbol = Symbol('unrefed'); +const kRefed = Symbol('refed'); module.exports = { TIMEOUT_MAX, @@ -27,6 +27,7 @@ module.exports = { async_id_symbol, trigger_async_id_symbol, Timeout, + kRefed, initAsyncResource, setUnrefTimeout, validateTimerDuration @@ -50,7 +51,7 @@ function initAsyncResource(resource, type) { // Timer constructor function. // The entire prototype is defined in lib/timers.js -function Timeout(callback, after, args, isRepeat, isUnrefed) { +function Timeout(callback, after, args, isRepeat) { after *= 1; // coalesce to number or NaN if (!(after >= 1 && after <= TIMEOUT_MAX)) { if (after > TIMEOUT_MAX) { @@ -62,7 +63,6 @@ function Timeout(callback, after, args, isRepeat, isUnrefed) { after = 1; // schedule on next tick, follows browser behavior } - this._called = false; this._idleTimeout = after; this._idlePrev = this; this._idleNext = this; @@ -75,22 +75,16 @@ function Timeout(callback, after, args, isRepeat, isUnrefed) { this._repeat = isRepeat ? after : null; this._destroyed = false; - this[unrefedSymbol] = isUnrefed; + this[kRefed] = null; initAsyncResource(this, 'Timeout'); } Timeout.prototype.refresh = function() { - if (this._handle) { - // 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.start(this._idleTimeout); - } else if (this[unrefedSymbol]) { - getTimers()._unrefActive(this); - } else { + if (this[kRefed]) getTimers().active(this); - } + else + getTimers()._unrefActive(this); return this; }; @@ -122,7 +116,7 @@ function setUnrefTimeout(callback, after, arg1, arg2, arg3) { break; } - const timer = new Timeout(callback, after, args, false, true); + const timer = new Timeout(callback, after, args, false); getTimers()._unrefActive(timer); return timer; diff --git a/lib/timers.js b/lib/timers.js index e7f13924db..3d200344a4 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -26,16 +26,17 @@ const { setupTimers, } = process.binding('timer_wrap'); const L = require('internal/linkedlist'); +const PriorityQueue = require('internal/priority_queue'); const { async_id_symbol, trigger_async_id_symbol, Timeout, + kRefed, initAsyncResource, validateTimerDuration } = require('internal/timers'); const internalUtil = require('internal/util'); const { createPromise, promiseResolve } = process.binding('util'); -const assert = require('assert'); const util = require('util'); const { ERR_INVALID_CALLBACK } = require('internal/errors').codes; const debug = util.debuglog('timer'); @@ -55,8 +56,6 @@ const kHasOutstanding = 2; const [immediateInfo, toggleImmediateRef] = setupTimers(processImmediate, processTimers); -const kRefed = Symbol('refed'); - // HOW and WHY the timers implementation works the way it does. // // Timers are crucial to Node.js. Internally, any TCP I/O connection creates a @@ -85,20 +84,17 @@ const kRefed = Symbol('refed'); // // Object maps are kept which contain linked lists keyed by their duration in // milliseconds. -// The linked lists within also have some meta-properties, one of which is a -// TimerWrap C++ handle, which makes the call after the duration to process the -// list it is attached to. // /* eslint-disable node-core/non-ascii-character */ // // ╔════ > Object Map // ║ // ╠══ -// ║ refedLists: { '40': { }, '320': { etc } } (keys of millisecond duration) -// ╚══ ┌─────────┘ +// ║ lists: { '40': { }, '320': { etc } } (keys of millisecond duration) +// ╚══ ┌────┘ // │ // ╔══ │ -// ║ TimersList { _idleNext: { }, _idlePrev: (self), _timer: (TimerWrap) } +// ║ TimersList { _idleNext: { }, _idlePrev: (self) } // ║ ┌────────────────┘ // ║ ╔══ │ ^ // ║ ║ { _idleNext: { }, _idlePrev: { }, _onTimeout: (callback) } @@ -126,36 +122,73 @@ const kRefed = Symbol('refed'); // always be due to timeout at a later time. // // Less-than constant time operations are thus contained in two places: -// TimerWrap's backing libuv timers implementation (a performant heap-based -// queue), and the object map lookup of a specific list by the duration of -// timers within (or creation of a new list). -// However, these operations combined have shown to be trivial in comparison to -// other alternative timers architectures. +// The PriorityQueue — an efficient binary heap implementation that does all +// operations in worst-case O(log n) time — which manages the order of expiring +// Timeout lists and the object map lookup of a specific list by the duration of +// timers within (or creation of a new list). However, these operations combined +// have shown to be trivial in comparison to other timers architectures. -// Object maps containing linked lists of timers, keyed and sorted by their +// Object map containing linked lists of timers, keyed and sorted by their // duration in milliseconds. // -// The difference between these two objects is that the former contains timers -// that will keep the process open if they are the only thing left, while the -// latter will not. -// // - key = time in milliseconds // - value = linked list -const refedLists = Object.create(null); -const unrefedLists = Object.create(null); +const lists = Object.create(null); + +// This is a priority queue with a custom sorting function that first compares +// the expiry times of two lists and if they're the same then compares their +// individual IDs to determine which list was created first. +const queue = new PriorityQueue(compareTimersLists, setPosition); + +function compareTimersLists(a, b) { + const expiryDiff = a.expiry - b.expiry; + if (expiryDiff === 0) { + if (a.id < b.id) + return -1; + if (a.id > b.id) + return 1; + } + return expiryDiff; +} + +function setPosition(node, pos) { + node.priorityQueuePosition = pos; +} +let handle = null; +let nextExpiry = Infinity; + +let timerListId = Number.MIN_SAFE_INTEGER; +let refCount = 0; + +function incRefCount() { + if (refCount++ === 0) + handle.ref(); +} + +function decRefCount() { + if (--refCount === 0) + handle.unref(); +} + +function createHandle(refed) { + debug('initial run, creating TimerWrap handle'); + handle = new TimerWrap(); + if (!refed) + handle.unref(); +} // Schedule or re-schedule a timer. // The item must have been enroll()'d first. const active = exports.active = function(item) { - insert(item, false); + insert(item, true, TimerWrap.now()); }; // Internal APIs that need timeouts should use `_unrefActive()` instead of // `active()` so that they do not unnecessarily keep the process open. exports._unrefActive = function(item) { - insert(item, true); + insert(item, false, TimerWrap.now()); }; @@ -164,23 +197,27 @@ exports._unrefActive = function(item) { // Appends a timer onto the end of an existing timers list, or creates a new // TimerWrap backed list if one does not already exist for the specified timeout // duration. -function insert(item, unrefed, start) { +function insert(item, refed, start) { const msecs = item._idleTimeout; - if (msecs < 0 || msecs === undefined) return; - - if (typeof start === 'number') { - item._idleStart = start; - } else { - item._idleStart = TimerWrap.now(); - } + if (msecs < 0 || msecs === undefined) + return; - const lists = unrefed === true ? unrefedLists : refedLists; + item._idleStart = start; // Use an existing list if there is one, otherwise we need to make a new one. var list = lists[msecs]; if (list === undefined) { debug('no %d list was found in insert, creating a new one', msecs); - lists[msecs] = list = new TimersList(msecs, unrefed); + const expiry = start + msecs; + lists[msecs] = list = new TimersList(expiry, msecs); + queue.insert(list); + + if (nextExpiry > expiry) { + if (handle === null) + createHandle(refed); + handle.start(msecs); + nextExpiry = expiry; + } } if (!item[async_id_symbol] || item._destroyed) { @@ -188,32 +225,55 @@ function insert(item, unrefed, start) { initAsyncResource(item, 'Timeout'); } + if (refed === !item[kRefed]) { + if (refed) + incRefCount(); + else + decRefCount(); + } + item[kRefed] = refed; + L.append(list, item); - assert(!L.isEmpty(list)); // list is not empty } -function TimersList(msecs, unrefed) { +function TimersList(expiry, msecs) { this._idleNext = this; // Create the list with the linkedlist properties to this._idlePrev = this; // prevent any unnecessary hidden class changes. - this._unrefed = unrefed; + this.expiry = expiry; + this.id = timerListId++; this.msecs = msecs; - - const timer = this._timer = new TimerWrap(); - timer._list = this; - - if (unrefed === true) - timer.unref(); - timer.start(msecs); + this.priorityQueuePosition = null; } +const { _tickCallback: runNextTicks } = process; function processTimers(now) { - if (this.owner) - return unrefdHandle(this.owner, now); - return listOnTimeout(this, now); + debug('process timer lists %d', now); + nextExpiry = Infinity; + + let list, ran; + while (list = queue.peek()) { + if (list.expiry > now) + break; + if (ran) + runNextTicks(); + listOnTimeout(list, now); + ran = true; + } + + if (refCount > 0) + handle.ref(); + else + handle.unref(); + + if (list !== undefined) { + nextExpiry = list.expiry; + handle.start(Math.max(nextExpiry - TimerWrap.now(), 1)); + } + + return true; } -function listOnTimeout(handle, now) { - const list = handle._list; +function listOnTimeout(list, now) { const msecs = list.msecs; debug('timeout callback %d', msecs); @@ -226,78 +286,69 @@ function listOnTimeout(handle, now) { // Check if this loop iteration is too early for the next timer. // 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) { - timeRemaining = 1; - } - handle.start(timeRemaining); + list.expiry = timer._idleStart + msecs; + list.id = timerListId++; + queue.percolateDown(1); debug('%d list wait because diff is %d', msecs, diff); - return true; + return; } // The actual logic for when a timeout happens. - L.remove(timer); - assert(timer !== L.peek(list)); + + const asyncId = timer[async_id_symbol]; if (!timer._onTimeout) { - if (destroyHooksExist() && !timer._destroyed && - typeof timer[async_id_symbol] === 'number') { - emitDestroy(timer[async_id_symbol]); + if (timer[kRefed]) + refCount--; + timer[kRefed] = null; + + if (destroyHooksExist() && !timer._destroyed) { + emitDestroy(asyncId); timer._destroyed = true; } continue; } + emitBefore(asyncId, timer[trigger_async_id_symbol]); + tryOnTimeout(timer); + + emitAfter(asyncId); } // If `L.peek(list)` returned nothing, the list was either empty or we have // called all of the timer timeouts. - // As such, we can remove the list and clean up the TimerWrap C++ handle. + // As such, we can remove the list from the object map and the PriorityQueue. debug('%d list empty', msecs); - assert(L.isEmpty(list)); - - // Either refedLists[msecs] or unrefedLists[msecs] may have been removed and - // recreated since the reference to `list` was created. Make sure they're - // the same instance of the list before destroying. - if (list._unrefed === true && list === unrefedLists[msecs]) { - delete unrefedLists[msecs]; - } else if (list === refedLists[msecs]) { - delete refedLists[msecs]; - } - - // Do not close the underlying handle if its ownership has changed - // (e.g it was unrefed in its callback). - if (!handle.owner) - handle.close(); - return true; + // The current list may have been removed and recreated since the reference + // to `list` was created. Make sure they're the same instance of the list + // before destroying. + if (list === lists[msecs]) { + delete lists[msecs]; + queue.shift(); + } } // An optimization so that the try/finally only de-optimizes (since at least v8 // 4.7) what is in this smaller function. function tryOnTimeout(timer, start) { - timer._called = true; - const timerAsyncId = (typeof timer[async_id_symbol] === 'number') ? - timer[async_id_symbol] : null; - var threw = true; - if (timerAsyncId !== null) - emitBefore(timerAsyncId, timer[trigger_async_id_symbol]); if (start === undefined && timer._repeat) start = TimerWrap.now(); try { ontimeout(timer); - threw = false; } finally { - if (timerAsyncId !== null) { - if (!threw) - emitAfter(timerAsyncId); - if (timer._repeat) { - rearm(timer, start); - } else if (destroyHooksExist() && !timer._destroyed) { - emitDestroy(timerAsyncId); + if (timer._repeat) { + rearm(timer, start); + } else { + if (timer[kRefed]) + refCount--; + timer[kRefed] = null; + + if (destroyHooksExist() && !timer._destroyed) { + emitDestroy(timer[async_id_symbol]); timer._destroyed = true; } } @@ -305,43 +356,35 @@ function tryOnTimeout(timer, start) { } -// A convenience function for re-using TimerWrap handles more easily. -// -// This mostly exists to fix https://github.com/nodejs/node/issues/1264. -// Handles in libuv take at least one `uv_run` to be registered as unreferenced. -// Re-using an existing handle allows us to skip that, so that a second `uv_run` -// will return no active handles, even when running `setTimeout(fn).unref()`. -function reuse(item) { - L.remove(item); - - const list = refedLists[item._idleTimeout]; - // if empty - reuse the watcher - if (list !== undefined && L.isEmpty(list)) { - debug('reuse hit'); - list._timer.stop(); - delete refedLists[item._idleTimeout]; - return list._timer; - } - - return null; -} - - // Remove a timer. Cancels the timeout and resets the relevant timer properties. function unenroll(item) { // Fewer checks may be possible, but these cover everything. if (destroyHooksExist() && - typeof item[async_id_symbol] === 'number' && + item[async_id_symbol] !== undefined && !item._destroyed) { emitDestroy(item[async_id_symbol]); item._destroyed = true; } - const handle = reuse(item); - if (handle !== null) { - debug('unenroll: list empty'); - handle.close(); + L.remove(item); + + // We only delete refed lists because unrefed ones are incredibly likely + // to come from http and be recreated shortly after. + // TODO: Long-term this could instead be handled by creating an internal + // clearTimeout that makes it clear that the list should not be deleted. + // That function could then be used by http and other similar modules. + if (item[kRefed]) { + const list = lists[item._idleTimeout]; + if (list !== undefined && L.isEmpty(list)) { + debug('unenroll: list empty'); + queue.removeAt(list.priorityQueuePosition); + delete lists[list.msecs]; + } + + decRefCount(); } + item[kRefed] = null; + // if active is called later, then we want to make sure not to insert again item._idleTimeout = -1; } @@ -403,7 +446,7 @@ function setTimeout(callback, after, arg1, arg2, arg3) { break; } - const timeout = new Timeout(callback, after, args, false, false); + const timeout = new Timeout(callback, after, args, false); active(timeout); return timeout; @@ -411,7 +454,7 @@ function setTimeout(callback, after, arg1, arg2, arg3) { setTimeout[internalUtil.promisify.custom] = function(after, value) { const promise = createPromise(); - const timeout = new Timeout(promise, after, [value], false, false); + const timeout = new Timeout(promise, after, [value], false); active(timeout); return promise; @@ -430,36 +473,20 @@ function ontimeout(timer) { Reflect.apply(timer._onTimeout, timer, args); } -function rearm(timer, start = TimerWrap.now()) { +function rearm(timer, start) { // // Do not re-arm unenroll'd or closed timers. - if (timer._idleTimeout === -1) return; - - // If timer is unref'd (or was - it's permanently removed from the list.) - if (timer._handle && timer instanceof Timeout) { - timer._handle.start(timer._repeat); - } else { - timer._idleTimeout = timer._repeat; + if (timer._idleTimeout === -1) + return; - const duration = TimerWrap.now() - start; - if (duration >= timer._repeat) { - // If callback duration >= timer._repeat, - // add 1 ms to avoid blocking eventloop - insert(timer, false, start + duration - timer._repeat + 1); - } else { - insert(timer, false, start); - } - } + timer._idleTimeout = timer._repeat; + insert(timer, timer[kRefed], start); } const clearTimeout = exports.clearTimeout = function clearTimeout(timer) { if (timer && timer._onTimeout) { timer._onTimeout = null; - if (timer instanceof Timeout) { - timer.close(); // for after === 0 - } else { - unenroll(timer); - } + unenroll(timer); } }; @@ -490,7 +517,7 @@ exports.setInterval = function setInterval(callback, repeat, arg1, arg2, arg3) { break; } - const timeout = new Timeout(callback, repeat, args, true, false); + const timeout = new Timeout(callback, repeat, args, true); active(timeout); return timeout; @@ -503,73 +530,25 @@ exports.clearInterval = function clearInterval(timer) { clearTimeout(timer); }; -function unrefdHandle(timer, now) { - try { - // Don't attempt to call the callback if it is not a function. - if (typeof timer._onTimeout === 'function') { - tryOnTimeout(timer, now); - } - } finally { - // Make sure we clean up if the callback is no longer a function - // even if the timer is an interval. - if (!timer._repeat || typeof timer._onTimeout !== 'function') { - timer.close(); - } - } - - return true; -} - Timeout.prototype.unref = function() { - if (this._handle) { - this._handle.unref(); - } else if (typeof this._onTimeout === 'function') { - const now = TimerWrap.now(); - if (!this._idleStart) this._idleStart = now; - var delay = this._idleStart + this._idleTimeout - now; - if (delay < 0) delay = 0; - - // Prevent running cb again when unref() is called during the same cb - if (this._called && !this._repeat) { - unenroll(this); - return; - } - - const handle = reuse(this); - if (handle !== null) { - handle._list = undefined; - } - - this._handle = handle || new TimerWrap(); - this._handle.owner = this; - this._handle.start(delay); - this._handle.unref(); + if (this[kRefed]) { + this[kRefed] = false; + decRefCount(); } return this; }; Timeout.prototype.ref = function() { - if (this._handle) - this._handle.ref(); + if (this[kRefed] === false) { + this[kRefed] = true; + incRefCount(); + } return this; }; Timeout.prototype.close = function() { - this._onTimeout = null; - if (this._handle) { - if (destroyHooksExist() && - typeof this[async_id_symbol] === 'number' && - !this._destroyed) { - emitDestroy(this[async_id_symbol]); - this._destroyed = true; - } - - this._idleTimeout = -1; - this._handle.close(); - } else { - unenroll(this); - } + clearTimeout(this); return this; }; diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index 88377a3e1b..1a5a22c25e 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -62,7 +62,6 @@ class TimerWrap : public HandleWrap { env->SetProtoMethod(constructor, "hasRef", HandleWrap::HasRef); env->SetProtoMethod(constructor, "start", Start); - env->SetProtoMethod(constructor, "stop", Stop); target->Set(timerString, constructor->GetFunction()); @@ -125,32 +124,17 @@ class TimerWrap : public HandleWrap { args.GetReturnValue().Set(err); } - static void Stop(const FunctionCallbackInfo<Value>& args) { - TimerWrap* wrap; - ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - - CHECK(HandleWrap::IsAlive(wrap)); - - int err = uv_timer_stop(&wrap->handle_); - args.GetReturnValue().Set(err); - } - static void OnTimeout(uv_timer_t* handle) { TimerWrap* wrap = static_cast<TimerWrap*>(handle->data); Environment* env = wrap->env(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); Local<Value> ret; - Local<Value> args[1]; + Local<Value> args[] = { env->GetNow() }; do { - args[0] = env->GetNow(); ret = wrap->MakeCallback(env->timers_callback_function(), 1, args) .ToLocalChecked(); - } while (ret->IsUndefined() && - !env->tick_info()->has_thrown() && - wrap->object()->Get(env->context(), - env->owner_string()).ToLocalChecked() - ->IsUndefined()); + } while (ret->IsUndefined() && !env->tick_info()->has_thrown()); } static void Now(const FunctionCallbackInfo<Value>& args) { diff --git a/test/async-hooks/test-timerwrap.setInterval.js b/test/async-hooks/test-timerwrap.setInterval.js index 058f2c9a9d..eab19be1df 100644 --- a/test/async-hooks/test-timerwrap.setInterval.js +++ b/test/async-hooks/test-timerwrap.setInterval.js @@ -51,6 +51,6 @@ function onexit() { hooks.disable(); hooks.sanityCheck('TIMERWRAP'); - checkInvocations(t, { init: 1, before: 3, after: 3, destroy: 1 }, + checkInvocations(t, { init: 1, before: 3, after: 3 }, 't: when process exits'); } diff --git a/test/async-hooks/test-timerwrap.setTimeout.js b/test/async-hooks/test-timerwrap.setTimeout.js index 9c873125d0..fcaf6bfc0a 100644 --- a/test/async-hooks/test-timerwrap.setTimeout.js +++ b/test/async-hooks/test-timerwrap.setTimeout.js @@ -23,7 +23,6 @@ checkInvocations(t1, { init: 1 }, 't1: when first timer installed'); function ontimeout() { checkInvocations(t1, { init: 1, before: 1 }, 't1: when first timer fired'); - // install second timeout with same TIMEOUT to see timer wrap being reused setTimeout(onsecondTimeout, TIMEOUT); const as = hooks.activitiesOfTypes('TIMERWRAP'); assert.strictEqual(as.length, 1); @@ -31,35 +30,21 @@ function ontimeout() { 't1: when second timer installed'); } -// even though we install 3 timers we only have two timerwrap resources created -// as one is reused for the two timers with the same timeout -let t2; - function onsecondTimeout() { - let as = hooks.activitiesOfTypes('TIMERWRAP'); + const as = hooks.activitiesOfTypes('TIMERWRAP'); assert.strictEqual(as.length, 1); checkInvocations(t1, { init: 1, before: 2, after: 1 }, 't1: when second timer fired'); // install third timeout with different TIMEOUT setTimeout(onthirdTimeout, TIMEOUT + 1); - as = hooks.activitiesOfTypes('TIMERWRAP'); - assert.strictEqual(as.length, 2); - t2 = as[1]; - assert.strictEqual(t2.type, 'TIMERWRAP'); - assert.strictEqual(typeof t2.uid, 'number'); - assert.strictEqual(typeof t2.triggerAsyncId, 'number'); checkInvocations(t1, { init: 1, before: 2, after: 1 }, 't1: when third timer installed'); - checkInvocations(t2, { init: 1 }, - 't2: when third timer installed'); } function onthirdTimeout() { - checkInvocations(t1, { init: 1, before: 2, after: 2, destroy: 1 }, + checkInvocations(t1, { init: 1, before: 3, after: 2 }, 't1: when third timer fired'); - checkInvocations(t2, { init: 1, before: 1 }, - 't2: when third timer fired'); tick(2); } @@ -69,8 +54,6 @@ function onexit() { hooks.disable(); hooks.sanityCheck('TIMERWRAP'); - checkInvocations(t1, { init: 1, before: 2, after: 2, destroy: 1 }, + checkInvocations(t1, { init: 1, before: 3, after: 3 }, 't1: when process exits'); - checkInvocations(t2, { init: 1, before: 1, after: 1, destroy: 1 }, - 't2: when process exits'); } diff --git a/test/parallel/test-handle-wrap-isrefed.js b/test/parallel/test-handle-wrap-isrefed.js index e1301b57f6..0b6afe83df 100644 --- a/test/parallel/test-handle-wrap-isrefed.js +++ b/test/parallel/test-handle-wrap-isrefed.js @@ -112,19 +112,23 @@ const dgram = require('dgram'); // timers { + const { Timer } = process.binding('timer_wrap'); + strictEqual(process._getActiveHandles().filter( + (handle) => (handle instanceof Timer)).length, 0); const timer = setTimeout(() => {}, 500); - timer.unref(); - strictEqual(Object.getPrototypeOf(timer._handle).hasOwnProperty('hasRef'), + const handles = process._getActiveHandles().filter( + (handle) => (handle instanceof Timer)); + strictEqual(handles.length, 1); + const handle = handles[0]; + strictEqual(Object.getPrototypeOf(handle).hasOwnProperty('hasRef'), true, 'timer_wrap: hasRef() missing'); - strictEqual(timer._handle.hasRef(), + strictEqual(handle.hasRef(), true); + timer.unref(); + strictEqual(handle.hasRef(), false, 'timer_wrap: unref() ineffective'); timer.ref(); - strictEqual(timer._handle.hasRef(), + strictEqual(handle.hasRef(), true, 'timer_wrap: ref() ineffective'); - timer._handle.close(common.mustCall(() => - strictEqual(timer._handle.hasRef(), - false, 'timer_wrap: not unrefed on close'))); } - // see also test/pseudo-tty/test-handle-wrap-isrefed-tty.js diff --git a/test/parallel/test-timers-next-tick.js b/test/parallel/test-timers-next-tick.js new file mode 100644 index 0000000000..1db1d18c3a --- /dev/null +++ b/test/parallel/test-timers-next-tick.js @@ -0,0 +1,15 @@ +'use strict'; + +const common = require('../common'); + +// This test documents an internal implementation detail of the Timers: +// since timers of different durations are stored in separate lists, +// a nextTick queue will clear after each list of timers. While this +// behaviour is not documented it could be relied on by Node's users. + +setTimeout(common.mustCall(() => { + process.nextTick(() => { clearTimeout(t2); }); +}), 1); +const t2 = setTimeout(common.mustNotCall(), 2); + +common.busyLoop(5); diff --git a/test/parallel/test-timers-same-timeout-wrong-list-deleted.js b/test/parallel/test-timers-same-timeout-wrong-list-deleted.js index c66ba0a57e..3e04cb6c4c 100644 --- a/test/parallel/test-timers-same-timeout-wrong-list-deleted.js +++ b/test/parallel/test-timers-same-timeout-wrong-list-deleted.js @@ -12,8 +12,6 @@ */ const common = require('../common'); -const assert = require('assert'); -const Timer = process.binding('timer_wrap').Timer; const TIMEOUT = common.platformTimeout(100); @@ -30,41 +28,9 @@ const handle1 = setTimeout(common.mustCall(function() { // erroneously deleted. If we are able to cancel the timer successfully, // the bug is fixed. clearTimeout(handle2); - - setImmediate(common.mustCall(function() { - setImmediate(common.mustCall(function() { - const activeTimers = getActiveTimers(); - - // Make sure our clearTimeout succeeded. One timer finished and - // the other was canceled, so none should be active. - assert.strictEqual(activeTimers.length, 0, 'Timers remain.'); - })); - })); }), 1); - // Make sure our timers got added to the list. - const activeTimers = getActiveTimers(); - const shortTimer = activeTimers.find(function(handle) { - return handle._list.msecs === 1; - }); - const longTimers = activeTimers.filter(function(handle) { - return handle._list.msecs === TIMEOUT; - }); - - // Make sure our clearTimeout succeeded. One timer finished and - // the other was canceled, so none should be active. - assert.strictEqual(activeTimers.length, 3, - 'There should be 3 timers in the list.'); - assert(shortTimer instanceof Timer, 'The shorter timer is not in the list.'); - assert.strictEqual(longTimers.length, 2, - 'Both longer timers should be in the list.'); - // When this callback completes, `listOnTimeout` should now look at the // correct list and refrain from removing the new TIMEOUT list which // contains the reference to the newer timer. }), TIMEOUT); - -function getActiveTimers() { - const activeHandles = process._getActiveHandles(); - return activeHandles.filter((handle) => handle instanceof Timer); -} diff --git a/test/parallel/test-timers-timeout-to-interval.js b/test/parallel/test-timers-timeout-to-interval.js index 6952f2231a..6b83e2e09e 100644 --- a/test/parallel/test-timers-timeout-to-interval.js +++ b/test/parallel/test-timers-timeout-to-interval.js @@ -8,5 +8,5 @@ const t = setTimeout(common.mustCall(() => { if (t._repeat) { clearInterval(t); } - t._repeat = true; + t._repeat = 1; }, 2), 1); diff --git a/test/parallel/test-timers-unref-leak.js b/test/parallel/test-timers-unref-leak.js deleted file mode 100644 index afecf7f15c..0000000000 --- a/test/parallel/test-timers-unref-leak.js +++ /dev/null @@ -1,14 +0,0 @@ -'use strict'; -const common = require('../common'); - -const timeout = setTimeout(common.mustCall(), 10); -timeout.unref(); - -// Wrap `close` method to check if the handle was closed -const close = timeout._handle.close; -timeout._handle.close = common.mustCall(function() { - return close.apply(this, arguments); -}); - -// Just to keep process alive and let previous timer's handle die -setTimeout(() => {}, 50); diff --git a/test/parallel/test-timers-unref-reuse-no-exposed-list.js b/test/parallel/test-timers-unref-reuse-no-exposed-list.js deleted file mode 100644 index 33b2da2f9e..0000000000 --- a/test/parallel/test-timers-unref-reuse-no-exposed-list.js +++ /dev/null @@ -1,14 +0,0 @@ -'use strict'; - -require('../common'); -const assert = require('assert'); - -const timer1 = setTimeout(() => {}, 1).unref(); -assert.strictEqual(timer1._handle._list, undefined, - 'timer1._handle._list should be undefined'); - -// Check that everything works even if the handle was not re-used. -setTimeout(() => {}, 1); -const timer2 = setTimeout(() => {}, 1).unref(); -assert.strictEqual(timer2._handle._list, undefined, - 'timer2._handle._list should be undefined'); |