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

Timers default at 1 due to historical reasons #20596

Closed
benjamingr opened this issue May 8, 2018 · 9 comments
Closed

Timers default at 1 due to historical reasons #20596

benjamingr opened this issue May 8, 2018 · 9 comments
Labels
discuss Issues opened for discussions and feedbacks. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@benjamingr
Copy link
Member

benjamingr commented May 8, 2018

@nodejs/timers

Some context: I'm helping maintain lolex which is a fake timer library. Currently @SimenB is working on porting jest to use lolex which means we're running into a bunch of edge cases internally. Both are large-ish Node.js packages with >1M weekly NPM downloads.

We currently set timers that exceed 2 ** 31 - 1 to 1. According to our code this is in order to match browser behaviour.

Browser timers are specced unlike ours - the DOM spec coerces the timeout to a Uint32 which converts overflows to 0 rather than 1.

This is more in tune of what browsers actually do though there are no relevant tests in the webidl test suite so we can't really know what browsers do, though:

setTimeout(() => console.log(1), Infinity);
setTimeout(() => console.log(2), 0);

Logs 1, 2 in Chrome and Node, except Node logs a pretty confusing warning saying the timeout was set to 1 (since we set timeouts < 1 to 1).

This is another difference in the web behavior which allows setting the timeout to 0 but automatically sets it to 5 once it's been nested four times:

Timers can be nested; after five such nested timers, however, the interval is forced to be at least four milliseconds.

I asked @bnoordhuis who committed the code to Node and he did this in order to match browsers which makes a lot of sense since we don't allow timers to be 0. Moreover, actual Chrome also doesn't allow 0 as a timeout:

setTimeout(() => console.log(2), 1);
setTimeout(() => console.log(1), 0);

Logs 2, 1 in both Chrome and Node. (Changing 1 to 2 fixes this).


My assumption here is that Chrome deviates from the timers spec due to historical reasons (websites not working or something similar). The question is:

  • Do we like our current behaviour? (Which aligns with Chrome and both deviate from the spec)
  • Does anyone know why Chrome behaves this way?
@benjamingr benjamingr added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. discuss Issues opened for discussions and feedbacks. labels May 8, 2018
@ryzokuken
Copy link
Contributor

@benjamingr what's the status on this?

@benjamingr
Copy link
Member Author

Still waiting for feedback and hoping for people to weigh in cc @nodejs/timers

@apapirovski
Copy link
Member

I haven't said much because I'm ok with both behaviours and also because changing this now could have some fallout due to ordering inconsistencies. It would be a pretty major change.

@Fishrock123
Copy link
Contributor

Maybe @domenic would know why chrome acts the way it does?

It seems we match browser behavior well enough. I think that we cannot change the coercion of 1 to 0 because that will likely break many, many people's programs and I would be strongly adverse to that idea.

@Fishrock123
Copy link
Contributor

Ok well, I just recently had this conversation with Domenic in which he seemed to not know this behavior existed: https://twitter.com/Fishrock123/status/1025141858878537728

tl;dr: the 0 to 1 coalesce behavior exists on every major browser except Firefox, which gets it "right", whatever "right" is here.

There's a LOT of node code that mistakenly uses 0 instead of 1, thinking it is "faster" or "fastest", without actually digging into the ordering of things. I'm very worried we could break a lot by changing this.

If doing something is desired, I suggest we experiment with issuing a warning.

@Pho3nixHun
Copy link

I have a nasty bug on v8.9.4 where the 0 value on setTimeout or setImmediate causes the "thread" hanging there until the next cycle. I'm not sure how, what info with I can help to solve this bug.

@benjamingr
Copy link
Member Author

I have a nasty bug on v8.9.4 where the 0 value on setTimeout or setImmediate causes the "thread" hanging there until the next cycle. I'm not sure how, what info with I can help to solve this bug.

Well, a reproduction repo with an MCVE we can run would be a great start. You can also go to the Node channel on IRC if you want synchronous help with making one.

@Pho3nixHun
Copy link

This might take some time, since the project is quite complicated. I'll get back to this topic once I have the required info.

@benjamingr
Copy link
Member Author

This discussion has idled - thanks everyone :)

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

No branches or pull requests

5 participants