test: fix timers-same-timeout-wrong-list-deleted

test-timers-same-timeout-wrong-list-deleted was flaky under load because
there is no guarantee that a timer will fire within a given period of
time. It had an exit handler that checked that the process was finishing
in less than twice as much as a timer was set for. Under load, the
timer could take over 200ms to fire even if it was set for 100ms, so
this was causing the test to be flaky on CI from time to time.

However, that timing check is unnecessary to identify the regression
that the test was written for. When run with a version of Node.js that
does not contain the fix that accompanied the test in its initial
commit, an assertion indicating that there were still timers in the
active timer list fired. So, this commit removes the exit handler timing
check and relies on the existing robust active timers list length check.

This allows us to move the test back to parallel because it does not
seem to fail under load anymore.

The test was refactored slightly, removing duplicated code to a
function, using `assert.strictEqual()` instead of `assert.equal()`,
changing a 10ms timer to 1ms, and improving the messages provided by
assertions.

Fixes: https://github.com/nodejs/node/issues/8459
PR-URL: https://github.com/nodejs/node/pull/10362
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

1 files changed, 72 insertions, 0 deletions

diff --git a/test/parallel/test-timers-same-timeout-wrong-list-deleted.js b/test/parallel/test-timers-same-timeout-wrong-list-deleted.js
new file mode 100644
index 0000000000..8a622b32e4
--- /dev/null
+++ b/test/parallel/test-timers-same-timeout-wrong-list-deleted.js
@@ -0,0 +1,72 @@
+'use strict';
+
+/*
+ * This is a regression test for https://github.com/nodejs/node/issues/7722.
+ *
+ * When nested timers have the same timeout, calling clearTimeout on the
+ * older timer after it has fired causes the list the newer timer is in
+ * to be deleted. Since the newer timer was not cleared, it still blocks
+ * the event loop completing for the duration of its timeout, however, since
+ * no reference exists to it in its list, it cannot be canceled and its
+ * callback is not called when the timeout elapses.
+ */
+
+const common = require('../common');
+const assert = require('assert');
+const Timer = process.binding('timer_wrap').Timer;
+
+const TIMEOUT = common.platformTimeout(100);
+
+const handle1 = setTimeout(common.mustCall(function() {
+  // Cause the old TIMEOUT list to be deleted
+  clearTimeout(handle1);
+
+  // Cause a new list with the same key (TIMEOUT) to be created for this timer
+  const handle2 = setTimeout(function() {
+    common.fail('Inner callback is not called');
+  }, TIMEOUT);
+
+  setTimeout(common.mustCall(function() {
+    // Attempt to cancel the second timer. Fix for this bug will keep the
+    // newer timer from being dereferenced by keeping its list from being
+    // erroneously deleted. If we are able to cancel the timer successfully,
+    // the bug is fixed.
+    clearTimeout(handle2);
+
+    setImmediate(common.mustCall(function() {
+      setImmediate(common.mustCall(function() {
+        const activeTimers = getActiveTimers();
+
+        // Make sure our clearTimeout succeeded. One timer finished and
+        // the other was canceled, so none should be active.
+        assert.strictEqual(activeTimers.length, 0, 'Timers remain.');
+      }));
+    }));
+  }), 1);
+
+  // Make sure our timers got added to the list.
+  const activeTimers = getActiveTimers();
+  const shortTimer = activeTimers.find(function(handle) {
+    return handle._list.msecs === 1;
+  });
+  const longTimers = activeTimers.filter(function(handle) {
+    return handle._list.msecs === TIMEOUT;
+  });
+
+  // Make sure our clearTimeout succeeded. One timer finished and
+  // the other was canceled, so none should be active.
+  assert.strictEqual(activeTimers.length, 3,
+                     'There should be 3 timers in the list.');
+  assert(shortTimer instanceof Timer, 'The shorter timer is not in the list.');
+  assert.strictEqual(longTimers.length, 2,
+                     'Both longer timers should be in the list.');
+
+  // When this callback completes, `listOnTimeout` should now look at the
+  // correct list and refrain from removing the new TIMEOUT list which
+  // contains the reference to the newer timer.
+}), TIMEOUT);
+
+function getActiveTimers() {
+  const activeHandles = process._getActiveHandles();
+  return activeHandles.filter((handle) => handle instanceof Timer);
+}