timers: fix cleanup of nested same-timeout timers

For nested timers with the same timeout, we can get into a situation where we have recreated a timer list immediately before we need to clean up an old timer list with the same key. Fix: make sure the list to be deleted is the same instance as the list whose reference was used to determine that a cleanup is necessary. If it's not the same instance, a new list with the same key has been created, and it should not be deleted. Fixes: #7722 PR-URL: #7827 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Julien Gilli <[email protected]>

Author: erinishimoticha

lib/timers.js

@@ -211,9 +211,13 @@ function listOnTimeout(){
 debug('%d list empty',msecs);
 assert(L.isEmpty(list));
 this.close();
-if(list._unrefed===true){
+
+// Either refedLists[msecs] or unrefedLists[msecs] may have been removed and
+// recreated since the reference to `list` was created. Make sure they're
+// the same instance of the list before destroying.
+if(list._unrefed===true&&list===unrefedLists[msecs]){
 deleteunrefedLists[msecs];
-}else{
+}elseif(list===refedLists[msecs]){
 deleterefedLists[msecs];
 }
 }

test/parallel/test-timers-same-timeout-wrong-list-deleted.js

@@ -0,0 +1,80 @@
+'use strict';
+
+/*
+ * This is a regression test for https://github.com/nodejs/node/issues/7722.
+ *
+ * When nested timers have the same timeout, calling clearTimeout on the
+ * older timer after it has fired causes the list the newer timer is in
+ * to be deleted. Since the newer timer was not cleared, it still blocks
+ * the event loop completing for the duration of its timeout, however, since
+ * no reference exists to it in its list, it cannot be canceled and its
+ * callback is not called when the timeout elapses.
+ */
+
+constcommon=require('../common');
+constassert=require('assert');
+constTimer=process.binding('timer_wrap').Timer;
+
+constTIMEOUT=common.platformTimeout(100);
+conststart=Timer.now();
+
+// This bug also prevents the erroneously dereferenced timer's callback
+// from being called, so we can't use it's execution or lack thereof
+// to assert that the bug is fixed.
+process.on('exit',function(){
+constend=Timer.now();
+assert.equal(end-start<TIMEOUT*2,true,
+'Elapsed time does not include second timer\'s timeout.');
+});
+
+consthandle1=setTimeout(common.mustCall(function(){
+// Cause the old TIMEOUT list to be deleted
+clearTimeout(handle1);
+
+// Cause a new list with the same key (TIMEOUT) to be created for this timer
+consthandle2=setTimeout(function(){
+common.fail('Inner callback is not called');
+},TIMEOUT);
+
+setTimeout(common.mustCall(function(){
+// Attempt to cancel the second timer. Fix for this bug will keep the
+// newer timer from being dereferenced by keeping its list from being
+// erroneously deleted. If we are able to cancel the timer successfully,
+// the bug is fixed.
+clearTimeout(handle2);
+setImmediate(common.mustCall(function(){
+setImmediate(common.mustCall(function(){
+constactiveHandles=process._getActiveHandles();
+constactiveTimers=activeHandles.filter(function(handle){
+returnhandleinstanceofTimer;
+});
+
+// Make sure our clearTimeout succeeded. One timer finished and
+// the other was canceled, so none should be active.
+assert.equal(activeTimers.length,0,'No Timers remain.');
+}));
+}));
+}),10);
+
+// Make sure our timers got added to the list.
+constactiveHandles=process._getActiveHandles();
+constactiveTimers=activeHandles.filter(function(handle){
+returnhandleinstanceofTimer;
+});
+constshortTimer=activeTimers.find(function(handle){
+returnhandle._list.msecs===10;
+});
+constlongTimers=activeTimers.filter(function(handle){
+returnhandle._list.msecs===TIMEOUT;
+});
+
+// Make sure our clearTimeout succeeded. One timer finished and
+// the other was canceled, so none should be active.
+assert.equal(activeTimers.length,3,'There are 3 timers in the list.');
+assert(shortTimerinstanceofTimer,'The shorter timer is in the list.');
+assert.equal(longTimers.length,2,'Both longer timers are in the list.');
+
+// When this callback completes, `listOnTimeout` should now look at the
+// correct list and refrain from removing the new TIMEOUT list which
+// contains the reference to the newer timer.
+}),TIMEOUT);