Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confused setTimout behaviour in NodeJS #46596

Open
jakecastelli opened this issue Feb 10, 2023 · 16 comments
Open

Confused setTimout behaviour in NodeJS #46596

jakecastelli opened this issue Feb 10, 2023 · 16 comments
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@jakecastelli
Copy link
Member

jakecastelli commented Feb 10, 2023

Version

14, 18

Platform

22.2.0 Darwin Kernel arm64

Subsystem

No response

What steps will reproduce the bug?

setTimeout(() => {
  console.log('calling 1st timeout')
}, 1)

setTimeout(() => {
  console.log('calling 2rd timeout')
}, 0)

// in nodejs
// calling 1st timeout
// calling 2rd timeout

// however, in chrome browser
// calling 2rd timeout
// calling 1st timeout

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

Would expect the second time to be called first

What do you see instead?

The first time out is called then the second timeout in NodeJS

Additional information

No response

@bnoordhuis
Copy link
Member

That's working as designed. From the timers documentation:

When delay is larger than 2147483647 or less than 1, the delay will be set to 1.

The WHATWG spec leaves some wiggle room ("This API does not guarantee that timers will run exactly on schedule") so some variation between implementations is to be expected.

Node could be changed to behave more like Chrome but there's a non-zero risk of ecosystem fallout.

@bnoordhuis bnoordhuis added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Feb 10, 2023
@jakecastelli
Copy link
Member Author

jakecastelli commented Feb 10, 2023

@bnoordhuis Thanks for clarifying it Ben!
I found even node 14 and node 18 behave a little bit different as well with the following code snippet.

for example:

setTimeout(() => {
  console.log('calling 1st timeout')
}, 3)

setTimeout(() => {
  console.log('calling 2rd timeout')
}, 2)

in node 14 it will be:

calling 1st timeout
calling 2rd timeout

however in node 18 it will be

calling 2rd timeout
calling 1st timeout

I guess there is no practical use case for the above in any event but just something I found interesting and not really documented somewhere.

@BridgeAR
Copy link
Member

I am in favor of changing the behavior for 0 to behave as chrome does.

Besides that, I would actually like to deviate from the spec when it comes to exceeding 2147483647. It is not user friendly at all to just change the timing to a lower number due to the overflow. Instead, we could either accept numbers up until Number.MAX_SAFE_INTEGER and or throw above that limit. It is otherwise pretty much impossible to detect these faulty values.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 11, 2023

I agree completely that the behavior in the spec is not good, but I don't think we should deviate from it.

@jasnell
Copy link
Member

jasnell commented Feb 11, 2023

Changing the timing of this stuff always carries a non-zero risk of breaking apps. While the inconsistency with browsers is unfortunate, I agree that we likely shouldn't change it.

@BridgeAR BridgeAR added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 11, 2023
@bnoordhuis
Copy link
Member

Perhaps someone wants to open a pull request and run it through citgm to get a feel for how bad it impacts the ecosystem?

@mcollina
Copy link
Member

I don't think we should deviate from the spec. I could be convinced if this would cause no ecosystem breakage.

@jakecastelli
Copy link
Member Author

Wanna point out the code blow in chrome browser does not set the delay to 1 which is a little bit surprising.

setTimeout(() => {
  console.log('finished')
}, 100000000000000)

@jakecastelli
Copy link
Member Author

jakecastelli commented Feb 15, 2023

@bnoordhuis Thanks for clarifying it Ben! I found even node 14 and node 18 behave a little bit different as well with the following code snippet.

for example:

setTimeout(() => {
  console.log('calling 1st timeout')
}, 3)

setTimeout(() => {
  console.log('calling 2rd timeout')
}, 2)

in node 14 it will be:

calling 1st timeout
calling 2rd timeout

however in node 18 it will be

calling 2rd timeout
calling 1st timeout

I guess there is no practical use case for the above in any event but just something I found interesting and not really documented somewhere.

