Repository navigation
Conversation
…read.sleep Replace CurrentMillisClock and real Thread.sleep delays in DirectRetryingExecutorTest with FakeApiClock and a sleep() override that advances virtual time deterministically. This prevents testMaxRetriesExceeded from flaking with premature totalTimeout expiration on slow CI runners. Fixes googleapis#14625
…elpers Use FakeApiClock and RecordingScheduler in ScheduledRetryingExecutorTest for deterministic, sleep-free retry tests without modifying existing asynchronous in-flight test methods. Refactor RecordingScheduler to extend ScheduledThreadPoolExecutor directly, eliminating Mockito dependencies and strict-stubbing friction. Add Javadoc documentation for FakeApiClock and RecordingScheduler. Fixes googleapis#13535
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the retrying executor tests to use deterministic simulated time instead of real wall-clock sleeps, replacing the Mockito-based RecordingScheduler with a concrete ScheduledThreadPoolExecutor that advances a FakeApiClock. DirectRetryingExecutorTest and ScheduledRetryingExecutorTest are updated to use this fake clock. The feedback suggests replacing the non-thread-safe ArrayList used for sleepDurations in RecordingScheduler with a CopyOnWriteArrayList to prevent potential concurrent modification issues during multi-threaded test execution.
| final List<java.time.Duration> sleepDurations = new ArrayList<>(); | ||
| final AtomicInteger iterationsCount = new AtomicInteger(0); | ||
| private final FakeApiClock clock; | ||
| private final List<Duration> sleepDurations = new ArrayList<>(); |
There was a problem hiding this comment.
The sleepDurations list is implemented as a standard ArrayList, which is not thread-safe. Since RecordingScheduler extends ScheduledThreadPoolExecutor and can be called from multiple threads concurrently, concurrent calls to schedule can lead to race conditions or ConcurrentModificationException when writing to or reading from sleepDurations.
Using CopyOnWriteArrayList ensures thread safety for both writing (appending new durations) and reading (via getSleepDurations()). If any index-based operations are performed on this list concurrently, remember to snapshot the list to an array first to prevent IndexOutOfBoundsException.
| private final List<Duration> sleepDurations = new ArrayList<>(); | |
| private final List<Duration> sleepDurations = new java.util.concurrent.CopyOnWriteArrayList<>(); |
References
- Before suggesting thread-safe collection alternatives (such as CopyOnWriteArrayList) to avoid lock contention, verify the existing definition of the collection to ensure it is not already thread-safe.
- To safely perform index-based operations on a CopyOnWriteArrayList that can be concurrently modified or shrunk, snapshot the list to an array (e.g., using toArray()) and perform all index-based accesses on the array snapshot to prevent IndexOutOfBoundsException.
This branch has not been deployed
1 participant
No description provided.