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: support BigInt in timers API #22086

Closed
wants to merge 5 commits into from

Conversation

idando
Copy link

@idando idando commented Aug 2, 2018

Adding support for BigInt in timers API.
This change allows passing a BigInt value as delay when calling setTimeout and setInterval.
The logic of timers was not changed, and the value limits are still enforced.
Tests and docs were updated to include cases of passing BigInt to these interfaces.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

#SimilarWeb_RnD

@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Aug 2, 2018
@benjamingr
Copy link
Member

Thank you for doing this!

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs and code changes LGTM though I am not sure if we want to explicit document the fact delay can be a bigint because that might imply that passing a value that doesn't fit in a number is meaningful.

Thanks for your contribution!

@benjamingr
Copy link
Member

@nodejs/timers in case anyone has a strong opinion about this. I'm personally +1 on allowing BigInts in more APIs as this is language compatibility and user expectation but I want to make sure we're not landing anything controversial without talking about it first.

@idando I'll explain how PR approvals work - basically this changes will be open for 72 hours and if there are no objections given and at least one collaborator approved it (me in this case) it will land. Other collaborators may show up and comment on it.

@Slayer95
Copy link

Slayer95 commented Aug 2, 2018

@benjamingr Regarding compatibility, it's of note that the current Stable build of Chrome (68) does not accept BigInts in the setTimeout and setInterval functions. Therefore, one needs to answer: if Node.js takes the lead, is it likely that other implementations would follow?

Furthermore, when working with BigInts, it's one of the guiding principles that implicit conversion should not be applied. In that sense, this goes against user expectations. On the other hand, allowing BigInts to be passed would introduce a wrong assumption that the timer functions now accept large values without overflowing.

Therefore, I'd argue that this has plenty of usability downsides ("footguns"), while it's not clear what sort of application logic would lead to straightforward and correct usage of a BigInt in these functions.

@mscdex
Copy link
Contributor

mscdex commented Aug 2, 2018

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I appreciate the effort, I don't see any evidence this conforms to the spec or is desired by users. Given that existing limits remain in place I also think this is more confusing than helpful.

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I share concerns about this... i’m Not sure what spec the browser follows here, but we certainly do not follow it with any exactness anyways.

That being said I’m not sure the behavior here is really intuitive - timers certainly can’t support anything larger than what a unint64_t can Store anyway, even if we were to not coalesce...

Need to think about it more when I’m at my laptop.

@joyeecheung
Copy link
Member

Regarding compatibility, it's of note that the current Stable build of Chrome (68) does not accept BigInts in the setTimeout and setInterval functions. Therefore, one needs to answer: if Node.js takes the lead, is it likely that other implementations would follow?

FWIW I think that can be answered if someone opens a PR to https://github.com/web-platform-tests/wpt/tree/master/html/webappapis/timers - the browsers run those tests so they'll notice

@littledan
Copy link

I'm not sure about this change. I typically recommend BigInt for values which may range higher than 2^53. For example, when counting ns since the Unix epoch, BigInt is probably better than Number, but here I'm not sure. Do you expect timers to be used with such large values?

@joyeecheung
Copy link
Member

@littledan The current behavior is documented as

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

@benjamingr
Copy link
Member

I don't understand why the pushback - it seems very reasonable to use BigInts as intervals - people might be using them because they're Ints and not for the Big part for safety (to make sure they don't setTimeout with a NaN) for instance.

The use case certainly seems legitimate to me - even if DOM timers don't support it yet.

@devsnek
Copy link
Member

devsnek commented Aug 3, 2018

I'm okay with the idea of longer intervals but we would have to perform hacky timer refreshing because libuv is c++.

on the other hand if you're dealing with time intervals longer than 24 days I would argue you shouldn't be using timers

@littledan
Copy link

Does this patch even add support for longer intervals? It looks like it just coerces the BigInt to a Number. If that's behavior, I would rather continue throwing an exception for use of BigInt here.

@benjamingr
Copy link
Member

Does this patch even add support for longer intervals? It looks like it just coerces the BigInt to a Number. If that's behavior, I would rather continue throwing an exception for use of BigInt here.

It does not, and I think I'm -1 on supporting large intervals (since it encourages the assumption servers and machines never go down).

Maybe we should improve the exception instead of allowing it? I still think allowing it makes more sense though (for the int in the bigInt part).

Another alternative is throwing on BigInts that are larger than the interval?

@Fishrock123
Copy link
Contributor

Fishrock123 commented Aug 3, 2018

people might be using them because they're Ints and not for the Big part for safety (to make sure they don't setTimeout with a NaN) for instance.

I'm not really sure how being an int helps anything here. Precision is already a little bit off no matter what (we don't support nanosecond timers, and event loop delay can happen).

Also, I'm not sure what reasonable Number you could provide that would end up being NaN? Could you elaborate?

@littledan
Copy link

I'd like to discourage people from using BigInt just because it's an int. This is actually why we put "big" in the name. Number is a perfectly good way to represent integers that are always small.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also -1 on this largely because I'm not seeing the value.

@addaleax
Copy link
Member

addaleax commented Sep 2, 2018

I’m closing this since it doesn’t seem like consensus can be reached here, and there has been no activity over the last 4 weeks. This can always be re-opened if there’s more discussion, though.

@addaleax addaleax closed this Sep 2, 2018
@benjamingr
Copy link
Member

I’m closing this since it doesn’t seem like consensus can be reached here, and there has been no activity over the last 4 weeks. This can always be re-opened if there’s more discussion, though.

It seems like consensus was reached and it was not to allow BigInts in timers. I am positive on the change (still) but have no strong feelings about it. Others have strong feelings against it.

I definitely see value in a PR to improve the error message on passing bigints to timers though if you want to give that a jab @idando

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

Successfully merging this pull request may close these issues.