timers: warn on overflowed timeout duration #71

Fishrock123 · 2017-09-19T18:29:23Z

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.

While it may be viable to allow the use of 64bit numbers in enroll(), browser setTimeout/setInterval just coerces it to 1, which we've been roughly following so far.

I previously had a PR for this at nodejs/node#6956 but I didn't really take it anywhere at the time.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

timers

hypesystem · 2017-09-19T18:32:25Z

Could you explain what you mean about "hitting the overflow"? I'm a bit confused by it.

Edit: as I understand the code, it means if a timeout or interval is given a number higher than the mac allowrd (32bit int?). It makes sense to warn when this happens, imo. But why does the limit exist?

Edit2: also, it seems that rn it is coerced to TIMEOUT_MAX, instead of 1 as in browsers (per your original message)?

addaleax

Looks good to me, but yeah, I wonder, what keeps us from supporting longer timeouts? libuv seems to support them?

Fishrock123 · 2017-09-19T18:35:39Z

@hypesystem see

ayo/lib/timers.js

Lines 46 to 47 in edc403c

    
           // Timeout values > TIMEOUT_MAX are set to 1. 
        
           const TIMEOUT_MAX = 2147483647; // 2^31-1

When a timer's duration is longer than that.

@addaleax in the OP:

While it may be viable to allow the use of 64bit numbers in enroll(), browser setTimeout/setInterval just coerces it to 1, which we've been roughly following so far.

hypesystem · 2017-09-19T18:37:18Z

@Fishrock123 realized I was a bit quick. Updated the original comment with better questions 😄

Fishrock123 · 2017-09-19T18:38:26Z

Edit2: also, it seems that rn it is coerced to TIMEOUT_MAX, instead of 1 as in browsers (per your original message)?

enroll() is usable separately from set{Timeout|Interval}(), which is where that particular line of code is from.

BridgeAR · 2017-09-20T01:50:36Z

@Fishrock123 why is the browser behavior the best though? Is it really necessary to follow that? I think it would be best to just increase the maximum instead.

jasnell · 2017-09-20T02:47:00Z

Is there any particular reason that nodejs/node#6956 wasn't pursued? It seemed like a reasonable change.

edited by @TimothyGu to fix link

Fishrock123 · 2017-09-20T14:47:50Z

why is the browser behavior the best though? Is it really necessary to follow that? I think it would be best to just increase the maximum instead.

Best? Not really.
Necessary to follow? Unsure, but that has been the historical precedent.

Consider that the maximum using 32 bits is already some ~26 days or so in length.

Is there any particular reason that nodejs/node#6956 wasn't pursued? It seemed like a reasonable change.

At the time, time&energy. Now-ish? Wanted to see how PRing to Ayo will be like. Also a part of me would kinda like to deal with less potential edge-case nonsense fallout since historically people just yell more when there are warnings, useful or not. ¯\_(ツ)_/¯

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: ayojs#71

Fishrock123 · 2017-09-20T15:10:54Z

It appears the linter run from travis didn't catch some long lines? Any idea why @addaleax?

addaleax · 2017-09-20T15:12:19Z

heh … I don’t think Travis is actually running the linter – my bad, sorry!

aqrln · 2017-09-20T18:24:24Z

