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

doc: add note about timeout delay > TIMEOUT_MAX #3512

Closed
wants to merge 3 commits into from

Conversation

sitegui
Copy link
Contributor

@sitegui sitegui commented Oct 25, 2015

When setTimeout() and setInterval() are called with delay greater than TIMEOUT_MAX (2147483647), the supplied value is ignored and 1 is used instead. This adds a note about this in the timers docs.

I actually got surprised by this behavior when trying to schedule a job to run in the first day of every month:

function schedule() {
    var now = new Date
    var nextMonth = new Date(now.getFullYear(), now.getMonth()+1)
    setTimeout(function () {
        shedule()
        doStuff()
    }, nextMonth - now)
}

It worked the first time, but afterwards it seemed to enter an infinite loop. I only found out why after searching and reading the timers.js code.
It only worked at first because it was past the 5th day of that month ;)

When setTimeout() and setInterval() are called with `delay` greater
than TIMEOUT_MAX (2147483647), the supplied value is ignored and 1 is
used instead. This adds a note about this in the timers docs.
@@ -16,6 +16,8 @@ It is important to note that your callback will probably not be called in exactl
the callback will fire, nor of the ordering things will fire in. The callback will
be called as close as possible to the time specified.

To follow browser behavior, when using delays larger than 2147483647 milliseconds (approximately 25 days), the timeout is executed immediately, as if the `delay` was set to 1.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe say "If delay is less than one, or greater than the maximum allowed value of 2147483647, it will be assigned a value of 1."

@mscdex mscdex added doc Issues and PRs related to the documentations. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Oct 25, 2015
@@ -16,6 +16,8 @@ It is important to note that your callback will probably not be called in exactl
the callback will fire, nor of the ordering things will fire in. The callback will
be called as close as possible to the time specified.

To follow browser behavior, when using delays larger than 2147483647 milliseconds (approximately 25 days) or less than 1, the timeout is executed immediately, as if the `delay` was set to 1.
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: word wrap 80 chars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ops, sorry

@trevnorris
Copy link
Contributor

Thanks for the added doc changes. LGTM.

@nodejs/tsc I don't believe using 2^31-1 as the TIMEOUT_MAX is in the spec. So what about fixing the impl to allow even larger values?

@indutny
Copy link
Member

indutny commented Oct 27, 2015

@trevnorris I don't mind, though we will need to document the lost of precision for the values greater than 2^53-1

@trevnorris
Copy link
Contributor

Okay. I'll leave this open and try to get to a fix by the end of the week.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 27, 2015

+1 if browser consistency is the only reason it's implemented like this. Would this only affect someone trying to run Node code in the browser?

@trevnorris
Copy link
Contributor

@cjihrig Think so. But at that point it's really UB, since in Firefox it will throw if the timeout is larger than 2^31 - 1 while on Chrome it will wrap. Test case:

setTimeout(()=>console.log('hi'),(-1 >>> 1) + 1);

EDIT: Firefox doesn't throw. It also wraps.

@bnoordhuis
Copy link
Member

it will wrap

Browsers don't wrap (in the modulo sense), they expire the timer immediately. If you enter the following in the web console:

> setTimeout(() => console.log('two'), 60e3 + Math.pow(2,31)); setTimeout(() => console.log('one'), Math.pow(2,31))
1
two
one

While with modulo arithmetic it would print "one two" (with a 60 second delay between them.)

I added TIMEOUT_MAX way back because someone complained node was different from the browser, so apparently it's relevant at least to some people.

@Fishrock123
Copy link
Contributor

I'd suggest just making an npm module that wraps timeout and spreads it out across multiple timeouts if you need anything larger.

@trevnorris
Copy link
Contributor

K. Then let's land this so devs realize a module should be made. :-)

@cjihrig
Copy link
Contributor

cjihrig commented Nov 2, 2015

If anyone from the future reads this, there are already a few such modules out there.

@Trott
Copy link
Member

Trott commented Nov 2, 2015

PR LGTM

@trevnorris
Copy link
Contributor

Thanks. Landed on ac7dd5f.

@trevnorris trevnorris closed this Nov 2, 2015
trevnorris pushed a commit that referenced this pull request Nov 2, 2015
When setTimeout() and setInterval() are called with `delay` greater than
TIMEOUT_MAX (2147483647), the supplied value is ignored and 1 is used
instead. Add a note about this in the timers docs.

PR-URL: #3512
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 7, 2015
When setTimeout() and setInterval() are called with `delay` greater than
TIMEOUT_MAX (2147483647), the supplied value is ignored and 1 is used
instead. Add a note about this in the timers docs.

PR-URL: #3512
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
MylesBorins pushed a commit that referenced this pull request Nov 16, 2015
When setTimeout() and setInterval() are called with `delay` greater than
TIMEOUT_MAX (2147483647), the supplied value is ignored and 1 is used
instead. Add a note about this in the timers docs.

PR-URL: #3512
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins
Copy link
Contributor

landed in lts-v4.x-staging as d02365b

rvagg pushed a commit that referenced this pull request Dec 4, 2015
When setTimeout() and setInterval() are called with `delay` greater than
TIMEOUT_MAX (2147483647), the supplied value is ignored and 1 is used
instead. Add a note about this in the timers docs.

PR-URL: #3512
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
When setTimeout() and setInterval() are called with `delay` greater than
TIMEOUT_MAX (2147483647), the supplied value is ignored and 1 is used
instead. Add a note about this in the timers docs.

PR-URL: #3512
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
When setTimeout() and setInterval() are called with `delay` greater than
TIMEOUT_MAX (2147483647), the supplied value is ignored and 1 is used
instead. Add a note about this in the timers docs.

PR-URL: #3512
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. 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.

10 participants