timers: minor _unrefActive fixes and improvements

This commit addresses most of the review comments in
https://github.com/nodejs/node/pull/2540, which are kept in this
separate commit so as to better preserve the prior two patches as they
landed in 0.12.

This commit:
- Fixes a bug with unrefActive timers and disposed domains.
- Fixes a bug with unrolling an unrefActive timer from another.
- Adds a test for both above bugs.
- Improves check logic, making it stricter, simpler, or both.
- Optimizes nicer with a smaller, separate function for the try/catch.

Fixes: https://github.com/nodejs/node-convergence-archive/issues/23
Ref: https://github.com/nodejs/node/issues/268
PR-URL: https://github.com/nodejs/node/pull/2540
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

diff --git a/lib/timers.js b/lib/timers.js
index 82ac156..e3d0eb1 100644 (file)
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -481,31 +481,36 @@ function _makeTimerTimeout(timer) {
   var domain = timer.domain;
   var msecs = timer._idleTimeout;
 
+  L.remove(timer);
+
   // Timer has been unenrolled by another timer that fired at the same time,
   // so don't make it timeout.
-  if (!msecs || msecs < 0)
+  if (msecs <= 0)
     return;
 
   if (!timer._onTimeout)
     return;
 
-  if (domain && domain._disposed)
-    return;
+  if (domain) {
+    if (domain._disposed)
+      return;
 
-  try {
-    var threw = true;
+    domain.enter();
+  }
 
-    if (domain) domain.enter();
+  debug('unreftimer firing timeout');
+  timer._called = true;
+  _runOnTimeout(timer);
 
-    debug('unreftimer firing timeout');
-    L.remove(timer);
-    timer._called = true;
-    timer._onTimeout();
+  if (domain)
+    domain.exit();
+}
 
+function _runOnTimeout(timer) {
+  var threw = true;
+  try {
+    timer._onTimeout();
     threw = false;
-
-    if (domain)
-      domain.exit();
   } finally {
     if (threw) process.nextTick(unrefTimeout);
   }
@@ -519,7 +524,7 @@ function unrefTimeout() {
   var timeSinceLastActive;
   var nextTimeoutTime;
   var nextTimeoutDuration;
-  var minNextTimeoutTime;
+  var minNextTimeoutTime = TIMEOUT_MAX;
   var timersToTimeout = [];
 
   // The actual timer fired and has not yet been rearmed,
@@ -534,7 +539,7 @@ function unrefTimeout() {
   // and rearm the actual timer if the next timeout to expire
   // will expire before the current actual timer.
   var cur = unrefList._idlePrev;
-  while (cur != unrefList) {
+  while (cur !== unrefList) {
     timeSinceLastActive = now - cur._idleStart;
 
     if (timeSinceLastActive < cur._idleTimeout) {
@@ -543,7 +548,7 @@ function unrefTimeout() {
 
       nextTimeoutDuration = cur._idleTimeout - timeSinceLastActive;
       nextTimeoutTime = now + nextTimeoutDuration;
-      if (minNextTimeoutTime == null ||
+      if (minNextTimeoutTime === TIMEOUT_MAX ||
           (nextTimeoutTime < minNextTimeoutTime)) {
         // We found a timeout that will expire earlier,
         // store its next timeout time now so that we
@@ -569,7 +574,7 @@ function unrefTimeout() {
 
   // Rearm the actual timer with the timeout delay
   // of the earliest timeout found.
-  if (minNextTimeoutTime != null) {
+  if (minNextTimeoutTime !== TIMEOUT_MAX) {
     unrefTimer.start(minNextTimeoutTime - now, 0);
     unrefTimer.when = minNextTimeoutTime;
     debug('unrefTimer rescheduled');

diff --git a/test/parallel/test-timers-unref-active-unenrolled-disposed.js b/test/parallel/test-timers-unref-active-unenrolled-disposed.js
new file mode 100644 (file)
index 0000000..ac22cd6
--- /dev/null
+++ b/test/parallel/test-timers-unref-active-unenrolled-disposed.js
@@ -0,0 +1,46 @@
+'use strict';
+
+// https://github.com/nodejs/node/pull/2540/files#r38231197
+
+const common = require('../common');
+const timers = require('timers');
+const assert = require('assert');
+const domain = require('domain');
+
+// Crazy stuff to keep the process open,
+// then close it when we are actually done.
+const TEST_DURATION = common.platformTimeout(100);
+const keepOpen = setTimeout(function() {
+  throw new Error('Test timed out. keepOpen was not canceled.');
+}, TEST_DURATION);
+
+const endTest = makeTimer(2);
+
+const someTimer = makeTimer(1);
+someTimer.domain = domain.create();
+someTimer.domain.dispose();
+someTimer._onTimeout = function() {
+  throw new Error('someTimer was not supposed to fire!');
+};
+
+endTest._onTimeout = common.mustCall(function() {
+  assert.strictEqual(someTimer._idlePrev, null);
+  assert.strictEqual(someTimer._idleNext, null);
+  clearTimeout(keepOpen);
+});
+
+const cancelsTimer = makeTimer(1);
+cancelsTimer._onTimeout = common.mustCall(function() {
+  someTimer._idleTimeout = 0;
+});
+
+timers._unrefActive(cancelsTimer);
+timers._unrefActive(someTimer);
+timers._unrefActive(endTest);
+
+function makeTimer(msecs) {
+  const timer = {};
+  timers.unenroll(timer);
+  timers.enroll(timer, msecs);
+  return timer;
+}