Skip to content

Commit d5aea00

Browse files
authored
test(pubsub): fix test race and re-enable testRunShutdown_TimeoutExceeded (#14339)
Re-enable `StreamingSubscriberConnectionTest.testRunShutdown_TimeoutExceeded` and resolve the test race condition that caused builds to hang indefinitely. - Synchronize the test thread with the background stop thread by polling for `Thread.State.TIMED_WAITING` before advancing `FakeClock`. This guarantees `startTime` is recorded before time advances, eliminating the infinite wait loop in `Waiter.tryWait()`. - Reduce test shutdown timeout from 2s to 500ms to reduce test latency by ~1.5s per run. - Add proper connection termination cleanup to client pinger and server monitor tests to prevent background callbacks leaking into other tests. - Remove unused `org.junit.Ignore` import. - Ran 100 iterations of this test to confirm it is not flaky. Fixes #13706
1 parent 54c0d0f commit d5aea00

2 files changed

Lines changed: 40 additions & 21 deletions

File tree

‎java-pubsub/google-cloud-pubsub/pom.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,11 @@
133133
<artifactId>mockito-core</artifactId>
134134
<scope>test</scope>
135135
</dependency>
136+
<dependency>
137+
<groupId>org.awaitility</groupId>
138+
<artifactId>awaitility</artifactId>
139+
<scope>test</scope>
140+
</dependency>
136141
<dependency>
137142
<groupId>com.google.cloud</groupId>
138143
<artifactId>google-cloud-core</artifactId>

‎java-pubsub/google-cloud-pubsub/src/test/java/com/google/cloud/pubsub/v1/StreamingSubscriberConnectionTest.java‎

Lines changed: 35 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,14 @@
1616

1717
package com.google.cloud.pubsub.v1;
1818

19+
import static org.awaitility.Awaitility.await;
1920
import static org.junit.Assert.*;
2021
import static org.mockito.ArgumentMatchers.any;
2122
import static org.mockito.ArgumentMatchers.argThat;
2223
import static org.mockito.Mockito.*;
2324

2425
import com.google.api.core.ApiFutures;
26+
import com.google.api.core.ApiService;
2527
import com.google.api.core.SettableApiFuture;
2628
import com.google.api.gax.batching.FlowControlSettings;
2729
import com.google.api.gax.batching.FlowController;
@@ -55,7 +57,6 @@
5557
import java.util.concurrent.atomic.AtomicInteger;
5658
import org.junit.After;
5759
import org.junit.Before;
58-
import org.junit.Ignore;
5960
import org.junit.Rule;
6061
import org.junit.Test;
6162
import org.junit.rules.TestName;
@@ -164,15 +165,14 @@ public void testRunShutdown_TimeoutMet() throws Exception {
164165
streamingSubscriberConnection.awaitTerminated(1, TimeUnit.SECONDS);
165166
}
166167

167-
@Ignore("https://github.com/googleapis/google-cloud-java/issues/13706")
168168
@Test
169169
public void testRunShutdown_TimeoutExceeded() throws Exception {
170170
final SettableApiFuture<com.google.protobuf.Empty> ackFuture = SettableApiFuture.create();
171171
when(mockSubscriberStub.acknowledgeCallable().futureCall(any(AcknowledgeRequest.class)))
172172
.thenReturn(ackFuture);
173173

174174
SubscriberShutdownSettings shutdownSettings =
175-
SubscriberShutdownSettings.newBuilder().setTimeout(Duration.ofSeconds(2)).build();
175+
SubscriberShutdownSettings.newBuilder().setTimeout(Duration.ofMillis(500)).build();
176176
StreamingSubscriberConnection.Builder builder =
177177
StreamingSubscriberConnection.newBuilder(
178178
mock(MessageReceiverWithAckResponse.class, withSettings().withoutAnnotations()));
@@ -195,25 +195,30 @@ public void testRunShutdown_TimeoutExceeded() throws Exception {
195195
});
196196
t.start();
197197

198-
Thread t2 =
199-
new Thread(
200-
() -> {
201-
try {
202-
streamingSubscriberConnection.awaitTerminated(1, TimeUnit.SECONDS);
203-
fail("Should have timed out");
204-
} catch (TimeoutException e) {
205-
// expected
206-
}
207-
});
208-
t2.start();
209-
t2.join();
198+
// Wait until the stop thread is blocked in tryWait (waiting for pending operations).
199+
await()
200+
.atMost(Duration.ofSeconds(5))
201+
.pollInterval(Duration.ofMillis(10))
202+
.until(() -> t.getState() == Thread.State.TIMED_WAITING);
203+
204+
// Connection should be in STOPPING state, not TERMINATED yet.
205+
assertEquals(ApiService.State.STOPPING, streamingSubscriberConnection.state());
206+
207+
// Verify awaitTerminated times out since the shutdown timeout has not elapsed yet.
208+
try {
209+
streamingSubscriberConnection.awaitTerminated(10, TimeUnit.MILLISECONDS);
210+
fail("Should have timed out");
211+
} catch (TimeoutException e) {
212+
// expected
213+
}
210214

211215
// Advance the clock past the shutdown timeout.
212-
clock.advance(3, TimeUnit.SECONDS);
213-
t.join();
216+
clock.advance(1, TimeUnit.SECONDS);
217+
t.join(5000);
218+
assertFalse("Stop thread should have finished", t.isAlive());
214219

215220
// Now it should terminate.
216-
streamingSubscriberConnection.awaitTerminated();
221+
streamingSubscriberConnection.awaitTerminated(5, TimeUnit.SECONDS);
217222
assertFalse(streamingSubscriberConnection.isRunning());
218223
assertFalse(messageFuture.isDone());
219224
}
@@ -748,7 +753,7 @@ public void testClientPinger_pingSent() {
748753
}
749754

