This repository has been archived by the owner. It is now read-only.

Timers can false start 1ms earlier than needed #4194

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@AlexeyKupershtokh

( Related issue: joyent#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

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Oct 26, 2012

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.

Member

bnoordhuis commented Oct 26, 2012

@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

This comment has been minimized.

Show comment Hide comment
@Nodejs-Jenkins

Nodejs-Jenkins Mar 13, 2013

Can one of the admins verify this patch?

Can one of the admins verify this patch?

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Mar 24, 2013

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

Thanks!

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.