Skip to content

Commit

Permalink
Reinstate testRunnableRunsAtMostOnceAfterCancellation (#99525)
Browse files Browse the repository at this point in the history
This test was failing in #34004 due to a race, and although #34296 made
the failures rarer they did not actually fix the race. Then in #99201 we
fixed the race but the resulting test over-synchronizes and no longer
meaningfully verifies the concurrent behaviour we were originally trying
to check. It also fails for other reasons. This commit reverts back to
the original test showing that we might run the action at most once
after cancellation without any further synchronization, but fixes the
assertion to use the value of the counter observed immediately after the
cancellation since we cannot be sure that no extra iterations execute
before the cancellation completes.
  • Loading branch information
DaveCTurner committed Sep 13, 2023
1 parent 6392a7f commit a6dd6a9
Showing 1 changed file with 16 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
import java.util.concurrent.atomic.AtomicReference;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.oneOf;
import static org.hamcrest.Matchers.sameInstance;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -273,25 +273,28 @@ public ScheduledCancellable schedule(Runnable command, TimeValue delay, String e
assertTrue(reschedulingRunnable.isCancelled());
}

public void testRunnableDoesNotRunAfterCancellation() throws Exception {
final int iterations = scaledRandomIntBetween(2, 12);
public void testRunnableRunsAtMostOnceAfterCancellation() throws Exception {
final var intervalMillis = randomLongBetween(1, 50);
final AtomicInteger counter = new AtomicInteger();
final CountDownLatch doneLatch = new CountDownLatch(iterations);
final CountDownLatch doneLatch = new CountDownLatch(scaledRandomIntBetween(1, 12));
final Runnable countingRunnable = () -> {
counter.incrementAndGet();
doneLatch.countDown();
};

final TimeValue interval = TimeValue.timeValueMillis(50L);
final Cancellable cancellable = threadPool.scheduleWithFixedDelay(countingRunnable, interval, Names.GENERIC);
doneLatch.await();
cancellable.cancel();

final int counterValue = counter.get();
assertThat(counterValue, equalTo(iterations));

final Cancellable cancellable = threadPool.scheduleWithFixedDelay(
countingRunnable,
TimeValue.timeValueMillis(intervalMillis),
Names.GENERIC
);
safeAwait(doneLatch);
assertTrue(cancellable.cancel());
final var iterations = counter.get();
if (rarely()) {
assertBusy(() -> assertThat(counter.get(), equalTo(iterations)), 5 * interval.millis(), TimeUnit.MILLISECONDS);
Thread.sleep(randomLongBetween(0, intervalMillis * 5));
} else if (randomBoolean()) {
Thread.yield();
}
assertThat(counter.get(), oneOf(iterations, iterations + 1));
}
}

0 comments on commit a6dd6a9

Please sign in to comment.