timers: truncate decimal values

Reverts some timers behavior back to as it was before 2930bd1 That commit introduced an unintended change which allowed non-integer timeouts to actually exist since the value is no longer converted to an integer via a TimeWrap handle directly. Even with the fix in e9de435 non-integer timeouts are still indeterministic, because libuv does not support them. This fixes the issue by emulating the old behavior: truncate the `_idleTimeout` before using it. See comments in #24214 for more background on this.
nodejs · Fishrock123 · Dec 4, 2018 · Jan 28, 2019 · 10e70b6c9faa5fff5eb96b45ec403b774217e424
commit 10e70b6c9faa5fff5eb96b45ec403b774217e424
diff --git a/doc/api/timers.md b/doc/api/timers.md
@@ -186,7 +186,7 @@ added: v0.0.1
 Schedules repeated execution of `callback` every `delay` milliseconds.
 
 When `delay` is larger than `2147483647` or less than `1`, the `delay` will be
-set to `1`.
+set to `1`. Non-integer delays are truncated to an integer.
 
 If `callback` is not a function, a [`TypeError`][] will be thrown.
 
@@ -209,7 +209,7 @@ nor of their ordering. The callback will be called as close as possible to the
 time specified.
 
 When `delay` is larger than `2147483647` or less than `1`, the `delay`
-will be set to `1`.
+will be set to `1`. Non-integer delays are truncated to an integer.
 
 If `callback` is not a function, a [`TypeError`][] will be thrown.
 

diff --git a/lib/timers.js b/lib/timers.js
@@ -193,10 +193,13 @@ exports._unrefActive = function(item) {
 // Appends a timer onto the end of an existing timers list, or creates a new
 // list if one does not already exist for the specified timeout duration.
 function insert(item, refed, start) {
-  const msecs = item._idleTimeout;
+  let msecs = item._idleTimeout;
   if (msecs < 0 || msecs === undefined)
     return;
 
+  // Truncate so that accuracy of sub-milisecond timers is not assumed.
+  msecs = Math.trunc(msecs);
+
   item._idleStart = start;
 
   // Use an existing list if there is one, otherwise we need to make a new one.
@@ -378,7 +381,9 @@ function unenroll(item) {
   // clearTimeout that makes it clear that the list should not be deleted.
   // That function could then be used by http and other similar modules.
   if (item[kRefed]) {
-    const list = lists[item._idleTimeout];
+    // Compliment truncation during insert().
+    const msecs = Math.trunc(item._idleTimeout);
+    const list = lists[msecs];
     if (list !== undefined && L.isEmpty(list)) {
       debug('unenroll: list empty');
       queue.removeAt(list.priorityQueuePosition);

diff --git a/test/parallel/test-timers-non-integer-delay.js b/test/parallel/test-timers-non-integer-delay.js
@@ -21,6 +21,7 @@
 
 'use strict';
 const common = require('../common');
+const assert = require('assert');
 
 /*
  * This test makes sure that non-integer timer delays do not make the process
@@ -47,3 +48,37 @@ const interval = setInterval(common.mustCall(() => {
     process.exit(0);
   }
 }, N), TIMEOUT_DELAY);
+
+// Test non-integer delay ordering
+{
+  const ordering = [];
+
+  setTimeout(common.mustCall(() => {
+    ordering.push(1);
+  }), 1);
+
+  setTimeout(common.mustCall(() => {
+    ordering.push(2);
+  }), 1.8);
+
+  setTimeout(common.mustCall(() => {
+    ordering.push(3);
+  }), 1.1);
+
+  setTimeout(common.mustCall(() => {
+    ordering.push(4);
+  }), 1);
+
+  setTimeout(common.mustCall(() => {
+    const expected = [1, 2, 3, 4];
+
+    assert.deepStrictEqual(
+      ordering,
+      expected,
+      `Non-integer delay ordering should be ${expected}, but got ${ordering}`
+    );
+
+    // 2 should always be last of these delays due to ordering guarentees by
+    // the implementation.
+  }), 2);
+}