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.mdhere, 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.

[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