lib/timers.js

 function createRepeatTimeout(callback, repeat, args) {
  repeat *= 1; // coalesce to number or NaN
-  if (!(repeat >= 1 && repeat <= TIMEOUT_MAX))
+  if (!(repeat >= 1 && repeat <= TIMEOUT_MAX)) {


Is it feasible to avoid duplicating code in createSingleTimeout() and createRepeatTimeout()?

Is the emitWarning call the duplicate code you're referring to?

The warning message differs slightly.

Yeah, that's what I was going to say. Doesn't seem worth abstracting, IMO.

Yup, sorry, didn't notice the difference in warning messages.

aqrln · 2017-09-20T18:25:28Z

test/parallel/test-timers-max-duration-warning.js

+
+const OVERFLOW = Math.pow(2, 31); // TIMEOUT_MAX is 2^31-1
+
+function TimerNotCanceled() {


I'd say this function's name should start with a lowercase letter.

It's be nice to have it run as a separate task, but I think this should work for now. Refs: ayojs/ayo#71

It'd be nice to have it run as a separate task, but I think this should work for now. Refs: ayojs#71

Refs: ayojs#71

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: ayojs#71

Divides the CI jobs by a custom `matrix:`, with a new linter job using an existing node.js binary to run `make lint-ci` (eslint). Refs: ayojs#71

Divides the CI jobs by a custom `matrix:`, with a new linter job using an existing node.js binary to run `make lint-ci` (eslint). Refs: #71 PR-URL: #75 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Fishrock123 · 2017-09-26T15:42:02Z

I suppose this should be land-able if it is something ya'll think would be good?

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: ayojs/ayo#71

Qard · 2017-09-26T16:49:10Z

Yeah, seems reasonable to me.

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: #71 Reviewed-By: Scott Trinh <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

addaleax · 2017-09-26T18:20:09Z

@Fishrock123 yup, thanks for the ping!

Landed in 607316c!

Cherry-pick from ayo Ayo commit log: > 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: ayojs/ayo#71 > Reviewed-By: Scott Trinh <[email protected]> > Reviewed-By: Alexey Orlenko <[email protected]> > Reviewed-By: Stephen Belanger <[email protected]> > Reviewed-By: Anna Henningsen <[email protected]> > Reviewed-By: Benjamin Gruenbaum <[email protected]>

Cherry-pick from ayo Ayo commit log: > 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: ayojs/ayo#71 > Reviewed-By: Scott Trinh <[email protected]> > Reviewed-By: Alexey Orlenko <[email protected]> > Reviewed-By: Stephen Belanger <[email protected]> > Reviewed-By: Anna Henningsen <[email protected]> > Reviewed-By: Benjamin Gruenbaum <[email protected]> PR-URL: #15627 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

addaleax approved these changes Sep 19, 2017

View reviewed changes

scotttrinh approved these changes Sep 19, 2017

View reviewed changes

benjamingr approved these changes Sep 20, 2017

View reviewed changes

Fishrock123 force-pushed the ayo-warn-on-timeout-overflow branch from b17d7b6 to 2045a3d Compare September 20, 2017 15:10

aqrln reviewed Sep 20, 2017

View reviewed changes

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 20, 2017

ci: run the linter from travis with every build

f7bfec8

It's be nice to have it run as a separate task, but I think this should work for now. Refs: ayojs/ayo#71

Fishrock123 mentioned this pull request Sep 20, 2017

ci: run the linter from travis with every build #75

Closed

4 tasks

Fishrock123 added a commit to Fishrock123/ayo that referenced this pull request Sep 20, 2017

ci: run the linter from travis with every build

25afd08

It'd be nice to have it run as a separate task, but I think this should work for now. Refs: ayojs#71

Fishrock123 added a commit to Fishrock123/ayo that referenced this pull request Sep 21, 2017

ci: attempt at separate linter CI job

183439e

Refs: ayojs#71

Fishrock123 added a commit to Fishrock123/ayo that referenced this pull request Sep 21, 2017

ci: attempt at separate linter CI job

50eb4a0

Refs: ayojs#71

timers: warn on overflowed timeout duration

7d216e4

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: ayojs#71

Fishrock123 force-pushed the ayo-warn-on-timeout-overflow branch from 2045a3d to 7d216e4 Compare September 21, 2017 16:19

Qard approved these changes Sep 21, 2017

View reviewed changes

aqrln approved these changes Sep 21, 2017

View reviewed changes

addaleax closed this Sep 26, 2017

jasnell mentioned this pull request Sep 26, 2017

timers: warn on overflowed timeout duration nodejs/node#15627

Closed

3 tasks


		const OVERFLOW = Math.pow(2, 31); // TIMEOUT_MAX is 2^31-1

		function TimerNotCanceled() {

timers: warn on overflowed timeout duration #71

timers: warn on overflowed timeout duration #71

Uh oh!

Conversation

Fishrock123 commented Sep 19, 2017

Checklist

Affected core subsystem(s)

Uh oh!

hypesystem commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hypesystem commented Sep 19, 2017

Uh oh!

Fishrock123 commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Sep 20, 2017

Uh oh!

jasnell commented Sep 20, 2017 • edited by TimothyGu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Sep 20, 2017

Uh oh!

Fishrock123 commented Sep 20, 2017

Uh oh!

addaleax commented Sep 20, 2017

Uh oh!

aqrln Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

scotttrinh Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

scotttrinh Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

aqrln Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

aqrln Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Sep 26, 2017

Uh oh!

Qard commented Sep 26, 2017

Uh oh!

addaleax commented Sep 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

hypesystem commented Sep 19, 2017 •

edited

Loading

Fishrock123 commented Sep 19, 2017 •

edited

Loading

Fishrock123 commented Sep 19, 2017 •

edited

Loading

jasnell commented Sep 20, 2017 •

edited by TimothyGu

Loading