timers: fix refresh for expired timers

Expired timers were not being refresh correctly and would always act as unrefed if refresh was called.
nodejs · apapirovski · Apr 22, 2019 · Apr 22, 2019 · May 31, 2019 · Dec 13, 2019
commit 33b3eb443faeb09ada5248a36eef057737b93c54
diff --git a/lib/internal/timers.js b/lib/internal/timers.js
@@ -207,23 +207,23 @@ Timeout.prototype.refresh = function() {
 };
 
 Timeout.prototype.unref = function() {
-  if (this[kRefed]) {
+  if (this[kRefed] && !this._destroyed) {
     this[kRefed] = false;
     decRefCount();
   }
   return this;
 };
 
 Timeout.prototype.ref = function() {
-  if (this[kRefed] === false) {
+  if (!this[kRefed] && !this._destroyed) {
     this[kRefed] = true;
     incRefCount();
   }
   return this;
 };
 
 Timeout.prototype.hasRef = function() {
-  return !!this[kRefed];
+  return !!(this[kRefed] && !this._destroyed);
 };
 
 function TimersList(expiry, msecs) {
@@ -317,12 +317,16 @@ function insertGuarded(item, refed, start) {
 
   insert(item, msecs, start);
 
-  if (!item[async_id_symbol] || item._destroyed) {
+  const isDestroyed = item._destroyed;
+  if (isDestroyed || !item[async_id_symbol]) {
     item._destroyed = false;
     initAsyncResource(item, 'Timeout');
   }
 
-  if (refed === !item[kRefed]) {
+  if (isDestroyed) {
+    if (refed)
+      incRefCount();
+  } else if (refed === !item[kRefed]) {
     if (refed)
       incRefCount();
     else
@@ -519,13 +523,14 @@ function getTimerCallbacks(runNextTicks) {
       const asyncId = timer[async_id_symbol];
 
       if (!timer._onTimeout) {
-        if (timer[kRefed])
-          refCount--;
-        timer[kRefed] = null;
-
-        if (destroyHooksExist() && !timer._destroyed) {
-          emitDestroy(asyncId);
+        if (!timer._destroyed) {
           timer._destroyed = true;
+
+          if (timer[kRefed])
+            refCount--;
+
+          if (destroyHooksExist())
+            emitDestroy(asyncId);
         }
         continue;
       }
@@ -546,15 +551,14 @@ function getTimerCallbacks(runNextTicks) {
         if (timer._repeat && timer._idleTimeout !== -1) {
           timer._idleTimeout = timer._repeat;
           insert(timer, timer._idleTimeout, start);
-        } else if (!timer._idleNext && !timer._idlePrev) {
+        } else if (!timer._idleNext && !timer._idlePrev && !timer._destroyed) {
+          timer._destroyed = true;
+
           if (timer[kRefed])
             refCount--;
-          timer[kRefed] = null;
 
-          if (destroyHooksExist() && !timer._destroyed) {
+          if (destroyHooksExist())
             emitDestroy(asyncId);
-          }
-          timer._destroyed = true;
         }
       }
 

diff --git a/lib/timers.js b/lib/timers.js
@@ -63,13 +63,14 @@ const {
 
 // Remove a timer. Cancels the timeout and resets the relevant timer properties.
 function unenroll(item) {
+  if (item._destroyed)
+    return;
+
+  item._destroyed = true;
+
   // Fewer checks may be possible, but these cover everything.
-  if (destroyHooksExist() &&
-      item[async_id_symbol] !== undefined &&
-      !item._destroyed) {
+  if (destroyHooksExist() && item[async_id_symbol] !== undefined)
     emitDestroy(item[async_id_symbol]);
-  }
-  item._destroyed = true;
 
   L.remove(item);
 
@@ -90,7 +91,6 @@ function unenroll(item) {
 
     decRefCount();
   }
-  item[kRefed] = null;
 
   // If active is called later, then we want to make sure not to insert again
   item._idleTimeout = -1;

diff --git a/test/parallel/test-timers-refresh.js b/test/parallel/test-timers-refresh.js
@@ -72,6 +72,20 @@ const { inspect } = require('util');
   strictEqual(timer.refresh(), timer);
 }
 
+// regular timer
+{
+  let called = false;
+  const timer = setTimeout(common.mustCall(() => {
+    if (!called) {
+      called = true;
+      process.nextTick(common.mustCall(() => {
+        timer.refresh();
+        strictEqual(timer.hasRef(), true);
+      }));
+    }
+  }, 2), 1);
+}
+
 // interval
 {
   let called = 0;