From 9c9277b819770be5faf038914734b7c0b87a69d5 Mon Sep 17 00:00:00 2001
From: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Date: Fri, 18 Nov 2016 15:47:15 -0500
Subject: [PATCH] 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>

 Conflicts:
	lib/timers.js
---
 lib/timers.js                                 | 19 +++++--
 .../test-timers-unenroll-unref-interval.js    | 49 +++++++++++++++++++
 2 files changed, 63 insertions(+), 5 deletions(-)
 create mode 100644 test/parallel/test-timers-unenroll-unref-interval.js

diff --git a/lib/timers.js b/lib/timers.js
index 2ff3381c857bbb..fdf38e8ef0a175 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -274,12 +274,12 @@ exports.setInterval = function(callback, repeat) {
   function wrapper() {
     timer._repeat();
 
-    // Timer might be closed - no point in restarting it
-    if (!timer._repeat)
+    // 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 (this._handle) {
+    if (this._handle && timer instanceof Timeout) {
       this._handle.start(repeat, 0);
     } else {
       timer._idleTimeout = repeat;
@@ -309,9 +309,17 @@ const Timeout = function(after) {
 
 
 function unrefdHandle() {
-  this.owner._onTimeout();
-  if (!this.owner._repeat)
+  // Don't attempt to call the callback if it is not a function.
+  if (typeof this.owner._onTimeout === 'function') {
+    this.owner._onTimeout();
+  }
+
+  // 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();
+  }
 }
 
 
@@ -351,6 +359,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 {
diff --git a/test/parallel/test-timers-unenroll-unref-interval.js b/test/parallel/test-timers-unenroll-unref-interval.js
new file mode 100644
index 00000000000000..2c8a6a385d2304
--- /dev/null
+++ b/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);