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

timers: fix setInterval() assert #5102

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Member

bnoordhuis commented Mar 21, 2013

Test case:

var t = setInterval(function() {}, 1);
process.nextTick(t.unref);

Output:

Assertion failed: (args.Holder()->InternalFieldCount() > 0),
function Unref, file ../src/handle_wrap.cc, line 78.

setInterval() returns a binding layer object. Make it stop doing that,
wrap the raw process.binding('timer_wrap').Timer object in a Timeout
object.

Fixes #4261.

Reviewers: @isaacs @TooTallNate

timers: fix setInterval() assert
Test case:

  var t = setInterval(function() {}, 1);
  process.nextTick(t.unref);

Output:

  Assertion failed: (args.Holder()->InternalFieldCount() > 0),
  function Unref, file ../src/handle_wrap.cc, line 78.

setInterval() returns a binding layer object. Make it stop doing that,
wrap the raw process.binding('timer_wrap').Timer object in a Timeout
object.

Fixes #4261.

@bnoordhuis bnoordhuis closed this May 15, 2013

@bnoordhuis bnoordhuis reopened this May 15, 2013

Member

bnoordhuis commented May 15, 2013

@tjfontaine @trevnorris: Review please?

lgtm

@bnoordhuis bnoordhuis closed this May 15, 2013

Member

bnoordhuis commented May 15, 2013

Landed in 22533c0.

Hello.

We experience fail of this test for our platform and I am currently trying to fix it, but I need to understand what is the expected behavior of this test.

test/simple/test-timers-unref.js:60

process.nextTick(t.unref.bind({}));

You are calling unref which expects this to be Timeout object, but bind an empty object instead. Which brings us to a situation where delay and this._idleTimeout are NaN and undefined correspondingly.

  if (!this._handle) {
    var now = Date.now();
    if (!this._idleStart) this._idleStart = now;
    var delay = this._idleStart + this._idleTimeout - now;
    if (delay < 0) delay = 0;
    exports.unenroll(this);
    this._handle = new Timer();
    this._handle.ontimeout = this._onTimeout;
    this._handle.start(delay, 0);
    this._handle.domain = this.domain;
    this._handle.unref();
  } else {
    this._handle.unref();
  }

Could you please clarify?

Thanks in advance.

Member

bnoordhuis commented Aug 20, 2015

t.unref.bind({}) is nonsensical, it has no well-defined behavior, but it shouldn't SIGABRT the process.

I think it'd be alright to add instanceof checks in the right places in lib/timers.js. If you do, target https://github.com/nodejs/node - this repo is effectively deprecated.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.