timers: emit process warning if delay is negative or NaN

[jakecastelli]

Partially addressed #46596 to keep the consistency of the warning message for TIMEOUT_MAX number as the negative number, NaN will be set to 1 as well.

[jakecastelli]

I personally would prefer to add TimeoutInvalidDelayWarning for both negative number or NaN but keen to hear others thought on this.

[debadree25]

Hi I think this would require a test too that the warning is emitted as per the conditions

[jakecastelli]

Hi I think this would require a test too that the warning is emitted as per the conditions

Hi Debadree, I was wondering if we should add a new warning name(s) to address negative number and NaN or just leave it empty, for sure, I will add tests.

[debadree25]

we should add a new warning name(s)

Didn't get you here 😅 new name for the warning?

[jakecastelli]

we should add a new warning name(s)

Didn't get you here 😅 new name for the warning?

Sorry, I was trying to refer to here, when the delay is greater than 2**31 - 1 the process would emit TimeoutOverflowWarning, you could also find it in the process.md here, I was wondering if we should add warning for the negative number and NaN

[debadree25]

Ah, I think having a name would be a good thing, would follow the pattern and make it easier to test maybe something like TimeoutNaNWarning or TimeoutNegativeWarning should be good

[jakecastelli]

Sorry, I didn't know that when you request a new review would remove the existing review request 😅

[jakecastelli]

I forgot to rebase @debadree25 sorry, would you mind running the workflows again 🙏

[debadree25]

The code looks good! will wait for a few more reviews!

[panva]

Yup.

[aduh95]

Sorry for missing your previous pings!

What's being deprecated is passing NaN or a negative number to setTimeout/setInterval.

Before this PR:

$ node -e 'setInterval(console.log,-1).close();setInterval(console.log,NaN).close();setTimeout(console.log,-1);setTimeout(console.log,NaN)'

With this PR:

$ node -e 'setInterval(console.log,-1).close();setInterval(console.log,NaN).close();setTimeout(console.log,-1);setTimeout(console.log,NaN)'
(node:43520) TimeoutNegativeWarning: -1 is a negative number.
Timeout duration was set to 1.
(Use `node --trace-warnings ...` to show where the warning was created)
(node:43520) TimeoutNaNWarning: NaN is not a number.
Timeout duration was set to 1.

As you can see, code that was working before is now emitting a warning.

node/doc/contributing/collaborator-guide.md

Lines 356 to 363 in 956521c

	Existing stable public APIs that change in a backward-incompatible way must
	undergo deprecation. The exceptions to this rule are:
	* Adding or removing errors thrown or reported by a public API.
	* Changing error messages for errors without error code.
	* Altering the timing and non-internal side effects of the public API.
	* Changes to errors thrown by dependencies of Node.js, such as V8.
	* One-time exceptions granted by the TSC.

node/doc/contributing/collaborator-guide.md

Lines 475 to 497 in 956521c

	All deprecations receive a unique and immutable identifier. Documentation,
	warnings, and errors use the identifier when referring to the deprecation. The
	documentation for the deprecation identifier must always remain in the API
	documentation. This is true even if the deprecation is no longer in use (for
	example, due to removal of an End-of-Life deprecated API).
	<a id="deprecation-cycle"></a>
	A _deprecation cycle_ is a major release during which an API has been in one of
	the three Deprecation levels. Documentation-Only Deprecations can land in a
	minor release. They can not change to a Runtime Deprecation until the next major
	release.
	No API can change to End-of-Life without going through a Runtime Deprecation
	cycle. There is no rule that deprecated code must progress to End-of-Life.
	Documentation-Only and Runtime Deprecations can remain in place for an unlimited
	duration.
	Communicate pending deprecations and associated mitigations with the ecosystem
	as soon as possible. If possible, do it before the pull request adding the
	deprecation lands on the `main` branch.
	Use the `notable-change` label on pull requests that add or change the
	deprecation level of an API.

[jakecastelli]

