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.

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 <[email protected]>
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 <[email protected]>
 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 <[email protected]>
 Date: Wed Jun 26 11:52:18 2024 +0930
 1 file changed, 1 insertion(+), 1 deletion(-)
[main e3f8f61393] fixup! lint
 Author: jakecastelli <[email protected]>
 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 [email protected]

Reviewed-By: Ulises Gascón [email protected]

Reviewed-By: Benjamin Gruenbaum [email protected]

Reviewed-By: Chengzhong Wu [email protected]
[detached HEAD 6f4962e0ac] timers: emit warning if delay is negative or NaN

Author: jakecastelli [email protected]

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 [email protected]

Reviewed-By: Ulises Gascón [email protected]

Reviewed-By: Benjamin Gruenbaum [email protected]

Reviewed-By: Chengzhong Wu [email protected]
[detached HEAD 65b8741bb4] fixup!

Author: jakecastelli [email protected]

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 [email protected]

Reviewed-By: Ulises Gascón [email protected]

Reviewed-By: Benjamin Gruenbaum [email protected]

Reviewed-By: Chengzhong Wu [email protected]
[detached HEAD dddc17ac4b] fixup! lint

Author: jakecastelli [email protected]

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