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

setTimeout imposes minimum 1-millisecond delay #34728

Closed
fmela opened this issue Aug 11, 2020 · 17 comments
Closed

setTimeout imposes minimum 1-millisecond delay #34728

fmela opened this issue Aug 11, 2020 · 17 comments
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@fmela
Copy link

fmela commented Aug 11, 2020

  • Version: v14.7.0
  • Platform: MacOS 10.14.6

What steps will reproduce the bug?

Using the following code, the counter reaches around 690 on my machine:

let counter = 0;
function doit() { counter++; setTimeout(doit, 0); }
setTimeout(() => {
    console.log('Counter:', counter);
    require('process').exit(0);
}, 1000);
setImmediate(doit);

Specifying a delay of 1 instead of 0 produces the same result. Changing "setTimeout(doit, 0)" to "setImmediate(doit)" results in the counter reaching around 580,000.

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

This occurs any time setTimeout is invoked with a delay argument that is zero.

What is the expected behavior?

setTimeout with a delay of 0 should not impose a 1-millisecond delay.

What do you see instead?

setTimeout imposes a minimum 1-millisecond delay.

@addaleax addaleax added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Aug 11, 2020
@addaleax
Copy link
Member

No strong opinion here, but imo setting setTimeout(fn, 0) and setTimeout(fn) to just be setImmediate(fn) would a) make sense and b) be considered a breaking change.

@nodejs/timers

@devsnek
Copy link
Member

devsnek commented Aug 11, 2020

setTimeout 0 and undefined sounds like a programming error to me.

@fmela
Copy link
Author

fmela commented Aug 11, 2020

@devsnek According to https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout, setTimeout of zero is valid, and if the argument is not specified, it is assumed to be zero.

@devsnek
Copy link
Member

devsnek commented Aug 11, 2020

@fmela that's not true in any implementation I'm aware of. node apparently sets it to 1 and browsers have a minimum between 4-16ms.

@fmela
Copy link
Author

fmela commented Aug 11, 2020

@devsnek https://nodejs.org/api/timers.html#timers_settimeout_callback_delay_args states: "When delay is larger than 2147483647 or less than 1, the delay will be set to 1."

So this is intended behavior. I don't understand the justification for imposing a minimum 1-millisecond delay, though.

@bnoordhuis
Copy link
Member

See nodejs/node-v0.x-archive#593 and commit 7fc835a - basically, it's because node wanted to mimic browser behavior.

Preempting the "why not 4 ms then?" question: because there's no frame rate in node.

@benjamingr
Copy link
Member

The current behavior is quite intentional - I think there is an issue from before (other than the one Ben linked to) I recall having to align with this behavior in Sinon (I help maintain fake-timers).

@benjamingr
Copy link
Member

@fmela and thank you for raising this issue and caring. We need more people to care about this, be engaged and contribute in timers and figure out what the correct behavior should be :] I appreciate it.

  • Given the issue Ben linked to and the fact this is intentional behavior because of timers, do you still feel like we should consider making a breaking change here?
  • Would you mind making a docs PR explaining/documenting the behavior of setting the timeout to 1 if it is 0?
    • I think it would be useful even if we don't end up changing the behavior.

@bnoordhuis
Copy link
Member

Closing, no follow-up.

@bnoordhuis bnoordhuis added the wontfix Issues that will not be fixed. label Aug 15, 2020
@benjamingr
Copy link
Member

To be clear, if at any point you wish to follow up @fmela we are happy to have a discussion here and figure out how to proceed.

Maybe @itayperry (who made a bunch of PRs to sinon) wants to pick up the docs PR part?

@itayperry
Copy link

I would love to help in any way possible :)

@benjamingr
Copy link
Member

@itayperry this needs a docs pr basically

@itayperry
Copy link

Is the docs update needed for explaining the current built-in behavior, or should I do the pr only if an actual change was made?

@fmela
Copy link
Author

fmela commented Aug 18, 2020

Thank you @benjamingr. Sorry, I lost track of this issue.

My two cents: the considerations of browsers, which are user-facing and must maintain interactive responsiveness, are not concerns that should be shared by node. It's reasonable to impose a minimum delay in setTimeout when used in a browser in order to prevent a particular script or tab from causing CPU starvation and harming interactivity. In addition, it seems arbitrary that node enforces a 1-millisecond minimum (why that number and not .01, .1, or 10 milliseconds, for example)?

node already provides the setImmediate facility which makes it easy to use up all the CPU if used incorrectly (or even intentionally), so it's not clear why setTimeout should prevent the user from doing the same. Similarly, browsers don't provide setImmediate due to differing concerns; it isn't appropriate for interactive user-facing applications.

I feel that node's implementation should trust the developer when they give a timeout of zero. It's not hard to imagine unintended latency on the order of milliseconds being introduced in server-side node applications in which the developer hasn't realized that setTimeout(, 0) silently introduces one millisecond of latency each time it is used. Anecdotally, it came as a surprise to me.

Thanks for your consideration!

@nullhook
Copy link

Chromium now removes the clamping: https://chromestatus.com/feature/4889002157015040

setTimeout(fn, 0) is immediate

@BridgeAR
Copy link
Member

Thanks for bringing this up @nullhook. I am going to reopen this as it does seem to become an option to change setTimeout(fn, 0) to behave as setImmediate(fn)

@BridgeAR BridgeAR reopened this Jun 23, 2022
@BridgeAR BridgeAR removed the wontfix Issues that will not be fixed. label Jun 23, 2022
@apapirovski
Copy link
Contributor

Closing given there's now an actual discussion on this topic in #46596 which was part of the tsc-agenda. I don't believe we need two issues tracking the same matter.

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

9 participants