Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

The maximum time setTimeout can handle should be documented, and perhaps it should throw an exception when it's exceeded #3605

Closed
calmh opened this issue Jul 1, 2012 · 5 comments

Comments

@calmh
Copy link

calmh commented Jul 1, 2012

It seems that setTimeout internally uses a signed 32 bit integer to represent the number of milliseconds to "sleep", giving a maximum delay of 2147483647 ms, about 24.9 days. This might seem like a lot, and I'm sure it doesn't pop up as an issue in in-browser Javascript implementations. However, there are a few cron modules in npm that naively use setTimeout and thus break on once-a-month and similar schedules.

The bug is in the module in question, for sure, but it would be nice if the documentation in http://nodejs.org/docs/latest/api/timers.html#setTimeout mentioned the constraints.

As far as I can see, the setTimeout method is only specified in some HTML5 draft (?) and it says nothing there about a maximum delay value or what should be done if it's exceeded. I'd consider it less surprising and more robust to have setTimeout throw an exception than call the callback immediately when the requested delay is impossible...

@calmh
Copy link
Author

calmh commented Jul 1, 2012

The behavior is of course implemented in v8, so perhaps discard that part of the issue and see this only as a documentation issue.

@calmh calmh closed this as completed Jul 1, 2012
@bnoordhuis
Copy link
Member

Actually, timers are implemented in node, not V8. I've updated the docs in b53cd97.

@calmh
Copy link
Author

calmh commented Jul 1, 2012

Oh, really? Then I'll close my pull request with the doc update. :) Thanks.

@calmh
Copy link
Author

calmh commented Jul 1, 2012

Althought "... changed to 1 ms" - is that really correct, maybe just changed? In 0.8.1 it seems like a straight wrap, i.e. setTimeout(something, 50*86400*1000) will result in a callback in not 1ms and not 50 days, but something more like a few hours probably...

@bnoordhuis
Copy link
Member

That's a bug in the implementation. I've opened PR #3607 for that.

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants