Fix timer .unref() handle management once and for all

lib/timers.js

@@ -157,15 +157,8 @@ exports.enroll = function(item, msecs) {
 // it will reset its timeout.
 exports.active = function(item) {
   var msecs = item._idleTimeout;
-  if (msecs >= 0) {
-    var list = lists[msecs];
-    if (!list || L.isEmpty(list)) {
-      insert(item, msecs);
-    } else {
-      item._idleStart = Timer.now();
-      L.append(list, item);
-    }
-  }
+  if (msecs >= 0)
+    insert(item, msecs);
 };
 
 
@@ -272,6 +265,11 @@ exports.setInterval = function(callback, repeat) {
 
   function wrapper() {
     timer._repeat.call(this);
+
+    // Timer might be closed - no point in restarting it
+    if (!timer._repeat)
+      return;
+
     // If timer is unref'd (or was - it's permanently removed from the list.)
     if (this._handle) {
       this._handle.start(repeat, 0);
@@ -301,6 +299,14 @@ const Timeout = function(after) {
   this._repeat = null;
 };
 
+
+function unrefdHandle() {
+  this.owner._onTimeout();
+  if (!this.owner._repeat)
+    this.owner.close();
+}
+
+
 Timeout.prototype.unref = function() {
   if (this._handle) {
     this._handle.unref();
@@ -315,7 +321,8 @@ Timeout.prototype.unref = function() {
     if (this._called && !this._repeat) return;
 
     this._handle = new Timer();
-    this._handle[kOnTimeout] = this._onTimeout;
+    this._handle.owner = this;
+    this._handle[kOnTimeout] = unrefdHandle;
     this._handle.start(delay, 0);
     this._handle.domain = this.domain;
     this._handle.unref();

src/timer_wrap.cc

@@ -73,6 +73,8 @@ class TimerWrap : public HandleWrap {
   static void Start(const FunctionCallbackInfo<Value>& args) {
     TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder());
 
+    CHECK(HandleWrap::IsAlive(wrap));
+
     int64_t timeout = args[0]->IntegerValue();
     int64_t repeat = args[1]->IntegerValue();
     int err = uv_timer_start(&wrap->handle_, OnTimeout, timeout, repeat);
@@ -82,20 +84,26 @@ class TimerWrap : public HandleWrap {
   static void Stop(const FunctionCallbackInfo<Value>& args) {
     TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder());
 
+    CHECK(HandleWrap::IsAlive(wrap));
+
     int err = uv_timer_stop(&wrap->handle_);
     args.GetReturnValue().Set(err);
   }
 
   static void Again(const FunctionCallbackInfo<Value>& args) {
     TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder());
 
+    CHECK(HandleWrap::IsAlive(wrap));
+
     int err = uv_timer_again(&wrap->handle_);
     args.GetReturnValue().Set(err);
   }
 
   static void SetRepeat(const FunctionCallbackInfo<Value>& args) {
     TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder());
 
+    CHECK(HandleWrap::IsAlive(wrap));
+
     int64_t repeat = args[0]->IntegerValue();
     uv_timer_set_repeat(&wrap->handle_, repeat);
     args.GetReturnValue().Set(0);
@@ -104,6 +112,8 @@ class TimerWrap : public HandleWrap {
   static void GetRepeat(const FunctionCallbackInfo<Value>& args) {
     TimerWrap* wrap = Unwrap<TimerWrap>(args.Holder());
 
+    CHECK(HandleWrap::IsAlive(wrap));
+
     int64_t repeat = uv_timer_get_repeat(&wrap->handle_);
     if (repeat <= 0xfffffff)
       args.GetReturnValue().Set(static_cast<uint32_t>(repeat));

test/parallel/test-timers-unref-leak.js

@@ -0,0 +1,25 @@
+var assert = require('assert');
+
+var called = 0;
+var closed = 0;
+
+var timeout = setTimeout(function() {
+  called++;
+}, 10);
+timeout.unref();
+
+// Wrap `close` method to check if the handle was closed
+var close = timeout._handle.close;
+timeout._handle.close = function() {
+  closed++;
+  return close.apply(this, arguments);
+};
+
+// Just to keep process alive and let previous timer's handle die
+setTimeout(function() {
+}, 50);
+
+process.on('exit', function() {
+  assert.equal(called, 1);
+  assert.equal(closed, 1);
+});

test/parallel/test-timers-unrefd-interval-still-fires.js

@@ -0,0 +1,18 @@
+/*
+ * This test is a regression test for joyent/node#8900.
+ */
+var assert = require('assert');
+
+var N = 5;
+var nbIntervalFired = 0;
+var timer = setInterval(function() {
+  ++nbIntervalFired;
+  if (nbIntervalFired === N)
+    clearInterval(timer);
+}, 1);
+
+timer.unref();
+
+setTimeout(function onTimeout() {
+  assert.strictEqual(nbIntervalFired, N);
+}, 100);