Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timers: emit process warning if delay is negative or NaN #46678

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.` +
jakecastelli marked this conversation as resolved.
Show resolved Hide resolved
'\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);
}
Loading