timers: warn on overflowed timeout duration by Fishrock123 · Pull Request #6956 · nodejs/node

Fishrock123 · 2016-05-24T20:50:58Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

timers

Description of change

Documentation is in @jasnell's PR at #6937

I realized there wasn't any clear indicator when you hit the overflow other than possibly unexpected behavior, and I think a warning may be appropriate.

cjihrig · 2016-05-24T21:23:33Z

Why not just emit a warning that the timer was invalid and converted to n?

Lots of code out there that does either setTimeout(cb) or setTimeout(cb, 0) -- even our own tests.

Fair enough. That would be quite noisy.

jasnell · 2016-05-25T03:24:12Z

This can be simplified to:
process.emitWarning(`${msecs} does not fit into a 32-bit signed integer.\n`+`Timeout duration was truncated to ${TIMEOUT_MAX}.`,'TimerOverflowWarning');
The TimerOverflowWarning name would be printed to the console and gives a consistent label to key off.

Fishrock123 · 2016-05-25T14:30:44Z

Updated, PTAL.

https://ci.nodejs.org/job/node-test-pull-request/2782/

cjihrig · 2016-05-25T15:02:15Z

LGTM

Fishrock123 · 2016-06-17T19:55:55Z

Blocked by #6937

Perviously there wasn't any clear indicator when you hit the overflow other than possibly unexpected behavior, and I think emitting a warning may be appropriate. PR-URL: nodejs#6956

Fishrock123 · 2016-06-29T10:13:23Z

@nodejs/collaborators Does this seem like a good idea or not?

mcollina · 2016-06-29T10:15:00Z

LGTM

thefourtheye · 2016-06-29T10:30:08Z


 after *= 1; // coalesce to number or NaN

