From 7c97dbf7299e316a5c3be13d73fb3eb8ca362bed Mon Sep 17 00:00:00 2001 From: jakecastelli <959672929@qq.com> Date: Thu, 16 Feb 2023 11:59:33 +1030 Subject: [PATCH 1/3] timers: emit warning if delay is negative or NaN 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. --- doc/api/process.md | 4 ++ doc/api/timers.md | 6 +- lib/internal/timers.js | 22 +++++- .../source_map_throw_set_immediate.snapshot | 2 +- ...mers-nan-duration-emit-once-per-process.js | 39 +++++++++++ .../test-timers-nan-duration-warning.js | 67 +++++++++++++++++++ ...-duration-warning-emit-once-per-process.js | 39 +++++++++++ .../test-timers-negative-duration-warning.js | 67 +++++++++++++++++++ .../test-timers-not-emit-duration-zero.js | 31 +++++++++ 9 files changed, 272 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-timers-nan-duration-emit-once-per-process.js create mode 100644 test/parallel/test-timers-nan-duration-warning.js create mode 100644 test/parallel/test-timers-negative-duration-warning-emit-once-per-process.js create mode 100644 test/parallel/test-timers-negative-duration-warning.js create mode 100644 test/parallel/test-timers-not-emit-duration-zero.js diff --git a/doc/api/process.md b/doc/api/process.md index b17739eeb5bb2f..0dcfed70f946ac 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -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. diff --git a/doc/api/timers.md b/doc/api/timers.md index 3789e9efd00906..fd459e2a007fb4 100644 --- a/doc/api/timers.md +++ b/doc/api/timers.md @@ -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. @@ -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. diff --git a/lib/internal/timers.js b/lib/internal/timers.js index 5c56cf106350c1..2c66d15a35ad34 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -75,6 +75,7 @@ const { MathMax, MathTrunc, + NumberIsNaN, NumberIsFinite, NumberMIN_SAFE_INTEGER, ReflectApply, @@ -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 } diff --git a/test/fixtures/source-map/output/source_map_throw_set_immediate.snapshot b/test/fixtures/source-map/output/source_map_throw_set_immediate.snapshot index d2d838ca35a3de..dbf15d20c250a3 100644 --- a/test/fixtures/source-map/output/source_map_throw_set_immediate.snapshot +++ b/test/fixtures/source-map/output/source_map_throw_set_immediate.snapshot @@ -6,6 +6,6 @@ Error: goodbye at Hello (*uglify-throw-original.js:5:9) at Immediate. (*uglify-throw-original.js:9:3) - at process.processImmediate (node:internal*timers:483:21) + at process.processImmediate (node:internal*timers:498:21) Node.js * diff --git a/test/parallel/test-timers-nan-duration-emit-once-per-process.js b/test/parallel/test-timers-nan-duration-emit-once-per-process.js new file mode 100644 index 00000000000000..4dd2eed5ace574 --- /dev/null +++ b/test/parallel/test-timers-nan-duration-emit-once-per-process.js @@ -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); +} diff --git a/test/parallel/test-timers-nan-duration-warning.js b/test/parallel/test-timers-nan-duration-warning.js new file mode 100644 index 00000000000000..f8bbf65a086f26 --- /dev/null +++ b/test/parallel/test-timers-nan-duration-warning.js @@ -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(); diff --git a/test/parallel/test-timers-negative-duration-warning-emit-once-per-process.js b/test/parallel/test-timers-negative-duration-warning-emit-once-per-process.js new file mode 100644 index 00000000000000..2cf3197789fcb8 --- /dev/null +++ b/test/parallel/test-timers-negative-duration-warning-emit-once-per-process.js @@ -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); +} diff --git a/test/parallel/test-timers-negative-duration-warning.js b/test/parallel/test-timers-negative-duration-warning.js new file mode 100644 index 00000000000000..4e236e1af9c6a2 --- /dev/null +++ b/test/parallel/test-timers-negative-duration-warning.js @@ -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(); diff --git a/test/parallel/test-timers-not-emit-duration-zero.js b/test/parallel/test-timers-not-emit-duration-zero.js new file mode 100644 index 00000000000000..c6a51c25b309f6 --- /dev/null +++ b/test/parallel/test-timers-not-emit-duration-zero.js @@ -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); +} From ff000c141346f475dfa060dcd140301d0f4c60db Mon Sep 17 00:00:00 2001 From: jakecastelli <959672929@qq.com> Date: Wed, 26 Jun 2024 11:52:18 +0930 Subject: [PATCH 2/3] fixup! --- .../source-map/output/source_map_throw_set_immediate.snapshot | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/source-map/output/source_map_throw_set_immediate.snapshot b/test/fixtures/source-map/output/source_map_throw_set_immediate.snapshot index dbf15d20c250a3..ee3294a2871f6e 100644 --- a/test/fixtures/source-map/output/source_map_throw_set_immediate.snapshot +++ b/test/fixtures/source-map/output/source_map_throw_set_immediate.snapshot @@ -6,6 +6,6 @@ Error: goodbye at Hello (*uglify-throw-original.js:5:9) at Immediate. (*uglify-throw-original.js:9:3) - at process.processImmediate (node:internal*timers:498:21) + at process.processImmediate (node:internal*timers:503:21) Node.js * From 970b44b07befcc6f8b8c67fe6cf0a1a7297a713c Mon Sep 17 00:00:00 2001 From: jakecastelli <959672929@qq.com> Date: Wed, 26 Jun 2024 14:58:22 +0930 Subject: [PATCH 3/3] fixup! lint --- lib/internal/timers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/timers.js b/lib/internal/timers.js index 2c66d15a35ad34..01800556611679 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -75,8 +75,8 @@ const { MathMax, MathTrunc, - NumberIsNaN, NumberIsFinite, + NumberIsNaN, NumberMIN_SAFE_INTEGER, ReflectApply, Symbol,