Skip to content

Commit 89dd6bf

Browse files
committed
Fix flow control bypass during client response draining in ext_proc.
When activateCall is called during draining (initiated by response drain), it flushes requests buffered in DelayedClientCall directly to the rawCall, bypassing flow control checks. To fix this: 1. Buffer request(n) calls in pendingRequests when the call is IDLE, instead of letting them go to DelayedClientCall. 2. Guard drainPendingRequests to check isResponseSidecarReady and abort if it is false (keeping requests buffered). 3. Trigger drainPendingRequests when response drain completes. Also updated tests to verify early request buffering during response drain. TAG=agy CONV=2c1e4760-c239-4698-810a-162bf10fccc4
1 parent aefc6d0 commit 89dd6bf

2 files changed

Lines changed: 33 additions & 7 deletions

File tree

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,9 @@ private void onExtProcStreamReady() {
726726
}
727727

728728
private void drainPendingRequests() {
729+
if (!isResponseSidecarReady()) {
730+
return;
731+
}
729732
int toRequest = pendingRequests.getAndSet(0);
730733
if (toRequest > 0) {
731734
super.request(toRequest);
@@ -828,6 +831,10 @@ public void request(int numMessages) {
828831
super.request(numMessages);
829832
return;
830833
}
834+
if (dataPlaneCallState.get() == DataPlaneCallState.IDLE) {
835+
pendingRequests.addAndGet(numMessages);
836+
return;
837+
}
831838
if (!isResponseSidecarReady()) {
832839
pendingRequests.addAndGet(numMessages);
833840
return;
@@ -1391,6 +1398,7 @@ void unblockAfterResponseDrain() {
13911398
}
13921399
proceedWithHeaders();
13931400
proceedWithSavedMessages();
1401+
dataPlaneClientCall.drainPendingRequests();
13941402
proceedWithClose();
13951403
}
13961404

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

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9020,6 +9020,8 @@ public void givenResponseDrainActive_whenAppRequestsMessages_thenRequestsBuffere
90209020
ExternalProcessorFilterConfig filterConfig = configOrError.config;
90219021

90229022
final CountDownLatch drainSentLatch = new CountDownLatch(1);
9023+
final CountDownLatch headersReceivedLatch = new CountDownLatch(1);
9024+
final CountDownLatch sendDrainLatch = new CountDownLatch(1);
90239025
// External Processor Server
90249026
ExternalProcessorGrpc.ExternalProcessorImplBase extProcImpl;
90259027
extProcImpl = new ExternalProcessorGrpc.ExternalProcessorImplBase() {
@@ -9032,10 +9034,20 @@ public StreamObserver<ProcessingRequest> process(
90329034
@Override
90339035
public void onNext(ProcessingRequest request) {
90349036
if (request.hasRequestHeaders()) {
9035-
responseObserver.onNext(ProcessingResponse.newBuilder()
9036-
.setRequestDrainResponses(true)
9037-
.build());
9038-
drainSentLatch.countDown();
9037+
headersReceivedLatch.countDown();
9038+
new Thread(() -> {
9039+
try {
9040+
sendDrainLatch.await();
9041+
synchronized (responseObserver) {
9042+
responseObserver.onNext(ProcessingResponse.newBuilder()
9043+
.setRequestDrainResponses(true)
9044+
.build());
9045+
}
9046+
drainSentLatch.countDown();
9047+
} catch (Exception e) {
9048+
System.out.println("JetskiDebug: error in mock server: " + e);
9049+
}
9050+
}).start();
90399051
}
90409052
}
90419053

@@ -9094,15 +9106,21 @@ public void request(int numMessages) {
90949106
ClientCall<String, String> proxyCall =
90959107
interceptCall(interceptor, METHOD_SAY_HELLO, callOptions, dataPlaneChannel);
90969108
proxyCall.start(new ClientCall.Listener<String>() {}, new Metadata());
9109+
9110+
// Wait for headers to reach mock server
9111+
assertThat(headersReceivedLatch.await(5, TimeUnit.SECONDS)).isTrue();
9112+
9113+
// App requests messages early (before drain is received)
9114+
proxyCall.request(3);
9115+
9116+
// Now trigger drain
9117+
sendDrainLatch.countDown();
90979118

90989119
// Wait for drain to be processed
90999120
assertThat(drainSentLatch.await(5, TimeUnit.SECONDS)).isTrue();
91009121
// proxyCall.isReady() should remain true during response drain (request path is NOT draining)
91019122
assertThat(proxyCall.isReady()).isTrue();
91029123

9103-
// App requests more messages
9104-
proxyCall.request(3);
9105-
91069124
// Verify requests are buffered and not sent to data plane
91079125
assertThat(dataPlaneRequestCount.get()).isEqualTo(0);
91089126

0 commit comments

Comments
 (0)