timers: fix processing of nested same delay timers

Whenever a timer with a specific timeout value creates a new timer with
the same timeout, the newly added timer might be processed immediately
in the same tick of the event loop instead of during the next tick of
the event loop at the earliest.

Fixes #25607

Author: whitlockjc

lib/timers.js

@@ -87,13 +87,35 @@ function listOnTimeout() {
 
   var first;
   while (first = L.peek(list)) {
-    // If the previous iteration caused a timer to be added,
-    // update the value of "now" so that timing computations are
-    // done correctly. See test/simple/test-timers-blocking-callback.js
-    // for more information.
-    if (now < first._monotonicStartTime) {
-      now = Timer.now();
-      debug('now: %d', now);
+    // This handles the case of a timer that was created within a timers
+    // callback with the same timeout value. For instance, when processing the
+    // timer that would call `bar` in such code:
+    //
+    //   setTimeout(function foo() { setTimeout(function bar() {}, 0) }, 0);
+    //
+    // or
+    //
+    //   setTimeout(function foo() { setTimeout(function bar() {}, 500) }, 500);
+    //
+    // We want to make sure that newly added timer fires in the next turn of the
+    // event loop at the earliest. So even if it's already expired now,
+    // reschedule it to fire later.
+    //
+    // At that point, it's not necessary to process any other timer in that
+    // list, because any remaining timer has been added within a callback of a
+    // timer that has already been processed, and thus needs to be processed at
+    // the earliest not in the current tick, but when the rescheduled timer will
+    // expire.
+    //
+    // See: https://github.com/joyent/node/issues/25607
+    if (now <= first._monotonicStartTime) {
+      var timeRemaining = msecs - (Timer.now() - first._monotonicStartTime);
+      if (timeRemaining < 0) {
+        timeRemaining = 0;
+      }
+      debug(msecs + ' list wait because timer was added from another timer');
+      list.start(timeRemaining, 0);
+      return;
     }
 
     var diff = now - first._monotonicStartTime;

test/common.js

@@ -22,6 +22,7 @@
 var path = require('path');
 var fs = require('fs');
 var assert = require('assert');
+var Timer = process.binding('timer_wrap').Timer;
 
 exports.testDir = path.dirname(__filename);
 exports.fixturesDir = path.join(exports.testDir, 'fixtures');
@@ -220,9 +221,9 @@ exports.hasMultiLocalhost = function hasMultiLocalhost() {
 };
 
 exports.busyLoop = function busyLoop(time) {
-  var startTime = new Date().getTime();
+  var startTime = Timer.now();
   var stopTime =  startTime + time;
-  while (new Date().getTime() < stopTime) {
+  while (Timer.now() < stopTime) {
     ;
   }
 };

test/simple/test-timers-nested.js

@@ -0,0 +1,58 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+
+// Make sure we test 0ms timers, since they would had always wanted to run on
+// the current tick, and greater than 0ms timers, for scenarios where the
+// outer timer takes longer to complete than the delay of the nested timer.
+// Since the process of recreating this is identical regardless of the timer
+// delay, these scenarios are in one test.
+var scenarios = [0, 100];
+
+scenarios.forEach(function (delay) {
+  var nestedCalled = false;
+
+  setTimeout(function A() {
+    // Create the nested timer with the same delay as the outer timer so that it
+    // gets added to the current list of timers being processed by
+    // listOnTimeout.
+    setTimeout(function B() {
+      nestedCalled = true;
+    }, delay);
+
+    // Busy loop for the same timeout used for the nested timer to ensure that
+    // we are in fact expiring the nested timer.
+    common.busyLoop(delay);
+
+    // The purpose of running this assert in nextTick is to make sure it runs
+    // after A but before the next iteration of the libuv event loop.
+    process.nextTick(function() {
+      assert.ok(!nestedCalled);
+    });
+
+    // Ensure that the nested callback is indeed called prior to process exit.
+    process.on('exit', function onExit() {
+      assert.ok(nestedCalled);
+    });
+  }, delay);
+});