timers: minor _unrefActive fixes and improvements

This commit addresses most of the review comments in
#2540, which are kept in this
separate commit so as to better preserve the prior two patches as they
landed in 0.12.
This commit:
- Fixes a bug with unrefActive timers and disposed domains.
- Fixes a bug with unrolling an unrefActive timer from another.
- Adds a test for both above bugs.
- Improves check logic, making it stricter, simpler, or both.
- Optimizes nicer with a smaller, separate function for the try/catch.
Fixes: nodejs/node-convergence-archive#23
Ref: #268
PR-URL: #2540
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Author: Fishrock123

lib/timers.js

@@ -481,31 +481,36 @@ function _makeTimerTimeout(timer) {
 vardomain=timer.domain;
 varmsecs=timer._idleTimeout;
 
+L.remove(timer);
+
 // Timer has been unenrolled by another timer that fired at the same time,
 // so don't make it timeout.
-if(!msecs||msecs<0)
+if(msecs<=0)
 return;
 
 if(!timer._onTimeout)
 return;
 
-if(domain&&domain._disposed)
-return;
+if(domain){
+if(domain._disposed)
+return;
 
-try{
-varthrew=true;
+domain.enter();
+}
 
-if(domain)domain.enter();
+debug('unreftimer firing timeout');
+timer._called=true;
+_runOnTimeout(timer);
 
-debug('unreftimer firing timeout');
-L.remove(timer);
-timer._called=true;
-timer._onTimeout();
+if(domain)
+domain.exit();
+}
 
+function_runOnTimeout(timer){
+varthrew=true;
+try{
+timer._onTimeout();
 threw=false;
-
-if(domain)
-domain.exit();
 }finally{
 if(threw)process.nextTick(unrefTimeout);
 }
@@ -519,7 +524,7 @@ function unrefTimeout() {
 vartimeSinceLastActive;
 varnextTimeoutTime;
 varnextTimeoutDuration;
-varminNextTimeoutTime;
+varminNextTimeoutTime=TIMEOUT_MAX;
 vartimersToTimeout=[];
 
 // The actual timer fired and has not yet been rearmed,
@@ -534,7 +539,7 @@ function unrefTimeout() {
 // and rearm the actual timer if the next timeout to expire
 // will expire before the current actual timer.
 varcur=unrefList._idlePrev;
-while(cur!=unrefList){
+while(cur!==unrefList){
 timeSinceLastActive=now-cur._idleStart;
 
 if(timeSinceLastActive<cur._idleTimeout){
@@ -543,7 +548,7 @@ function unrefTimeout() {
 
 nextTimeoutDuration=cur._idleTimeout-timeSinceLastActive;
 nextTimeoutTime=now+nextTimeoutDuration;
-if(minNextTimeoutTime==null||
+if(minNextTimeoutTime===TIMEOUT_MAX||
 (nextTimeoutTime<minNextTimeoutTime)){
 // We found a timeout that will expire earlier,
 // store its next timeout time now so that we
@@ -569,7 +574,7 @@ function unrefTimeout() {
 
 // Rearm the actual timer with the timeout delay
 // of the earliest timeout found.
-if(minNextTimeoutTime!=null){
+if(minNextTimeoutTime!==TIMEOUT_MAX){
 unrefTimer.start(minNextTimeoutTime-now,0);
 unrefTimer.when=minNextTimeoutTime;
 debug('unrefTimer rescheduled');

test/parallel/test-timers-unref-active-unenrolled-disposed.js

@@ -0,0 +1,46 @@
+'use strict';
+
+// https://github.com/nodejs/node/pull/2540/files#r38231197
+
+constcommon=require('../common');
+consttimers=require('timers');
+constassert=require('assert');
+constdomain=require('domain');
+
+// Crazy stuff to keep the process open,
+// then close it when we are actually done.
+constTEST_DURATION=common.platformTimeout(100);
+constkeepOpen=setTimeout(function(){
+thrownewError('Test timed out. keepOpen was not canceled.');
+},TEST_DURATION);
+
+constendTest=makeTimer(2);
+
+constsomeTimer=makeTimer(1);
+someTimer.domain=domain.create();
+someTimer.domain.dispose();
+someTimer._onTimeout=function(){
+thrownewError('someTimer was not supposed to fire!');
+};
+
+endTest._onTimeout=common.mustCall(function(){
+assert.strictEqual(someTimer._idlePrev,null);
+assert.strictEqual(someTimer._idleNext,null);
+clearTimeout(keepOpen);
+});
+
+constcancelsTimer=makeTimer(1);
+cancelsTimer._onTimeout=common.mustCall(function(){
+someTimer._idleTimeout=0;
+});
+
+timers._unrefActive(cancelsTimer);
+timers._unrefActive(someTimer);
+timers._unrefActive(endTest);
+
+functionmakeTimer(msecs){
+consttimer={};
+timers.unenroll(timer);
+timers.enroll(timer,msecs);
+returntimer;
+}