Skip to content

Commit

Permalink
timers: emit warning if delay is negative or NaN
Browse files Browse the repository at this point in the history
Emit process warning once per process when delay is a negative number or
not a number, this will prevent unexpected behaviour caused by invalid
`delay` also keep the consistency of the behaviour and warning message
for `TIMEOUT_MAX` number As the negative number is invalid delay will be
set to 1.

PR-URL: #46678
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
  • Loading branch information
jakecastelli authored Jun 28, 2024
1 parent 764b13d commit f5ed338
Show file tree
Hide file tree
Showing 9 changed files with 272 additions and 5 deletions.
4 changes: 4 additions & 0 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,10 @@ A few of the warning types that are most common include:
* `'TimeoutOverflowWarning'` - Indicates that a numeric value that cannot fit
within a 32-bit signed integer has been provided to either the `setTimeout()`
or `setInterval()` functions.
* `'TimeoutNegativeWarning'` - Indicates that a negative number has provided to
either the `setTimeout()` or `setInterval()` functions.
* `'TimeoutNaNWarning'` - Indicates that a value which is not a number has
provided to either the `setTimeout()` or `setInterval()` functions.
* `'UnsupportedWarning'` - Indicates use of an unsupported option or feature
that will be ignored rather than treated as an error. One example is use of
the HTTP response status message when using the HTTP/2 compatibility API.
Expand Down
6 changes: 3 additions & 3 deletions doc/api/timers.md
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,8 @@ changes:

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`. Non-integer delays are truncated to an integer.
When `delay` is larger than `2147483647` or less than `1` or `NaN`, the `delay`
will be set to `1`. Non-integer delays are truncated to an integer.

If `callback` is not a function, a [`TypeError`][] will be thrown.

Expand Down Expand Up @@ -272,7 +272,7 @@ Node.js makes no guarantees about the exact timing of when callbacks will fire,
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`
When `delay` is larger than `2147483647` or less than `1` or `NaN`, the `delay`
will be set to `1`. Non-integer delays are truncated to an integer.

