Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

timers: fix setInterval() assert #5102

Closed
wants to merge 1 commit into from

2 participants

@bnoordhuis

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

@bnoordhuis bnoordhuis 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.
0775c12
@bnoordhuis bnoordhuis closed this
@bnoordhuis bnoordhuis reopened this
@tjfontaine
Owner

lgtm

@bnoordhuis bnoordhuis closed this
@bnoordhuis

Landed in 22533c0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 13, 2013
  1. @bnoordhuis

    timers: fix setInterval() assert

    bnoordhuis authored
    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.
This page is out of date. Refresh to see the latest.
Showing with 32 additions and 12 deletions.
  1. +25 −12 lib/timers.js
  2. +7 −0 test/simple/test-timers-unref.js
View
37 lib/timers.js
@@ -222,7 +222,7 @@ exports.setTimeout = function(callback, after) {
exports.clearTimeout = function(timer) {
if (timer && (timer.ontimeout || timer._onTimeout)) {
timer.ontimeout = timer._onTimeout = null;
- if (timer instanceof Timer || timer instanceof Timeout) {
+ if (timer instanceof Timeout) {
timer.close(); // for after === 0
} else {
exports.unenroll(timer);
@@ -232,39 +232,52 @@ exports.clearTimeout = function(timer) {
exports.setInterval = function(callback, repeat) {
- var timer = new Timer();
-
- if (process.domain) timer.domain = process.domain;
-
repeat *= 1; // coalesce to number or NaN
if (!(repeat >= 1 && repeat <= TIMEOUT_MAX)) {
repeat = 1; // schedule on next tick, follows browser behaviour
}
+ var timer = new Timeout(repeat);
var args = Array.prototype.slice.call(arguments, 2);
- timer.ontimeout = function() {
- callback.apply(timer, args);
- }
+ timer._onTimeout = wrapper;
+ timer._repeat = true;
+
+ if (process.domain) timer.domain = process.domain;
+ exports.active(timer);
- timer.start(repeat, repeat);
return timer;
+
+ function wrapper() {
+ callback.apply(this, args);
+ // If callback called clearInterval().
+ if (timer._repeat === false) return;
+ // If timer is unref'd (or was - it's permanently removed from the list.)
+ if (this._handle) {
+ this._handle.start(repeat, 0);
+ } else {
+ timer._idleTimeout = repeat;
+ exports.active(timer);
+ }
+ }
};
exports.clearInterval = function(timer) {
- if (timer instanceof Timer) {
- timer.ontimeout = null;
- timer.close();
+ if (timer && timer._repeat) {
+ timer._repeat = false;
+ clearTimeout(timer);
}
};
+
var Timeout = function(after) {
this._idleTimeout = after;
this._idlePrev = this;
this._idleNext = this;
this._idleStart = null;
this._onTimeout = null;
+ this._repeat = false;
};
Timeout.prototype.unref = function() {
View
7 test/simple/test-timers-unref.js
@@ -54,6 +54,13 @@ check_unref = setInterval(function() {
checks += 1;
}, 100);
+// Should not assert on args.Holder()->InternalFieldCount() > 0. See #4261.
+(function() {
+ var t = setInterval(function() {}, 1);
+ process.nextTick(t.unref.bind({}));
+ process.nextTick(t.unref.bind(t));
+})();
+
process.on('exit', function() {
assert.strictEqual(interval_fired, false, 'Interval should not fire');
assert.strictEqual(timeout_fired, false, 'Timeout should not fire');
Something went wrong with that request. Please try again.