Skip to content

Commit

Permalink
timers: assure setTimeout callback only runs once
Browse files Browse the repository at this point in the history
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: #1191
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
PR-URL: #1231
  • Loading branch information
silverwind committed Mar 26, 2015
1 parent 2ccc8f3 commit caf0b36
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 0 deletions.
7 changes: 7 additions & 0 deletions lib/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ function listOnTimeout() {
if (domain)
domain.enter();
threw = true;
first._called = true;
first._onTimeout();
if (domain)
domain.exit();
Expand Down Expand Up @@ -293,6 +294,7 @@ exports.clearInterval = function(timer) {


const Timeout = function(after) {
this._called = false;
this._idleTimeout = after;
this._idlePrev = this;
this._idleNext = this;
Expand All @@ -310,6 +312,10 @@ Timeout.prototype.unref = function() {
var delay = this._idleStart + this._idleTimeout - now;
if (delay < 0) delay = 0;
exports.unenroll(this);

// Prevent running cb again when unref() is called during the same cb
if (this._called && !this._repeat) return;

this._handle = new Timer();
this._handle[kOnTimeout] = this._onTimeout;
this._handle.start(delay, 0);
Expand Down Expand Up @@ -492,6 +498,7 @@ function unrefTimeout() {
if (domain) domain.enter();
threw = true;
debug('unreftimer firing timeout');
first._called = true;
first._onTimeout();
threw = false;
if (domain)
Expand Down
12 changes: 12 additions & 0 deletions test/parallel/test-timers-unref.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ var interval_fired = false,
timeout_fired = false,
unref_interval = false,
unref_timer = false,
unref_callbacks = 0,
interval, check_unref, checks = 0;

var LONG_TIME = 10 * 1000;
Expand Down Expand Up @@ -34,6 +35,16 @@ check_unref = setInterval(function() {
checks += 1;
}, 100);

setTimeout(function() {
unref_callbacks++;
this.unref();
}, SHORT_TIME);

// Should not timeout the test
setInterval(function() {
this.unref();
}, SHORT_TIME);

// Should not assert on args.Holder()->InternalFieldCount() > 0. See #4261.
(function() {
var t = setInterval(function() {}, 1);
Expand All @@ -46,4 +57,5 @@ process.on('exit', function() {
assert.strictEqual(timeout_fired, false, 'Timeout should not fire');
assert.strictEqual(unref_timer, true, 'An unrefd timeout should still fire');
assert.strictEqual(unref_interval, true, 'An unrefd interval should still fire');
assert.strictEqual(unref_callbacks, 1, 'Callback should only run once');
});

1 comment on commit caf0b36

@silverwind
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended PR_URL to PR-URL ;)

Please sign in to comment.