Skip to content

Commit

Permalink
fix(test): Fix intermittent timeout in PooledRequestQueueTest (#4777)
Browse files Browse the repository at this point in the history
I fixed a deadlock in this test a few weeks ago, but just ran into
one other timeout. This commit makes a few minor fixes to reduce
the chances of hitting a timeout here.

First:
(1) Use a fixed thread pool executor so we create all the threads
we need up front and reduce the work later.
(2) Don't start our timeout clock until the job we're expecting
to timeout has been queued.
(3) Increase the startup timeout to 50 ms and only time out the
test after 10x (instead of 5x); this is just so we're dealing with
slightly larger absolute numbers.  (Before we were only waiting
100 ms to time out the test, now we're waiting 500ms.)
(4) Fail with a specific message if we timeout, rather than assert
on the status of the job (which we know won't be in the expected
state because we killed it). This just helps debugging as I was
confused at first what happened.

I think this only happens if you are running the clouddriver tests
locally with your editor open while on a video call, but hopefully
with these fixes the tests pass even in that case.
  • Loading branch information
ezimanyi committed Aug 7, 2020
1 parent 1d28568 commit 22feb2a
Showing 1 changed file with 14 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.Assertions.fail;
import static org.mockito.Mockito.mock;

import com.netflix.spectator.api.NoopRegistry;
Expand Down Expand Up @@ -66,12 +67,12 @@ void timesOutIfRequestDoesNotComplete() {

@Test
void timesOutRequestIfDoesNotStartInTime() throws Exception {
long startTimeout = 20;
long startTimeout = 50;
PooledRequestQueue queue =
new PooledRequestQueue(
dynamicConfigService, new NoopRegistry(), startTimeout, 5 * startTimeout, 1);

ExecutorService executor = Executors.newCachedThreadPool();
ExecutorService executor = Executors.newFixedThreadPool(2);

CountDownLatch blockingJobStarted = new CountDownLatch(1);
CountDownLatch testJobExited = new CountDownLatch(1);
Expand All @@ -92,12 +93,14 @@ void timesOutRequestIfDoesNotStartInTime() throws Exception {

// Submit another job to the queue, and ensure that it is rejected before starting.
AtomicBoolean testJobRan = new AtomicBoolean(false);
CountDownLatch testJobQueued = new CountDownLatch(1);
Future<Void> testJob =
executor.submit(
safeRun(
() -> {
try {
blockingJobStarted.await();
testJobQueued.countDown();
queue.execute(
"foo",
() -> {
Expand All @@ -109,11 +112,16 @@ void timesOutRequestIfDoesNotStartInTime() throws Exception {
}
}));

// Wait for our jobs to finish and interrupt them if they take longer than a reasonable
// amount of time (where reasonable is a few times the startup timeout).
executor.shutdown();
executor.awaitTermination(5 * startTimeout, TimeUnit.MILLISECONDS);
executor.shutdownNow();

// Once the test job is queued, we'll wait a few times the startup timeout for it to finish.
testJobQueued.await();
if (!executor.awaitTermination(10 * startTimeout, TimeUnit.MILLISECONDS)) {
executor.shutdownNow();
// Fail the test immediately rather than assert on the status of the jobs, given that we
// interrupted them abnormally.
fail("Timeout waiting for queued jobs to finish.");
}

assertThatThrownBy(testJob::get).hasCauseInstanceOf(PromiseNotStartedException.class);
assertThat(testJobRan.get()).isFalse();
Expand Down

0 comments on commit 22feb2a

Please sign in to comment.