Is it possible to discuss the above in tsc agenda meeting as well (despite it is mentioned in the doc - The callback will likely not be invoked in precisely delay milliseconds. 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.), looks like there is a behaviour change in node v18.x in comparison to previous node versions but it is not being mentioned in the doc history.
Screenshot 2023-02-16 at 1 19 23 am

@mcollina
Copy link
Member

There is no behavior change, timers are weird for this and they highly depend on the event loop phases, the startup time of Node.js and the underlining operating system.

@BridgeAR
Copy link
Member

BridgeAR commented Feb 15, 2023

The standard itself does actually not provide any restrictions when it comes to zero being handled as one or the maximum value being a 32-bit signed integer. The latter seems to be a historic implementation detail of browsers while not ending in the spec itself.
https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#run-steps-after-a-timeout

I suggest to split the discussion into three parts:

  1. How to handle zero (the spec only defines to set the value to zero in case the value is below zero)
  2. What to do with timers set above 2 ** 32? (the spec does not provide any limit)
  3. How to handle nesting level as defined by the spec. It is optional as it seems and we do not implement this.

@bnoordhuis
Copy link
Member

What to do with timers set above 2 ** 32? (the spec does not provide any limit)

For the record, node follows what browsers did ca. 2011: nodejs/node-v0.x-archive#593 (comment)

@BridgeAR
Copy link
Member

The 1 ms minimum has recently been removed from chrome: https://chromestatus.com/feature/4889002157015040

Seems like the browsers do improve the situation minimally and as such, we could also do that. It would of course be a breaking change that would have to be communicated.
I won't get to it before the weekend but I plan on opening a PR for the a couple of minor improvements that are all in line with the spec to improve our limits and check CITGM results.

jakecastelli added a commit to jakecastelli/node that referenced this issue Feb 16, 2023
Partially addressed nodejs#46596 to keep the consistency of the warning message for TIMEOUT_MAX number as the negative number will be set to 1.
jakecastelli added a commit to jakecastelli/node that referenced this issue Feb 16, 2023
Partially addressed nodejs#46596 to keep the consistency of the warning message for TIMEOUT_MAX number as the negative number will be set to 1.
jakecastelli added a commit to jakecastelli/node that referenced this issue Feb 16, 2023
Partially addressed nodejs#46596.

To keep the consistency of the warning message for TIMEOUT_MAX number
As the negative number will be set to 1.
@jakecastelli
Copy link
Member Author

jakecastelli commented Feb 17, 2023

Hi Ruben @BridgeAR 👋 thank you for your thoughts and reply! I followed the link to the spec but didn't find the reference for this one 🤔 would you be able to elaborate:

How to handle zero (the spec only defines to set the value to zero in case the value is below zero)

From what I understand, nodejs will set the negative numbers or 0 or NaN to 1.

jakecastelli added a commit to jakecastelli/node that referenced this issue Feb 19, 2023
Partially addressed nodejs#46596.

To keep the consistency of the warning message for TIMEOUT_MAX number
As the negative number will be set to 1.
@apapirovski apapirovski removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 23, 2023
@apapirovski
Copy link
Member

We briefly discussed this during the TSC meeting but we did not have a sufficient audience to provide the counter perspective. From my perspective, and I believe it's echoed by @jasnell, we already don't follow the standard today and trying to make this change doesn't meaningfully change that.

In addition, the behaviour of timers is more problematic in a server environment as compared to the browser. Anecdotally I've also seen reliance on this behaviour — as silly as that is — out in the wild and I'm not sure as to the value of a semver major change here.

I would be very clear -1 on this change or other changes to try & align with the spec without some clear evidence that it would be net-positive for the project.

@Fishrock123
Copy link
Contributor

So Chrome “fixed” their lower-end clamping to 1ms? Is that the deal here?

If so Node.js should probably get rid of that too. It was confusing and there are probably more bugs that exist because of that behavior than will exist when removing that behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

No branches or pull requests

8 participants