750755
@Test
751-
public void testClientPinger_pingsNotSentWhenDisabled() {
756+
public void testClientPinger_pingsNotSentWhenDisabled() throws TimeoutException {
752757
ClientStream<StreamingPullRequest> mockClientStream =
753758
mock(ClientStream.class, withSettings().withoutAnnotations());
754759
when(mockStreamingCallable.splitCall(any(ResponseObserver.class), any()))
@@ -768,10 +773,13 @@ public void testClientPinger_pingsNotSentWhenDisabled() {
768773
systemExecutor.advanceTime(CLIENT_PING_INTERVAL);
769774

770775
verify(mockClientStream, times(1)).send(any(StreamingPullRequest.class));
776+
777+
streamingSubscriberConnection.stopAsync();
778+
streamingSubscriberConnection.awaitTerminated(5, TimeUnit.SECONDS);
771779
}
772780

773781
@Test
774-
public void testServerMonitor_timesOut() {
782+
public void testServerMonitor_timesOut() throws TimeoutException {
775783
ClientStream<StreamingPullRequest> mockClientStream =
776784
mock(ClientStream.class, withSettings().withoutAnnotations());
777785
ArgumentCaptor<ResponseObserver<StreamingPullResponse>> observerCaptor =
@@ -815,10 +823,13 @@ public void testServerMonitor_timesOut() {
815823
StatusException exception = (StatusException) exceptionCaptor.getValue();
816824
assertEquals(Code.UNAVAILABLE, exception.getStatus().getCode());
817825
assertEquals("Keepalive timeout with server", exception.getStatus().getDescription());
826+
827+
streamingSubscriberConnection.stopAsync();
828+
streamingSubscriberConnection.awaitTerminated(5, TimeUnit.SECONDS);
818829
}
819830

820831
@Test
821-
public void testServerMonitor_doesNotTimeOutIfResponseReceived() {
832+
public void testServerMonitor_doesNotTimeOutIfResponseReceived() throws TimeoutException {
822833
ClientStream<StreamingPullRequest> mockClientStream =
823834
mock(ClientStream.class, withSettings().withoutAnnotations());
824835
ArgumentCaptor<ResponseObserver<StreamingPullResponse>> observerCaptor =
@@ -847,6 +858,9 @@ public void testServerMonitor_doesNotTimeOutIfResponseReceived() {
847858
observer.onResponse(StreamingPullResponse.getDefaultInstance());
848859

849860
verify(mockClientStream, never()).closeSendWithError(any(Exception.class));
861+
862+
streamingSubscriberConnection.stopAsync();
863+
streamingSubscriberConnection.awaitTerminated(5, TimeUnit.SECONDS);
850864
}
851865

852866
private StreamingSubscriberConnection getStreamingSubscriberConnection(

0 commit comments

Comments
 (0)