Permalink
Browse files

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.

PR-URL: #24819
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information...
Fishrock123 authored and targos committed Dec 4, 2018
1 parent edc9ceb commit d0705bd24b4e9e897a482dc204bad897181781e7
Showing with 44 additions and 4 deletions.
  1. +2 −2 doc/api/timers.md
  2. +7 −2 lib/timers.js
  3. +35 −0 test/parallel/test-timers-non-integer-delay.js
@@ -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.

@@ -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);
@@ -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);
}

0 comments on commit d0705bd

Please sign in to comment.