timers: fix a bug in error handling

When a timeout within a list of timeouts (that consists of only that specific timeout) throws during its execution, it's possible for the TimerWrap handle to become shared between both that list and an unref'd timeout created in the future. This fixes the bug by extending error handling in timeout execution to check for whether the current list is empty and if so do cleanup on it. PR-URL: #20497Fixes: #19970 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>

Author: apapirovski

lib/timers.js

@@ -217,6 +217,17 @@ function TimersList(msecs, unrefed){
 this.nextTick=false;
 }
 
+functiondeleteTimersList(list,msecs){
+// 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];
+}elseif(list===refedLists[msecs]){
+deleterefedLists[msecs];
+}
+}
+
 functionlistOnTimeout(){
 varlist=this._list;
 varmsecs=list.msecs;
@@ -288,14 +299,7 @@ function listOnTimeout(){
 debug('%d list empty',msecs);
 assert(L.isEmpty(list));
 
-// 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];
-}elseif(list===refedLists[msecs]){
-deleterefedLists[msecs];
-}
+deleteTimersList(list,msecs);
 
 // Do not close the underlying handle if its ownership has changed
 // (e.g it was unrefed in its callback).
@@ -329,24 +333,34 @@ function tryOnTimeout(timer, list){
 }
 }
 
-if(!threw)return;
+if(threw){
+const{ msecs }=list;
+
+if(L.isEmpty(list)){
+deleteTimersList(list,msecs);
 
-// Postpone all later list events to next tick. We need to do this
-// so that the events are called in the order they were created.
-constlists=list._unrefed===true ? unrefedLists : refedLists;
-for(varkeyinlists){
-if(key>list.msecs){
-lists[key].nextTick=true;
+if(!list._timer.owner)
+list._timer.close();
+}else{
+// Postpone all later list events to next tick. We need to do this
+// so that the events are called in the order they were created.
+constlists=list._unrefed===true ? unrefedLists : refedLists;
+for(varkeyinlists){
+if(key>msecs){
+lists[key].nextTick=true;
+}
+}
+
+// We need to continue processing after domain error handling
+// is complete, but not by using whatever domain was left over
+// when the timeout threw its exception.
+constdomain=process.domain;
+process.domain=null;
+// If we threw, we need to process the rest of the list in nextTick.
+process.nextTick(listOnTimeoutNT,list);
+process.domain=domain;
 }
 }
-// We need to continue processing after domain error handling
-// is complete, but not by using whatever domain was left over
-// when the timeout threw its exception.
-constdomain=process.domain;
-process.domain=null;
-// If we threw, we need to process the rest of the list in nextTick.
-process.nextTick(listOnTimeoutNT,list);
-process.domain=domain;
 }
 }
 

test/parallel/test-timers-throw-reschedule.js

@@ -0,0 +1,27 @@
+'use strict';
+
+constcommon=require('../common');
+constassert=require('assert');
+
+// This test checks that throwing inside a setTimeout where that Timeout
+// instance is the only one within its list of timeouts, doesn't cause
+// an issue with an unref timeout scheduled in the error handler.
+// Refs: https://github.com/nodejs/node/issues/19970
+
+consttimeout=common.platformTimeout(50);
+
+consttimer=setTimeout(common.mustNotCall(),2**31-1);
+
+process.once('uncaughtException',common.mustCall((err)=>{
+assert.strictEqual(err.message,'setTimeout Error');
+
+constnow=Date.now();
+setTimeout(common.mustCall(()=>{
+assert(Date.now()-now>=timeout);
+clearTimeout(timer);
+}),timeout).unref();
+}));
+
+setTimeout(()=>{
+thrownewError('setTimeout Error');
+},timeout);