+ if (after > TIMEOUT_MAX) {


If after is NaN, this condition will never be met.

NaN is like 0 or -1, and also coerces to 1, right? If so, that's probably not surprising.
I'm going off the fact that the max timeout might be surprising.

NaN won't be coerced to 1 :O

I'm going off the fact that the max timeout might be surprising.
The warning will anyway show NaN does not fit.... That would already be surprising I guess

Ohhh I get it.

ronkorving · 2016-06-29T11:34:34Z

@Fishrock123 IMHO, ideally, we would support > 31 bit (I actually had to write https://www.npmjs.com/package/safe-timers because of this, although I have to admit my use-case was browsers), but I'm guessing that may be a bit much to ask at this time.

trevnorris · 2016-06-30T19:44:28Z

Why don't we support more than 2^31-1? We can at least support 2^52 if we stay away from any bitwise operators.

RReverser · 2016-07-01T09:20:46Z

@trevnorris Are there any real-world use-cases for such timeouts?

ronkorving · 2016-07-01T09:50:20Z

I had one where expiration time of data was driven by user input. The timer fired instantly when the timer was far in the future.

RReverser · 2016-07-01T09:55:41Z

@ronkorving Not arguing that we should warn on overflows, but thinking that we probably should not set such timers at all after the warning rather than keeping them in the event loop as they're unlikely to fire.

ronkorving · 2016-07-01T10:05:09Z

I agree. If we have an upper bound we go beyond, it's better (in my use-case anyway, I can't speak for others) not to fire the timer at all.

Fishrock123 · 2016-07-01T15:57:35Z

IIRC coercing was to match browser behavior?

I think you can still have a larger timeout by manually creating one with enroll() and active()?

jasnell · 2016-08-04T22:48:05Z

Ping @Fishrock123 ...

imyller · 2016-09-15T22:29:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4063/

ping @Fishrock123

Fishrock123 · 2016-09-17T21:27:43Z

Wonder what @thealphanerd thinks of this.

MylesBorins · 2016-09-18T08:30:28Z

Let's see what citgm says. Warnings have been surprisingly breaking in the past

citgm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/390/

mcollina · 2016-09-19T10:15:25Z

LGTM.

I'm not fond of this warning, I would rather fix the actual issue. IMHO we should throw if such a timer is added, but that is semver-major.

If I made my calculations right, the max timer is between 24 and 25 days. I think this is big enough.
Maintaining a data structure within core for that long period of time sounds like a design smell from my point of view.

imyller · 2016-09-19T10:47:28Z

@mcollina

Maintaining a data structure within core for that long period like a design smell

Generally, not just applied to timers, in many embedded industrial applications (area where I work in) process lifetimes can be measured in years rather than days or hours. Especially if the system is controlling uninterruptable physical process.

It is true, that there is a hint of bad design in such long timer timeout periods, but generally core runtime should be robust enough to support any long running, uninterrupted, or semi-mission-critical tasks; as long as code API/ABI usage is bug free and to the core spec. If core has any limitations against that, I'd consider it a serious core issue.

Node.js might not be certified or ready for primetime in this business area just yet, but that shouldn't stop us from keeping that capability as a goal.

mcollina · 2016-09-19T11:27:09Z

@imyller I agree with you, we should fix it.

I am just saying that keeping such a long timer going should be the responsibility of another piece of software (core or npm). I do not see this as a limitation, we can just link to that module from the docs.

I am also in to internalize that module as a timers.setLongTimeout() thing.

Node.js timers are optimized for something completely different: short lived timers (ence the limit). It is usually hard to optimize some for two different competing goals.

I do not like the warning. Either we can fix it and support longer timers with the same perf, or we should throw, and maybe suggest to use another API or module.

Processes that run uninterrupted for more than a lunar month are hard to see in modern cloud stacks.

Fishrock123 · 2016-09-29T14:08:18Z

I don't feel good about throwing, I feel that too many applications may pass user input directly.

jasnell · 2017-01-06T19:36:28Z

ping @Fishrock123 ... status on this one?

Fishrock123 · 2017-01-06T21:09:16Z

Unsure, do people think this is a good idea?

I don't really want to do it unless a significant number of people do tbh. It could still be a good idea not to ignore though.

mcollina · 2017-01-06T21:26:03Z

I think we should do something about it. I would prefer a throw, but a warning would be ok too.

jasnell · 2017-03-01T00:55:07Z

How about we print a deprecation warning on overflow in Node.js 8.0.0 and move towards a throw in 9 or later?

mcollina · 2017-03-01T14:49:47Z

How about we print a deprecation warning on overflow in Node.js 8.0.0 and move towards a throw in 9 or later?

I am 👍 on that.

cjihrig · 2017-03-01T14:56:18Z

I have mixed feelings about a deprecation warning for an overflow. I think it should be a normal warning, indicating overflow. We could also mention in that warning that it will throw in future releases (because we don't want to emit two warnings).

jasnell · 2017-03-01T15:58:42Z

I'd be good with a regular warning in 8.x and a throw in 9x or later.

Fishrock123 · 2017-03-01T16:28:57Z

I'm not sure how I feel about throwing. IIRC browsers cap similarly and also do not throw and we kinda sorta follow them here.

I think the best suggestion was just to uncap it. Not sure I'll get around to that though.

ronkorving · 2017-03-02T10:46:58Z

How about supporting large numbers instead? I've tried to promote this before, and it didn't seem to get much traction. Still, I personally think it's valid as I really don't like (seemingly) arbitrary limits. I wrote a module to deal with large intervals, which perhaps may serve as inspiration: https://www.npmjs.com/package/safe-timers

If there is a limit, I think a warning is more appropriate than a throw because the browser also won't throw. I kinda wish it did though.

edit: Reading again, I think that's exactly what @Fishrock123 means by uncapping? :) Sorry for the redundancy if so.

Fishrock123 · 2017-03-06T17:15:16Z

Yeah I think we should just aim to support large numbers.

jasnell · 2017-05-28T21:43:25Z

This isn't going to make it for 8.0.0

BridgeAR · 2017-09-12T18:42:50Z

I guess this approach is not what we want anymore and instead the limit should just be removed?

@Fishrock123 are you going to follow up on this?

jasnell · 2017-09-15T23:20:11Z

Given the complete lack of activity on this, I'm inclined to close. @Fishrock123 Please reopen if you disagree.

Previously there wasn't any clear indicator when you hit the overflow other than possibly unexpected behavior, and I think emitting a warning may be appropriate. PR-URL: nodejs/node#6956

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label May 24, 2016

cjihrig reviewed May 24, 2016
View reviewed changes

Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label May 24, 2016

jasnell reviewed May 25, 2016
View reviewed changes

Fishrock123 force-pushed the warn-on-timeout-overflow branch from 08222b4 to f0fab62Compare May 25, 2016 14:29

Fishrock123 added the blocked PRs that are blocked by other issues or PRs. label Jun 17, 2016

Fishrock123 mentioned this pull request Jun 28, 2016
doc: general improvements to timers.md #6937
Closed
2 tasks

Fishrock123 removed the blocked PRs that are blocked by other issues or PRs. label Jun 29, 2016

timers: warn on overflowed timeout duration
0367986
Perviously there wasn't any clear indicator when you hit the overflow other than possibly unexpected behavior, and I think emitting a warning may be appropriate. PR-URL: nodejs#6956

Fishrock123 force-pushed the warn-on-timeout-overflow branch from f0fab62 to 0367986Compare June 29, 2016 10:13

thefourtheye reviewed Jun 29, 2016
View reviewed changes

Trott force-pushed the master branch from b0df363 to c5ce7f4Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88Compare October 18, 2016 17:01

jasnell added the stalled Issues and PRs that are stalled. label Jan 6, 2017

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67Compare February 1, 2017 01:00

targos added this to the 8.0.0 milestone Mar 1, 2017

refack force-pushed the master branch from 16073c0 to fbe946bCompare April 14, 2017 04:11

jasnell removed this from the 8.0.0 milestone May 28, 2017

jasnell closed this Sep 15, 2017

Fishrock123 mentioned this pull request Sep 19, 2017
timers: warn on overflowed timeout duration ayojs/ayo#71
Closed
3 tasks

jasnell mentioned this pull request Sep 26, 2017
timers: warn on overflowed timeout duration #15627
Closed
3 tasks


		after *= 1; // coalesce to number or NaN

		if (after > TIMEOUT_MAX) {

Uh oh!

Uh oh!

Conversation

Fishrock123 commented May 24, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnellMay 25, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented May 25, 2016

Uh oh!

cjihrig commented May 25, 2016

Uh oh!

Fishrock123 commented Jun 17, 2016

Uh oh!

Fishrock123 commented Jun 29, 2016

Uh oh!

mcollina commented Jun 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronkorving commented Jun 29, 2016

Uh oh!

trevnorris commented Jun 30, 2016

Uh oh!

RReverser commented Jul 1, 2016

Uh oh!

ronkorving commented Jul 1, 2016

Uh oh!

RReverser commented Jul 1, 2016

Uh oh!

ronkorving commented Jul 1, 2016

Uh oh!

Fishrock123 commented Jul 1, 2016

Uh oh!

jasnell commented Aug 4, 2016

Uh oh!

imyller commented Sep 15, 2016

Uh oh!

Fishrock123 commented Sep 17, 2016

Uh oh!

MylesBorins commented Sep 18, 2016

Uh oh!

mcollina commented Sep 19, 2016

Uh oh!

imyller commented Sep 19, 2016

Uh oh!

mcollina commented Sep 19, 2016

Uh oh!

Fishrock123 commented Sep 29, 2016

Uh oh!

jasnell commented Jan 6, 2017

Uh oh!

Fishrock123 commented Jan 6, 2017

Uh oh!

mcollina commented Jan 6, 2017

Uh oh!

jasnell commented Mar 1, 2017

Uh oh!

mcollina commented Mar 1, 2017

Uh oh!

cjihrig commented Mar 1, 2017

Uh oh!

jasnellMay 25, 2016•
edited
Loading

ronkorving commented Mar 2, 2017•
edited
Loading