From 5bdb4018014bd0ca0bd160670e97dab4e4e8e1c4 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Wed, 15 Apr 2020 11:48:52 -0700 Subject: [PATCH] test: remove timers-blocking-callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the bug this test is intented to catch is reintroduced, or if 5aac4c42da104c30d8f701f1042d61c2f06b7e6c is effectively reverted, many (50+) tests time out, rendering this test redundant and unnecessary. in particular, the following timer tests catch an effective revert of 5aac4c42da104c30d8f701f1042d61c2f06b7e6c: not ok 21 parallel/test-timers-api-refs not ok 22 parallel/test-timers-args not ok 23 parallel/test-timers-destroyed not ok 25 parallel/test-timers-nested not ok 26 parallel/test-timers-interval-throw not ok 28 parallel/test-timers-non-integer-delay not ok 32 parallel/test-timers-ordering not ok 33 parallel/test-timers-refresh not ok 34 parallel/test-timers-refresh-in-callback not ok 35 parallel/test-timers-reset-process-domain-on-throw not ok 40 parallel/test-timers-timeout-to-interval not ok 41 parallel/test-timers-uncaught-exception not ok 42 parallel/test-timers-timeout-with-non-integer not ok 43 parallel/test-timers-unenroll-unref-interval not ok 44 parallel/test-timers-unref not ok 45 parallel/test-timers-unref-active not ok 46 parallel/test-timers-unrefd-interval-still-fires not ok 47 parallel/test-timers-unrefed-in-callback not ok 48 parallel/test-timers-user-call not ok 49 parallel/test-timers-zero-timeout Refs: https://github.com/nodejs/node/issues/21781 PR-URL: https://github.com/nodejs/node/pull/32870 Reviewed-By: Anatoli Papirovski Reviewed-By: Michaël Zasso Reviewed-By: Rich Trott --- test/sequential/sequential.status | 2 - .../test-timers-blocking-callback.js | 114 ------------------ 2 files changed, 116 deletions(-) delete mode 100644 test/sequential/test-timers-blocking-callback.js diff --git a/test/sequential/sequential.status b/test/sequential/sequential.status index f73b428de5f390..fce8bd959f0326 100644 --- a/test/sequential/sequential.status +++ b/test/sequential/sequential.status @@ -17,8 +17,6 @@ test-worker-prof: PASS, FLAKY [$system==linux] [$system==macos] -# https://github.com/nodejs/node/issues/21781 -test-timers-blocking-callback: PASS, FLAKY [$system==solaris] # Also applies to SmartOS diff --git a/test/sequential/test-timers-blocking-callback.js b/test/sequential/test-timers-blocking-callback.js deleted file mode 100644 index a5e0f596a34b93..00000000000000 --- a/test/sequential/test-timers-blocking-callback.js +++ /dev/null @@ -1,114 +0,0 @@ -// Flags: --expose-internals -'use strict'; - -/* - * This is a regression test for - * https://github.com/nodejs/node-v0.x-archive/issues/15447 and - * https://github.com/nodejs/node-v0.x-archive/issues/9333. - * - * When a timer is added in another timer's callback, its underlying timer - * handle was started with a timeout that was actually incorrect. - * - * The reason was that the value that represents the current time was not - * updated between the time the original callback was called and the time - * the added timer was processed by timers.listOnTimeout. That led the - * logic in timers.listOnTimeout to do an incorrect computation that made - * the added timer fire with a timeout of scheduledTimeout + - * timeSpentInCallback. - * - * This test makes sure that a timer added by another timer's callback - * fires with the expected timeout. - * - * It makes sure that it works when the timers list for a given timeout is - * empty (see testAddingTimerToEmptyTimersList) and when the timers list - * is not empty (see testAddingTimerToNonEmptyTimersList). - */ - -const common = require('../common'); -const assert = require('assert'); -const { sleep } = require('internal/util'); - -const TIMEOUT = 100; - -let nbBlockingCallbackCalls; -let latestDelay; -let timeCallbackScheduled; - -// These tests are timing dependent so they may fail even when the bug is -// not present (if the host is sufficiently busy that the timers are delayed -// significantly). However, they fail 100% of the time when the bug *is* -// present, so to increase reliability, allow for a small number of retries. -let retries = 2; - -function initTest() { - nbBlockingCallbackCalls = 0; - latestDelay = 0; - timeCallbackScheduled = 0; -} - -function blockingCallback(retry, callback) { - ++nbBlockingCallbackCalls; - - if (nbBlockingCallbackCalls > 1) { - latestDelay = Date.now() - timeCallbackScheduled; - // Even if timers can fire later than when they've been scheduled - // to fire, they shouldn't generally be more than 100% late in this case. - // But they are guaranteed to be at least 100ms late given the bug in - // https://github.com/nodejs/node-v0.x-archive/issues/15447 and - // https://github.com/nodejs/node-v0.x-archive/issues/9333. - if (latestDelay >= TIMEOUT * 2) { - if (retries > 0) { - retries--; - return retry(callback); - } - assert.fail(`timeout delayed by more than 100% (${latestDelay}ms)`); - } - if (callback) - return callback(); - } else { - // Block by busy-looping to trigger the issue - sleep(TIMEOUT); - - timeCallbackScheduled = Date.now(); - setTimeout(blockingCallback.bind(null, retry, callback), TIMEOUT); - } -} - -function testAddingTimerToEmptyTimersList(callback) { - initTest(); - // Call setTimeout just once to make sure the timers list is - // empty when blockingCallback is called. - setTimeout( - blockingCallback.bind(null, testAddingTimerToEmptyTimersList, callback), - TIMEOUT - ); -} - -function testAddingTimerToNonEmptyTimersList() { - // If both timers fail and attempt a retry, only actually do anything for one - // of them. - let retryOK = true; - const retry = () => { - if (retryOK) - testAddingTimerToNonEmptyTimersList(); - retryOK = false; - }; - - initTest(); - // Call setTimeout twice with the same timeout to make - // sure the timers list is not empty when blockingCallback is called. - setTimeout( - blockingCallback.bind(null, retry), - TIMEOUT - ); - setTimeout( - blockingCallback.bind(null, retry), - TIMEOUT - ); -} - -// Run the test for the empty timers list case, and then for the non-empty -// timers list one. -testAddingTimerToEmptyTimersList( - common.mustCall(testAddingTimerToNonEmptyTimersList) -);