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.

[jakecastelli]

Sorry guys, its been a while. Just wanted to get a bit of directions, are you guys happy with the current state of the code or want me to make more changes (e.g. emit error per process for negative number and NaN)

[debadree25]

I think lets make it once per process and land it!

[jakecastelli]

There is a bit of inconsistency in NodeJS docs here it says

setTimeout(callback[, delay[, ...args]])

delay is an option argument however down below

delay The number of milliseconds to wait before calling the callback. Default: 1.

It didn't say optional, while MDN specifically says delay is optional here

Anyway...

This PR will emit warning on code like this:

setTimeout(() =>{ /* do something */ })

Should we do this to the after see here?

after === undefined ? 1 : after *= 1; // Coalesce to number or NaN

ping @jasnell @debadree25 👀

[debadree25]

I think we should not emit a warning in the case of setTimeout(() =>{ /* do something */ }) only explicitly passing an invalid value should we generate a warning

[jakecastelli]

Agreed @debadree25, patched a fix so it will only emit when the argument is invalid.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

This should follow the usual deprecation cycle – i.e. first a doc-only deprecation should land, then this PR can land as semver-major turning it into a runtime deprecation.

[jakecastelli]

This should follow the usual deprecation cycle – i.e. first a doc-only deprecation should land, then this PR can land as semver-major turning it into a runtime deprecation.

Hi @aduh95 thanks for the feedback! Can I quickly ask about the deprecation part, as I didn't see if there is any deprecation in my PR, just wondering if I am missing out something.

[jakecastelli]

ping @aduh95 👀

[jakecastelli]

Would be great if I can get a bit more direction on this PR.

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