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

Timers can false start 1ms earlier than needed #4194

Closed
wants to merge 1 commit into from
Closed

Timers can false start 1ms earlier than needed #4194

wants to merge 1 commit into from

Conversation

AlexeyKupershtokh
Copy link

( Related issue: #534 )

@bnoordhuis , you actually introduced another bug in 82e9da9

Lets say:

var _idleStart = 0;
var msecs = 1000;
var now = 999;
var diff = now - _idleStart;
console.log('diff', diff);
if (diff + 1 < msecs) {
  console.log('schedule for', msecs - diff, 'ms more');
} else {
  console.log('immediate exec');
}

Outputs

diff 999
immediate exec

which is definitely wrong.

There is a complete test for this bug. You can include into node/test/simple:

var common = require('../common');
var assert = require('assert');
var i;

var N = 30;

var last_i = 0;
var last_ts = 0;
var start = Date.now();

var f = function(i) {
  if (i <= N) {
    // check order
    assert(i == last_i + 1, 'iteration order is broken on ' + i + 'th iteration, prev is ' + last_i);
    last_i = i;

    // check that this iteration has been fired at leat 1ms later than the previous
    var now = Date.now();
    console.log(i, now);
    assert(now >= last_ts + 1, 'iteration has been fired within the same ms as the previous on ' + i + 'th iteration');
    last_ts = now;

    // schedule next iteration
    setTimeout(f, 1, i + 1);
  }
}
f(1);

it fails on the current master:

1 1351075317097
2 1351075317103
3 1351075317106
4 1351075317106

timers.js:102
            if (!process.listeners('uncaughtException').length) throw e;
                                                                      ^
AssertionError: iteration has been fired within the same ms as the previous on 4th iteration
    at f (/home/wicked/Alawar/node/test/simple/test-next-tick-ordering3.js:41:5)
    at exports.setTimeout.timer._onTimeout (timers.js:195:16)
    at Timer.list.ontimeout (timers.js:100:19)
    at process.startup.processMakeCallback.process._makeCallback (node.js:248:20)

If I revert the 82e9da9 , the test passes.

@bnoordhuis
Copy link
Member

@bnoordhuis , you actually introduced another bug in 82e9da9

You'll note that I'm not @ry. I'm a lot taller and more handsome to boot.

Can you send a PR for this? Reverting 82e9da9 seems reasonable enough and a quick test indicates that it works.

@Nodejs-Jenkins
Copy link

Can one of the admins verify this patch?

@isaacs
Copy link

isaacs commented Mar 24, 2013

Landed on 9fae4dc (rebased to current v0.10 code).

Thanks!

@isaacs isaacs closed this Mar 24, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants