Skip to content

Commit c9e02fa

Browse files
committed
Fix onReady notification flow control and verification.
1. Fix onReadyNotify in DataPlaneListener to only forward onReady if dataPlaneClientCall.isReady() is true. 2. Call onReadyNotify in handleRequestDrainComplete to notify the app when request draining completes and the path becomes ready again. 3. Update givenRequestDrainActive_whenAppRequestsMessages_thenRequestsDrained to verify that onReady is NOT called during request drain but IS called after the drain completes. TAG=agy CONV=2c1e4760-c239-4698-810a-162bf10fccc4
1 parent 71a3964 commit c9e02fa

2 files changed

Lines changed: 33 additions & 2 deletions

File tree

xds/src/main/java/io/grpc/xds/ExternalProcessorClientInterceptor.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1064,6 +1064,9 @@ private void handleRequestDrainComplete() {
10641064
if (requestDraining.compareAndSet(true, false)) {
10651065
requestDrainComplete.set(true);
10661066
drainPendingDrainingMessages();
1067+
if (wrappedListener != null) {
1068+
wrappedListener.onReadyNotify();
1069+
}
10671070
}
10681071
}
10691072

@@ -1327,7 +1330,9 @@ public void onClose(Status status, Metadata trailers) {
13271330
}
13281331

13291332
void onReadyNotify() {
1330-
dataPlaneClientCall.getCallContext().run(() -> delegate().onReady());
1333+
if (dataPlaneClientCall.isReady()) {
1334+
dataPlaneClientCall.getCallContext().run(() -> delegate().onReady());
1335+
}
13311336
}
13321337

13331338
void proceedWithHeaders() {

xds/src/test/java/io/grpc/xds/ExternalProcessorClientInterceptorTest.java

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9185,13 +9185,16 @@ public void givenRequestDrainActive_whenAppRequestsMessages_thenRequestsDrained(
91859185
final CountDownLatch headersReceivedLatch = new CountDownLatch(1);
91869186
final CountDownLatch sendDrainLatch = new CountDownLatch(1);
91879187
final CountDownLatch filterSentDrainCompleteLatch = new CountDownLatch(1);
9188+
final AtomicReference<StreamObserver<ProcessingResponse>> responseObserverRef =
9189+
new AtomicReference<>();
91889190
// External Processor Server
91899191
ExternalProcessorGrpc.ExternalProcessorImplBase extProcImpl;
91909192
extProcImpl = new ExternalProcessorGrpc.ExternalProcessorImplBase() {
91919193
@Override
91929194
@SuppressWarnings("unchecked")
91939195
public StreamObserver<ProcessingRequest> process(
91949196
final StreamObserver<ProcessingResponse> responseObserver) {
9197+
responseObserverRef.set(responseObserver);
91959198
((ServerCallStreamObserver<ProcessingResponse>) responseObserver).request(100);
91969199
return new StreamObserver<ProcessingRequest>() {
91979200
@Override
@@ -9203,6 +9206,7 @@ public void onNext(ProcessingRequest request) {
92039206
sendDrainLatch.await();
92049207
synchronized (responseObserver) {
92059208
responseObserver.onNext(ProcessingResponse.newBuilder()
9209+
.setRequestHeaders(HeadersResponse.newBuilder().build())
92069210
.setRequestDrainRequests(true)
92079211
.build());
92089212
}
@@ -9270,7 +9274,8 @@ public void request(int numMessages) {
92709274
CallOptions callOptions = DEFAULT_CALL_OPTIONS.withExecutor(MoreExecutors.directExecutor());
92719275
ClientCall<String, String> proxyCall =
92729276
interceptCall(interceptor, METHOD_SAY_HELLO, callOptions, dataPlaneChannel);
9273-
proxyCall.start(new ClientCall.Listener<String>() {}, new Metadata());
9277+
ClientCall.Listener<String> mockListener = Mockito.mock(ClientCall.Listener.class);
9278+
proxyCall.start(mockListener, new Metadata());
92749279

92759280
// Wait for headers to reach mock server
92769281
assertThat(headersReceivedLatch.await(5, TimeUnit.SECONDS)).isTrue();
@@ -9289,10 +9294,31 @@ public void request(int numMessages) {
92899294

92909295
// Verify requests are now drained to data plane (request drain does not block response path)
92919296
assertThat(dataPlaneRequestCount.get()).isEqualTo(3);
9297+
9298+
// Verify onReady was NOT called because request path is draining
9299+
Mockito.verify(mockListener, Mockito.never()).onReady();
92929300

92939301
// Wait for filter to send drain_complete to mock server
92949302
assertThat(filterSentDrainCompleteLatch.await(5, TimeUnit.SECONDS)).isTrue();
92959303

9304+
// Echo drain_complete back to filter to complete handshake
9305+
synchronized (responseObserverRef.get()) {
9306+
responseObserverRef.get().onNext(ProcessingResponse.newBuilder()
9307+
.setRequestBody(BodyResponse.newBuilder()
9308+
.setResponse(CommonResponse.newBuilder()
9309+
.setBodyMutation(BodyMutation.newBuilder()
9310+
.setStreamedResponse(StreamedBodyResponse.newBuilder()
9311+
.setDrainComplete(true)
9312+
.build())
9313+
.build())
9314+
.build())
9315+
.build())
9316+
.build());
9317+
}
9318+
9319+
// Verify onReady IS now called after request drain completes
9320+
Mockito.verify(mockListener, Mockito.timeout(5000)).onReady();
9321+
92969322
proxyCall.cancel("Cleanup", null);
92979323
channelManager.close();
92989324
}

0 commit comments

Comments
 (0)