From efbed9cf09ca46a4e83f3fb3053a6d64aef9d080 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 20 Dec 2016 13:38:22 -0800 Subject: [PATCH] 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 Reviewed-By: James M Snell --- ...-timers-same-timeout-wrong-list-deleted.js | 40 ++++++++----------- 1 file changed, 16 insertions(+), 24 deletions(-) rename test/{sequential => parallel}/test-timers-same-timeout-wrong-list-deleted.js (68%) diff --git a/test/sequential/test-timers-same-timeout-wrong-list-deleted.js b/test/parallel/test-timers-same-timeout-wrong-list-deleted.js similarity index 68% rename from test/sequential/test-timers-same-timeout-wrong-list-deleted.js rename to test/parallel/test-timers-same-timeout-wrong-list-deleted.js index 05c0233e124b83..8a622b32e434af 100644 --- a/test/sequential/test-timers-same-timeout-wrong-list-deleted.js +++ b/test/parallel/test-timers-same-timeout-wrong-list-deleted.js @@ -16,16 +16,6 @@ const assert = require('assert'); const Timer = process.binding('timer_wrap').Timer; const TIMEOUT = common.platformTimeout(100); -const start = Timer.now(); - -// This bug also prevents the erroneously dereferenced timer's callback -// from being called, so we can't use it's execution or lack thereof -// to assert that the bug is fixed. -process.on('exit', function() { - const end = Timer.now(); - assert.equal(end - start < TIMEOUT * 2, true, - 'Elapsed time does not include second timer\'s timeout.'); -}); const handle1 = setTimeout(common.mustCall(function() { // Cause the old TIMEOUT list to be deleted @@ -42,27 +32,22 @@ const handle1 = setTimeout(common.mustCall(function() { // 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 activeHandles = process._getActiveHandles(); - const activeTimers = activeHandles.filter(function(handle) { - return handle instanceof Timer; - }); + const activeTimers = getActiveTimers(); // Make sure our clearTimeout succeeded. One timer finished and // the other was canceled, so none should be active. - assert.equal(activeTimers.length, 0, 'No Timers remain.'); + assert.strictEqual(activeTimers.length, 0, 'Timers remain.'); })); })); - }), 10); + }), 1); // Make sure our timers got added to the list. - const activeHandles = process._getActiveHandles(); - const activeTimers = activeHandles.filter(function(handle) { - return handle instanceof Timer; - }); + const activeTimers = getActiveTimers(); const shortTimer = activeTimers.find(function(handle) { - return handle._list.msecs === 10; + return handle._list.msecs === 1; }); const longTimers = activeTimers.filter(function(handle) { return handle._list.msecs === TIMEOUT; @@ -70,11 +55,18 @@ const handle1 = setTimeout(common.mustCall(function() { // Make sure our clearTimeout succeeded. One timer finished and // the other was canceled, so none should be active. - assert.equal(activeTimers.length, 3, 'There are 3 timers in the list.'); - assert(shortTimer instanceof Timer, 'The shorter timer is in the list.'); - assert.equal(longTimers.length, 2, 'Both longer timers are in the list.'); + 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); +}