Skip to content

Commit

Permalink
timers: use consistent checks for canceled timers
Browse files Browse the repository at this point in the history
Previously not all codepaths set `timer._idleTimeout = -1` for canceled
or closed timers, and not all codepaths checked it either.

Unenroll uses this to say that a timer is indeed closed and it is the
closest thing there is to an authoritative source for this.

Refs: #9606
Fixes: #9561
PR-URL: #9685
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
Fishrock123 committed Nov 22, 2016
1 parent fa98eec commit 02c2bf7
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 2 deletions.
16 changes: 14 additions & 2 deletions lib/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,9 @@ function ontimeout(timer) {


function rearm(timer) {
// // 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);
Expand Down Expand Up @@ -463,9 +466,17 @@ function Timeout(after, callback, args) {


function unrefdHandle() {
ontimeout(this.owner);
if (!this.owner._repeat)
// Don't attempt to call the callback if it is not a function.
if (typeof this.owner._onTimeout === 'function') {
ontimeout(this.owner);
}

// 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();
}
}


Expand Down Expand Up @@ -505,6 +516,7 @@ Timeout.prototype.ref = function() {
Timeout.prototype.close = function() {
this._onTimeout = null;
if (this._handle) {
this._idleTimeout = -1;
this._handle[kOnTimeout] = null;
this._handle.close();
} else {
Expand Down
49 changes: 49 additions & 0 deletions test/parallel/test-timers-unenroll-unref-interval.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
'use strict';

const common = require('../common');
const timers = require('timers');

{
const interval = setInterval(common.mustCall(() => {
clearTimeout(interval);
}), 1).unref();
}

{
const interval = setInterval(common.mustCall(() => {
interval.close();
}), 1).unref();
}

{
const interval = setInterval(common.mustCall(() => {
timers.unenroll(interval);
}), 1).unref();
}

{
const interval = setInterval(common.mustCall(() => {
interval._idleTimeout = -1;
}), 1).unref();
}

{
const interval = setInterval(common.mustCall(() => {
interval._onTimeout = null;
}), 1).unref();
}

// Use timers' intrinsic behavior to keep this open
// exactly long enough for the problem to manifest.
//
// See https://github.com/nodejs/node/issues/9561
//
// Since this is added after it will always fire later
// than the previous timeouts, unrefed or not.
//
// Keep the event loop alive for one timeout and then
// another. Any problems will occur when the second
// should be called but before it is able to be.
setTimeout(common.mustCall(() => {
setTimeout(common.mustCall(() => {}), 1);
}), 1);

0 comments on commit 02c2bf7

Please sign in to comment.