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: cleanup interval handling #1272

Closed
wants to merge 2 commits into from

Conversation

Fishrock123
Copy link
Contributor

Uses null as the false-y value for _repeat as like other properties.

Removes un-reachable statement in setInterval’s wrapper().

The path that clearing an interval takes is this:

As such, it will never be called. If someone were to manually set it in the callback, timer._repeat.call(this); would throw anyways.

R=@trevnorris / @bnoordhuis / @silverwind

Calling this.unref() during the callback of SetTimeout caused the
callback to get executed twice because unref() didn't expect to be
called during that time and did not stop the ref()ed Timeout but
did start a new timer. This commit prevents the new timer creation
when the callback was already called.

Fixes: nodejs#1191
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
PR_URL: nodejs#1231
@Fishrock123 Fishrock123 added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 26, 2015
@trevnorris
Copy link
Contributor

LGTM

Uses `null` as the false-y value for `_repeat` as like other properties.
Removes un-reachable statement in setInterval’s `wrapper()`.

PR-URL: nodejs#1272
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis
Copy link
Member

@Fishrock123
Copy link
Contributor Author

New windows errors appear to be from something in v1.x: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/376/

@Fishrock123
Copy link
Contributor Author

@bnoordhuis LGTY?

Edit: hmm, let me try this before some of those recent commits.

@Fishrock123
Copy link
Contributor Author

CI for this change against the last known 'green' base: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/378/

@Fishrock123
Copy link
Contributor Author

Hmmm. There is no way this change should effect this?

Edit: how in the hell is a timer firing 1.25ms early?

=== release test-timers-first-fire ===
Path: parallel/test-timers-first-fire
timer fired in -1.2554560000000023
assert.js:87
  throw new assert.AssertionError({
        ^
AssertionError: Timer fired early
    at null._onTimeout (c:\workspace\iojs+any-pr+multi\nodes\iojs-win2012r2\test\parallel\test-timers-first-fire.js:11:10)
    at Timer.listOnTimeout (timers.js:88:15)
Command: c:\workspace\iojs+any-pr+multi\nodes\iojs-win2012r2\Release\iojs.exe c:\workspace\iojs+any-pr+multi\nodes\iojs-win2012r2\test\parallel\test-timers-first-fire.js

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/378/nodes=iojs-win2012r2/console

@Arnavion
Copy link

uv__hrtime uses double arithmetic on the 64-bit int returned by QueryPerformanceCounter. hrtime_interval_ is a double calculated as 1.0 / QPFrequency. This double arithmetic is potentially the source of the inaccuracies.

QPC should only be used for calculating deltas, and these deltas should be calculated on the int64 itself. The value can be very large - QPC is allowed to overflow after 100 years (2^32 seconds) which means that an interval of a single second can contribute to 33 significant digits in the result.

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 26, 2015
Uses `null` as the false-y value for `_repeat` as like other properties.
Removes un-reachable statement in setInterval’s `wrapper()`.

PR-URL: nodejs#1272
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123
Copy link
Contributor Author

Thanks, landed in 776b73b (weird, it didn't see this as a merge.)

@trevnorris
Copy link
Contributor

@Fishrock123 don't think you pushed this up to iojs.

@trevnorris
Copy link
Contributor

Also that time is us, not ms, iirc.

@Fishrock123
Copy link
Contributor Author

It's pushed to iojs, but I guess it didn't push to my PR branch properly. Oh well.

@silverwind
Copy link
Contributor

The PR actually contains my previously amended commit, so that's probably the cause of confusion.

@Fishrock123
Copy link
Contributor Author

Yeah, it's just github being silly.

Fishrock123 added a commit to Fishrock123/libuv that referenced this pull request Mar 26, 2015
Eliminates floating-point operations that can cause precision loss.

Thanks to @Arnavion for suggesting the fix:
nodejs/node#1272 (comment)
Fishrock123 added a commit to Fishrock123/libuv that referenced this pull request Mar 26, 2015
Eliminates floating-point operations that can cause precision loss.

Thanks to @Arnavion for suggesting the fix:
nodejs/node#1272 (comment)
@rvagg rvagg mentioned this pull request Mar 28, 2015
@@ -271,8 +272,6 @@ exports.setInterval = function(callback, repeat) {

function wrapper() {
timer._repeat.call(this);
// If callback called clearInterval().
if (timer._repeat === null) return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mucked this up. Fedor is fixing it as part of #1330

Fishrock123 added a commit to Fishrock123/libuv that referenced this pull request Apr 10, 2015
Reduces floating-point operations that can cause precision loss.

Thanks to @Arnavion for suggesting the fix:
nodejs/node#1272 (comment)
Fishrock123 added a commit to Fishrock123/libuv that referenced this pull request Apr 10, 2015
Reduces floating-point operations that can cause precision loss.

Thanks to @Arnavion for suggesting the fix:
nodejs/node#1272 (comment)
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.

5 participants