From ac9f6f7f8a264b52b2c0631a496558e4e34bda72 Mon Sep 17 00:00:00 2001 From: Chris Vest Date: Sun, 30 Jun 2024 16:52:00 -0700 Subject: [PATCH] Fix test flakiness The test counting allocators use synchronized lists, but their iterators are not thread-safe. --- .../java/blackbox/AllocatorBasedPoolTest.java | 79 +++++++++++-------- .../java/blackbox/ThreadBasedPoolTest.java | 45 ++++++----- src/test/java/blackbox/slow/PoolIT.java | 5 +- .../java/blackbox/slow/ThreadBasedPoolIT.java | 5 +- 4 files changed, 79 insertions(+), 55 deletions(-) diff --git a/src/test/java/blackbox/AllocatorBasedPoolTest.java b/src/test/java/blackbox/AllocatorBasedPoolTest.java index 63161ba3..a9083b60 100644 --- a/src/test/java/blackbox/AllocatorBasedPoolTest.java +++ b/src/test/java/blackbox/AllocatorBasedPoolTest.java @@ -265,10 +265,10 @@ void mustUseProvidedExpiration(Taps taps) throws Exception { /** * In the hopefully unlikely event that an Expiration throws an * exception, that exception should bubble out of the pool unspoiled. - * + *

* We test for this by configuring an Expiration that always throws. * No guarantees are being made about when, exactly, it is that the pool will - * invoke the Expiration. Therefore we claim and release an object a + * invoke the Expiration. Therefore, we claim and release an object a * couple of times. That ought to do it. */ @ParameterizedTest @@ -290,7 +290,7 @@ void exceptionsFromExpirationMustBubbleOut(Taps taps) { /** * If the Expiration throws an exception when evaluating a slot, then that * slot should be considered invalid. - * + *

* We test for this by configuring an expiration that always throws, * and then we make a claim and a release to make sure that it got invoked. * Then, since the pool size is one, we make another claim to make sure that @@ -518,7 +518,7 @@ void slotInfoAgeMustResetAfterReallocation(Taps taps) throws InterruptedExceptio * they must be replaced/renewed. Pools should generally try to renew * before the timeout elapses for the given object, but we don't test for * that here. - * We set the TTL to be 1 milliseconds, because that is short enough that + * We set the TTL to be 1 millisecond, because that is short enough that * we can wait for it in a spin-loop. This way, the objects will always * appear to have expired when checked. This means that every claim will * always allocate a new object, and so our two claims will translate to @@ -670,8 +670,10 @@ void mustNotDeallocateTheSameObjectMoreThanOnce(Taps taps) throws Exception { tap.claim(longTimeout).release(); // check if the deallocation list contains duplicates List deallocations = allocator.getDeallocations(); - for (Poolable elm : deallocations) { - assertThat(Collections.frequency(deallocations, elm)).as("Deallocations of %s", elm).isEqualTo(1); + synchronized (deallocations) { + for (Poolable elm : deallocations) { + assertThat(Collections.frequency(deallocations, elm)).as("Deallocations of %s", elm).isEqualTo(1); + } } } @@ -980,7 +982,7 @@ void claimMustThrowIfReallocationReturnsNull(Taps taps) throws Exception { * start. And one that can be generally tested for across pool * implementations. Chances are, that if a pool handles this specific case, * then it handles all cases that are relevant to its implementation. - * + *

* We test for this by depleting a big pool with a very short TTL. After the * pool has been depleted, the allocator will no longer create any more * objects, so no expired objects will be renewed. Then we try to claim one @@ -1087,24 +1089,26 @@ void mustBeAbleToShutDownWhenAllocateAlwaysThrows() throws Exception { /** * Here's the scenario we're trying to target: - * - * - You (or your thread) do a successful claim, and triumphantly stores it - * in the ThreadLocal cache. - * - You then return the object after use, so now it's back in the - * live-queue for others to grab. - * - Someone else claims the object, and explicitly expires the object. - * - You want to claim an object again, and start by looking in the - * ThreadLocal cache. - * - You find the slot for the object you had last, but the slot is poisoned - * with the explicit expiration. - * - Now, because you found it in the ThreadLocal cache – and notably did + *

+ *

*/ @ParameterizedTest @EnumSource(Taps.class) @@ -1162,10 +1166,10 @@ void getTargetSizeMustReturnLastSetTargetSize() { /** * When we increase the size of a depleted pool, it should be possible to * make claim again and get out newly allocated objects. - * + *

* We test for this by depleting a pool, upping the size and then claiming * again with a timeout that is longer than the timeout of the test. The test - * pass if it does not timeout. + * pass if it does not time out. */ @ParameterizedTest @EnumSource(Taps.class) @@ -1186,7 +1190,7 @@ void increasingSizeMustAllowMoreAllocations(Taps taps) throws Exception { * allocates, when the pool is shrunk. This is difficult because the pool * cannot tell us when it reaches the target size, so we have to figure this * out by using a special allocator. - * + *

* We test for this by configuring a CountingAllocator that also unparks a * thread (namely ours, the main thread for the test) at every allocation * and deallocation. We also configure the pool to have a somewhat large @@ -1236,14 +1240,14 @@ void decreasingSizeMustEventuallyDeallocateSurplusObjects(Taps taps) throws Exce * Similar to the decreasingSizeMustEventuallyDeallocateSurplusObjects test * above, but this time the objects are all expired after the pool has been * shrunk. - * + *

* Again, we deplete the pool. Once depleted, our expiration has been * configured such, that all subsequent items one tries to claim, will be * expired. - * + *

* Then we set the new lower target size, and release just enough for the * pool to reach the new target. - * + *

* Then we try to claim an object from the pool with a very short timeout. * This will return null because the pool is still depleted. We also check * that the pool has not made any new allocations, even though we have been @@ -1298,7 +1302,7 @@ void settingTargetSizeOnPoolThatHasBeenShutDownDoesNothing() { /** * Make sure that the pool does not get into a bad state, caused by concurrent * background resizing jobs interfering with each other. - * + *

* We test this by creating a small pool, then resizing it larger (so much so that * any resizing job is unlikely to finish before we can make our next move) and then * immediately resizing it smaller again. This should put multiple resizing jobs in @@ -1353,7 +1357,10 @@ void decreasingSizeMustNotDeallocateTlrClaimedObjects(Taps taps) throws Exceptio } obj.release(); - assertThat(allocator.getDeallocations()).doesNotContain(obj); + List deallocations = allocator.getDeallocations(); + synchronized (deallocations) { + assertThat(deallocations).doesNotContain(obj); + } } /** @@ -1962,7 +1969,10 @@ void explicitlyExpiredObjectsMustBeDeallocated(Taps taps) throws Exception { a.expire(); a.release(); tap.claim(longTimeout).release(); - assertThat(allocator.getDeallocations()).contains(a); + List deallocations = allocator.getDeallocations(); + synchronized (deallocations) { + assertThat(deallocations).contains(a); + } } @@ -1978,7 +1988,10 @@ void shutDownMustDeallocateExplicitlyExpiredObjects(Taps taps) throws Exception shutdown.await(longTimeout); assertEquals(allocator.countAllocations(), 1); assertEquals(allocator.countDeallocations(), 1); - assertThat(allocator.getDeallocations()).contains(a); + List deallocations = allocator.getDeallocations(); + synchronized (deallocations) { + assertThat(deallocations).contains(a); + } } // NOTE: When adding, removing or modifying tests, also remember to update diff --git a/src/test/java/blackbox/ThreadBasedPoolTest.java b/src/test/java/blackbox/ThreadBasedPoolTest.java index 06f8b32c..5181e6ca 100644 --- a/src/test/java/blackbox/ThreadBasedPoolTest.java +++ b/src/test/java/blackbox/ThreadBasedPoolTest.java @@ -383,7 +383,10 @@ void mustNotFrivolouslyReallocateNonPoisonedSlotsDuringEagerRecovery() try { assertThat(allocator.countAllocations()).isEqualTo(3); assertThat(allocator.countDeallocations()).isEqualTo(0); // allocation failed - assertThat(allocator.getDeallocations()).doesNotContain(a, b); + List deallocations = allocator.getDeallocations(); + synchronized (deallocations) { + assertThat(deallocations).doesNotContain(a, b); + } } finally { a.release(); b.release(); @@ -567,30 +570,32 @@ void mustStillBeUsableAfterExceptionInReallocateWithBackgroundThread(Taps taps) /** * Here's the scenario we're trying to target: - * - * - You (or your thread) do a successful claim, and triumphantly stores it - * in the ThreadLocal cache. - * - You then return the object after use, so now it's back in the - * live-queue for others to grab. - * - Someone else tries to claim the object, but decides that it has expired, - * and sends it off through the dead-queue to be reallocated. - * - The reallocation fails for some reason, and the slot is now poisoned. - * - You want to claim an object again, and start by looking in the - * ThreadLocal cache. - * - You find the slot for the object you had last, but the slot is poisoned. - * - Now, because you found it in the ThreadLocal cache – and notably did + *

+ *

*/ @ParameterizedTest @EnumSource(Taps.class) diff --git a/src/test/java/blackbox/slow/PoolIT.java b/src/test/java/blackbox/slow/PoolIT.java index a625922a..0fc4244a 100644 --- a/src/test/java/blackbox/slow/PoolIT.java +++ b/src/test/java/blackbox/slow/PoolIT.java @@ -270,7 +270,10 @@ void mustNotDeallocateNullsFromLiveQueueDuringShutdown() throws Exception { semaphore.acquire(); } - assertThat(allocator.getDeallocations()).isEqualTo(subList); + List deallocations = allocator.getDeallocations(); + synchronized (deallocations) { + assertThat(deallocations).isEqualTo(subList); + } objs.get(startingSize - 1).release(); assertTrue(completionFuture.get(1, TimeUnit.MINUTES), "shutdown timeout elapsed"); diff --git a/src/test/java/blackbox/slow/ThreadBasedPoolIT.java b/src/test/java/blackbox/slow/ThreadBasedPoolIT.java index 6a921bec..caa87b44 100644 --- a/src/test/java/blackbox/slow/ThreadBasedPoolIT.java +++ b/src/test/java/blackbox/slow/ThreadBasedPoolIT.java @@ -126,7 +126,10 @@ void decreasingSizeOfDepletedPoolMustOnlyDeallocateAllocatedObjects() semaphore.acquire(); } - assertThat(allocator.getDeallocations()).containsExactlyElementsOf(subList); + List deallocations = allocator.getDeallocations(); + synchronized (deallocations) { + assertThat(deallocations).containsExactlyElementsOf(subList); + } objs.get(startingSize - 1).release(); }