Much thanks! Now I understood.

The policy have changes, runtime warnings are now allowed on non-deprecated APIs

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/59989/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/46678
✔  Done loading data for nodejs/node/pull/46678
----------------------------------- PR info ------------------------------------
Title      timers: emit process warning if delay is negative or NaN (#46678)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     jakecastelli:improve-timer-warning-message -> nodejs:main
Labels     timers, semver-major, author ready, needs-ci, review wanted
Commits    3
 - timers: emit warning if delay is negative or NaN
 - fixup!
 - fixup! lint
Committers 1
 - jakecastelli <959672929@qq.com>
PR-URL: https://github.com/nodejs/node/pull/46678
Reviewed-By: Debadree Chatterjee 
Reviewed-By: Ulises Gascón 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Chengzhong Wu 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46678
Reviewed-By: Debadree Chatterjee 
Reviewed-By: Ulises Gascón 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Chengzhong Wu 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 16 Feb 2023 01:34:23 GMT
   ✔  Approvals: 4
   ✔  - Debadree Chatterjee (@debadree25): https://github.com/nodejs/node/pull/46678#pullrequestreview-1304735554
   ✔  - Ulises Gascón (@UlisesGascon): https://github.com/nodejs/node/pull/46678#pullrequestreview-1314970311
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/46678#pullrequestreview-2145179529
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/46678#pullrequestreview-2145602094
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-06-27T16:48:46Z: https://ci.nodejs.org/job/node-test-pull-request/59989/
- Querying data for job/node-test-pull-request/59989/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 46678
From https://github.com/nodejs/node
 * branch                  refs/pull/46678/merge -> FETCH_HEAD
✔  Fetched commits as 764b13d75c0a..970b44b07bef
--------------------------------------------------------------------------------
[main 82ecda2b49] timers: emit warning if delay is negative or NaN
 Author: jakecastelli <959672929@qq.com>
 Date: Thu Feb 16 11:59:33 2023 +1030
 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
[main 99160b3ff3] fixup!
 Author: jakecastelli <959672929@qq.com>
 Date: Wed Jun 26 11:52:18 2024 +0930
 1 file changed, 1 insertion(+), 1 deletion(-)
[main e3f8f61393] fixup! lint
 Author: jakecastelli <959672929@qq.com>
 Date: Wed Jun 26 14:58:22 2024 +0930
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

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: #46678

Reviewed-By: Debadree Chatterjee debadree333@gmail.com

Reviewed-By: Ulises Gascón ulisesgascongonzalez@gmail.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Reviewed-By: Chengzhong Wu legendecas@gmail.com
[detached HEAD 6f4962e0ac] timers: emit warning if delay is negative or NaN

Author: jakecastelli 959672929@qq.com

Date: Thu Feb 16 11:59:33 2023 +1030

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

Rebasing (3/6)

Rebasing (4/6)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup!
PR-URL: #46678

Reviewed-By: Debadree Chatterjee debadree333@gmail.com

Reviewed-By: Ulises Gascón ulisesgascongonzalez@gmail.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Reviewed-By: Chengzhong Wu legendecas@gmail.com
[detached HEAD 65b8741bb4] fixup!

Author: jakecastelli 959672929@qq.com

Date: Wed Jun 26 11:52:18 2024 +0930

1 file changed, 1 insertion(+), 1 deletion(-)

Rebasing (5/6)

Rebasing (6/6)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup! lint
PR-URL: #46678

Reviewed-By: Debadree Chatterjee debadree333@gmail.com

Reviewed-By: Ulises Gascón ulisesgascongonzalez@gmail.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Reviewed-By: Chengzhong Wu legendecas@gmail.com
[detached HEAD dddc17ac4b] fixup! lint

Author: jakecastelli 959672929@qq.com

Date: Wed Jun 26 14:58:22 2024 +0930

1 file changed, 1 insertion(+), 1 deletion(-)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/9712029958

[nodejs-github-bot]

Landed in f5ed338