timers: fix not to close reused timer handle

The timer handle is reused when it is unrefed in order to avoid new
callback in beforeExit(nodejs#3407). If it is unrefed within a setInterval
callback, the reused timer handle is closed so that setInterval no
longer keep working. This fix does not close the handle in case of
setInterval.

PR-URL: nodejs/node#11646
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

lib/timers.js

@@ -222,7 +222,6 @@ function listOnTimeout() {
   // As such, we can remove the list and clean up the TimerWrap C++ handle.
   debug('%d list empty', msecs);
   assert(L.isEmpty(list));
-  this.close();
 
   // Either refedLists[msecs] or unrefedLists[msecs] may have been removed and
   // recreated since the reference to `list` was created. Make sure they're
@@ -232,6 +231,13 @@ function listOnTimeout() {
   } else if (list === refedLists[msecs]) {
     delete refedLists[msecs];
   }
+
+  // Do not close the underlying handle if its ownership has changed
+  // (e.g it was unrefed in its callback).
+  if (this.owner)
+    return;
+
+  this.close();
 }
 
 

test/parallel/test-timers-unrefed-in-callback.js

@@ -0,0 +1,61 @@
+'use strict';
+// Checks that setInterval timers keep running even when they're
+// unrefed within their callback.
+
+require('../common');
+const assert = require('assert');
+const net = require('net');
+
+let counter1 = 0;
+let counter2 = 0;
+
+// Test1 checks that clearInterval works as expected for a timer
+// unrefed within its callback: it removes the timer and its callback
+// is not called anymore. Note that the only reason why this test is
+// robust is that:
+// 1. the repeated timer it creates has a delay of 1ms
+// 2. when this test is completed, another test starts that creates a
+//    new repeated timer with the same delay (1ms)
+// 3. because of the way timers are implemented in libuv, if two
+//    repeated timers A and B are created in that order with the same
+//    delay, it is guaranteed that the first occurrence of timer A
+//    will fire before the first occurrence of timer B
+// 4. as a result, when the timer created by Test2 fired 11 times, if
+//    the timer created by Test1 hadn't been removed by clearInterval,
+//    it would have fired 11 more times, and the assertion in the
+//    process'exit event handler would fail.
+function Test1() {
+  // server only for maintaining event loop
+  const server = net.createServer().listen(0);
+
+  const timer1 = setInterval(() => {
+    timer1.unref();
+    if (counter1++ === 3) {
+      clearInterval(timer1);
+      server.close(() => {
+        Test2();
+      });
+    }
+  }, 1);
+}
+
+
+// Test2 checks setInterval continues even if it is unrefed within
+// timer callback. counter2 continues to be incremented more than 11
+// until server close completed.
+function Test2() {
+  // server only for maintaining event loop
+  const server = net.createServer().listen(0);
+
+  const timer2 = setInterval(() => {
+    timer2.unref();
+    if (counter2++ === 3)
+      server.close();
+  }, 1);
+}
+
+process.on('exit', () => {
+  assert.strictEqual(counter1, 4);
+});
+
+Test1();