timers: assure setTimeout callback only runs once

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 <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> PR-URL: #1231
nodejs · Mar 26, 2015 · caf0b36 · caf0b36 · silverwind · Mar 26, 2015
1 parent 2ccc8f3
commit caf0b36
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 0 deletions.
diff --git a/lib/timers.js b/lib/timers.js
@@ -85,6 +85,7 @@ function listOnTimeout() {
         if (domain)
           domain.enter();
         threw = true;
+        first._called = true;
         first._onTimeout();
         if (domain)
           domain.exit();
@@ -293,6 +294,7 @@ exports.clearInterval = function(timer) {
 
 
 const Timeout = function(after) {
+  this._called = false;
   this._idleTimeout = after;
   this._idlePrev = this;
   this._idleNext = this;
@@ -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);
@@ -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)

diff --git a/test/parallel/test-timers-unref.js b/test/parallel/test-timers-unref.js
@@ -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;
@@ -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);
@@ -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');
 });