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.

PR-URL: nodejs#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]>

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.

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.

lib/internal/timers.js

@@ -76,6 +76,7 @@ const {
   MathMax,
   MathTrunc,
   NumberIsFinite,
+  NumberIsNaN,
   NumberMIN_SAFE_INTEGER,
   ReflectApply,
   Symbol,
@@ -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
     }

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.<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 *

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

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();

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

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();

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