Browse Source

timers: use consistent checks for canceled timers

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: https://github.com/nodejs/node/pull/9606
Fixes: https://github.com/nodejs/node/issues/9561
PR-URL: https://github.com/nodejs/node/pull/9685
Reviewed-By: Rich Trott <rtrott@gmail.com>
v6
Jeremiah Senkpiel 8 years ago
parent
commit
3f1e38c847
  1. 16
      lib/timers.js
  2. 49
      test/parallel/test-timers-unenroll-unref-interval.js

16
lib/timers.js

@ -384,6 +384,9 @@ function ontimeout(timer) {
function rearm(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 is unref'd (or was - it's permanently removed from the list.)
if (timer._handle && timer instanceof Timeout) { if (timer._handle && timer instanceof Timeout) {
timer._handle.start(timer._repeat); timer._handle.start(timer._repeat);
@ -463,9 +466,17 @@ function Timeout(after, callback, args) {
function unrefdHandle() { function unrefdHandle() {
ontimeout(this.owner); // Don't attempt to call the callback if it is not a function.
if (!this.owner._repeat) 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(); this.owner.close();
}
} }
@ -505,6 +516,7 @@ Timeout.prototype.ref = function() {
Timeout.prototype.close = function() { Timeout.prototype.close = function() {
this._onTimeout = null; this._onTimeout = null;
if (this._handle) { if (this._handle) {
this._idleTimeout = -1;
this._handle[kOnTimeout] = null; this._handle[kOnTimeout] = null;
this._handle.close(); this._handle.close();
} else { } else {

49
test/parallel/test-timers-unenroll-unref-interval.js

@ -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);
Loading…
Cancel
Save