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: #9606Fixes: #9561 PR-URL: #9685 Reviewed-By: Rich Trott <[email protected]>

Author: Fishrock123

lib/timers.js

@@ -384,6 +384,9 @@ function ontimeout(timer){
 
 
 functionrearm(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&&timerinstanceofTimeout){
 timer._handle.start(timer._repeat);
@@ -463,9 +466,17 @@ function Timeout(after, callback, args){
 
 
 functionunrefdHandle(){
-ontimeout(this.owner);
-if(!this.owner._repeat)
+// Don't attempt to call the callback if it is not a function.
+if(typeofthis.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
+||typeofthis.owner._onTimeout!=='function'){
 this.owner.close();
+}
 }
 
 
@@ -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{

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

@@ -0,0 +1,49 @@
+'use strict';
+
+constcommon=require('../common');
+consttimers=require('timers');
+
+{
+constinterval=setInterval(common.mustCall(()=>{
+clearTimeout(interval);
+}),1).unref();
+}
+
+{
+constinterval=setInterval(common.mustCall(()=>{
+interval.close();
+}),1).unref();
+}
+
+{
+constinterval=setInterval(common.mustCall(()=>{
+timers.unenroll(interval);
+}),1).unref();
+}
+
+{
+constinterval=setInterval(common.mustCall(()=>{
+interval._idleTimeout=-1;
+}),1).unref();
+}
+
+{
+constinterval=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);