If `callback` is not a function, a [`TypeError`][] will be thrown.
Expand Down
22 changes: 21 additions & 1 deletion lib/internal/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ const {
MathMax,
MathTrunc,
NumberIsFinite,
NumberIsNaN,
NumberMIN_SAFE_INTEGER,
ReflectApply,
Symbol,
Expand Down Expand Up @@ -164,17 +165,36 @@ function initAsyncResource(resource, type) {
if (initHooksExist())
emitInit(asyncId, type, triggerAsyncId, resource);
}

let warnedNegativeNumber = false;
let warnedNotNumber = false;

class Timeout {
// Timer constructor function.
// The entire prototype is defined in lib/timers.js
constructor(callback, after, args, isRepeat, isRefed) {
after *= 1; // Coalesce to number or NaN
if (after === undefined) {
after = 1;
} else {
after *= 1; // Coalesce to number or NaN
}

if (!(after >= 1 && after <= TIMEOUT_MAX)) {
if (after > TIMEOUT_MAX) {
process.emitWarning(`${after} does not fit into` +
' a 32-bit signed integer.' +
'\nTimeout duration was set to 1.',
'TimeoutOverflowWarning');
} else if (after < 0 && !warnedNegativeNumber) {
warnedNegativeNumber = true;
process.emitWarning(`${after} is a negative number.` +
'\nTimeout duration was set to 1.',
'TimeoutNegativeWarning');
} else if (NumberIsNaN(after) && !warnedNotNumber) {
warnedNotNumber = true;
process.emitWarning(`${after} is not a number.` +
'\nTimeout duration was set to 1.',
'TimeoutNaNWarning');
}
after = 1; // Schedule on next tick, follows browser behavior
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@
Error: goodbye
at Hello (*uglify-throw-original.js:5:9)
at Immediate.<anonymous> (*uglify-throw-original.js:9:3)
at process.processImmediate (node:internal*timers:483:21)
at process.processImmediate (node:internal*timers:503:21)

Node.js *
39 changes: 39 additions & 0 deletions test/parallel/test-timers-nan-duration-emit-once-per-process.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';

const common = require('../common');
const assert = require('assert');

const NOT_A_NUMBER = NaN;

function timerNotCanceled() {
assert.fail('Timer should be canceled');
}

process.on(
'warning',
common.mustCall((warning) => {
if (warning.name === 'DeprecationWarning') return;

const lines = warning.message.split('\n');

assert.strictEqual(warning.name, 'TimeoutNaNWarning');
assert.strictEqual(lines[0], `${NOT_A_NUMBER} is not a number.`);
assert.strictEqual(lines.length, 2);
}, 1)
);

{
const timeout = setTimeout(timerNotCanceled, NOT_A_NUMBER);
clearTimeout(timeout);
}

{
const interval = setInterval(timerNotCanceled, NOT_A_NUMBER);
clearInterval(interval);
}

{
const timeout = setTimeout(timerNotCanceled, NOT_A_NUMBER);
timeout.refresh();
clearTimeout(timeout);
}
67 changes: 67 additions & 0 deletions test/parallel/test-timers-nan-duration-warning.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
'use strict';

require('../common');
const assert = require('assert');
const child_process = require('child_process');
const path = require('path');

const NOT_A_NUMBER = NaN;

function timerNotCanceled() {
assert.fail('Timer should be canceled');
}

const testCases = ['timeout', 'interval', 'refresh'];

function runTests() {
const args = process.argv.slice(2);

const testChoice = args[0];

if (!testChoice) {
const filePath = path.join(__filename);

testCases.forEach((testCase) => {
const { stdout } = child_process.spawnSync(
process.execPath,
[filePath, testCase],
{ encoding: 'utf8' }
);

const lines = stdout.split('\n');

if (lines[0] === 'DeprecationWarning') return;

assert.strictEqual(lines[0], 'TimeoutNaNWarning');
assert.strictEqual(lines[1], `${NOT_A_NUMBER} is not a number.`);
assert.strictEqual(lines[2], 'Timeout duration was set to 1.');
});
}

if (args[0] === testCases[0]) {
const timeout = setTimeout(timerNotCanceled, NOT_A_NUMBER);
clearTimeout(timeout);
}

if (args[0] === testCases[1]) {
const interval = setInterval(timerNotCanceled, NOT_A_NUMBER);
clearInterval(interval);
}

if (args[0] === testCases[2]) {
const timeout = setTimeout(timerNotCanceled, NOT_A_NUMBER);
timeout.refresh();
clearTimeout(timeout);
}

process.on(
'warning',

(warning) => {
console.log(warning.name);
console.log(warning.message);
}
);
}

runTests();
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';

const common = require('../common');
const assert = require('assert');

const NEGATIVE_NUMBER = -1;

function timerNotCanceled() {
assert.fail('Timer should be canceled');
}

process.on(
'warning',
common.mustCall((warning) => {
if (warning.name === 'DeprecationWarning') return;

const lines = warning.message.split('\n');

assert.strictEqual(warning.name, 'TimeoutNegativeWarning');
assert.strictEqual(lines[0], `${NEGATIVE_NUMBER} is a negative number.`);
assert.strictEqual(lines.length, 2);
}, 1)
);

{
const timeout = setTimeout(timerNotCanceled, NEGATIVE_NUMBER);
clearTimeout(timeout);
}

{
const interval = setInterval(timerNotCanceled, NEGATIVE_NUMBER);
clearInterval(interval);
}

{
const timeout = setTimeout(timerNotCanceled, NEGATIVE_NUMBER);
timeout.refresh();
clearTimeout(timeout);
}
67 changes: 67 additions & 0 deletions test/parallel/test-timers-negative-duration-warning.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
'use strict';

require('../common');
const assert = require('assert');
const child_process = require('child_process');
const path = require('path');

const NEGATIVE_NUMBER = -1;

function timerNotCanceled() {
assert.fail('Timer should be canceled');
}

const testCases = ['timeout', 'interval', 'refresh'];

function runTests() {
const args = process.argv.slice(2);

const testChoice = args[0];

if (!testChoice) {
const filePath = path.join(__filename);

testCases.forEach((testCase) => {
const { stdout } = child_process.spawnSync(
process.execPath,
[filePath, testCase],
{ encoding: 'utf8' }
);

const lines = stdout.split('\n');

if (lines[0] === 'DeprecationWarning') return;

assert.strictEqual(lines[0], 'TimeoutNegativeWarning');
assert.strictEqual(lines[1], `${NEGATIVE_NUMBER} is a negative number.`);
assert.strictEqual(lines[2], 'Timeout duration was set to 1.');
});
}

if (args[0] === testCases[0]) {
const timeout = setTimeout(timerNotCanceled, NEGATIVE_NUMBER);
clearTimeout(timeout);
}

if (args[0] === testCases[1]) {
const interval = setInterval(timerNotCanceled, NEGATIVE_NUMBER);
clearInterval(interval);
}

if (args[0] === testCases[2]) {
const timeout = setTimeout(timerNotCanceled, NEGATIVE_NUMBER);
timeout.refresh();
clearTimeout(timeout);
}

process.on(
'warning',

(warning) => {
console.log(warning.name);
console.log(warning.message);
}
);
}

runTests();
31 changes: 31 additions & 0 deletions test/parallel/test-timers-not-emit-duration-zero.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';

const common = require('../common');
const assert = require('assert');

function timerNotCanceled() {
assert.fail('Timer should be canceled');
}

process.on(
'warning',
common.mustNotCall(() => {
assert.fail('Timer should be canceled');
})
);

{
const timeout = setTimeout(timerNotCanceled, 0);
clearTimeout(timeout);
}

{
const interval = setInterval(timerNotCanceled, 0);
clearInterval(interval);
}

{
const timeout = setTimeout(timerNotCanceled, 0);
timeout.refresh();
clearTimeout(timeout);
}

0 comments on commit f5ed338

Please sign in to comment.