Repository navigation
xDS: Support retries in RawMessageClientInterceptor and conditionally add it to the filter chain - #13042
Conversation
…payload access Introduce Filter.requiresPayloadAccess(config, overrideConfig) to allow filters to declare whether they need access to message payloads. Update ExternalProcessorFilter to implement this method, returning false when body send mode is NONE for both request and response. Update XdsNameResolver to only install RawMessageClientInterceptor when at least one configured filter requires payload access. Generalize ExternalProcessorClientInterceptor to support generic request/response types when payload interception is not needed. TAG=agy CONV=56c87717-243d-4f12-af9e-c532e509892a
| } else { | ||
| byteString = ByteString.readFrom(stream); | ||
| } | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
What's the best practice on handling exceptions in sendMessage ? Should we cancel the call and close listener or something when an exception happens with this error?
There was a problem hiding this comment.
I had thought the application can catch and cancel the call, but looking at how ClientCallImpl.sendMessageInternal handles exceptions by canceling the stream and the application gets notified via an onClose callback, I have now followed the same, and added a new unit test rawMessageClientInterceptor_sendMessageIoException_cancelsCall to cover this.
There was a problem hiding this comment.
So, might be worth discussing, but since we trigger cancel on sendmessage, but we don't have a way to make the application aware of it, the application might send another message or halfclose after cancellation triggering illegalstateexception.
We could either add a cancellation boolean or replace rawcall with noopcall to prevent it (I believe CheckedForwardingClientCall does this) or do cancellation(cleanup) + throw(making application handle error).
There was a problem hiding this comment.
I am not sure how, but we probably need to unit test the behavior when the RawInterceptor isn't installed. I think all existing tests install it unconditionally.
There was a problem hiding this comment.
I have updated the helper ExternalProcessorClientInterceptorTest.interceptCall() to mirror XdsNameResolver.createFilters():
- Only install
RawMessageClientInterceptor(interceptReq, interceptResp)wheninterceptReq || interceptRespis true. - That way, all existing header/trailer-only tests in
ExternalProcessorClientInterceptorTestwill automatically run withoutRawMessageClientInterceptorinstalled (and one-way body tests will only intercept the configured direction).
There was a problem hiding this comment.
Looks good at an initial glance, just make sure to verify with jacoco reports on coverage numbers.
…w feedback - Split Filter.requiresPayloadAccess into requiresRequestPayloadAccess and requiresResponsePayloadAccess so RawMessageClientInterceptor only intercepts the direction(s) that require payload access. - Short-circuit ExternalProcessorUtil.outboundStreamToByteString when the input stream is already a KnownLengthInputStream, and reuse it in RawMessageClientInterceptor. - Cancel the call in RawMessageClientInterceptor.sendMessage when reading the request stream fails with IOException or RuntimeException. - Fail the RPC as a protocol error if ext_proc returns a request_body or response_body response when the corresponding body mode is not GRPC. - Guard DataPlaneListener.onMessage and drainSavedMessages against casting response messages to InputStream when responseBodyMode != GRPC. TAG=agy CONV=56c87717-243d-4f12-af9e-c532e509892a
…ode is GRPC and add test TAG=agy CONV=56c87717-243d-4f12-af9e-c532e509892a
1. Supporting retries
Previously,
RawMessageClientInterceptor.sendMessage()forwarded a single-useInputStream directly to
rawCall.sendMessage(). When retries were triggered byRetriableStream, attempt 1 drained the InputStream to EOF. Subsequent retry
attempts would then serialize the already-drained stream, sending an empty (0-byte)
request payload.
Fix this by:
ByteString(usingDrainablewhen supported) in
RawMessageClientInterceptor.sendMessage()and wrapping itin a
KnownLengthInputStream.getByteString()onKnownLengthInputStream.RAW_MARSHALLER.stream()return a freshKnownLengthInputStreamwrappingthe underlying
ByteStringon each attempt, allowing retries to read the payloadfrom byte 0.
2. Conditional addition of
RawMessageClientInterceptorAlso we need to only install
RawMessageClientInterceptorwhen a filter requires payload access to avoid the performance hit for non ext_proc cases. As suggested in the grfc discussion, introduceFilter.requiresPayloadAccess(config, overrideConfig)to allow filters to declare whether they need access to message payloads. UpdateExternalProcessorFilterto implement this method, returning false when body send mode is NONE for both request and response. UpdateXdsNameResolverto only installRawMessageClientInterceptorwhen at least one configured filter requires payload access. GeneralizeExternalProcessorClientInterceptorto support generic request/response types when payload interception is not needed.Generalizing
ExternalProcessorClientInterceptorto Support Pass-through TypesExternalProcessorClientInterceptorpreviously hardcodedClientCall<InputStream, InputStream>andSimpleForwardingClientCallListener<InputStream>, assumingRawMessageClientInterceptorwas always present earlier in the chain to convert application messages toInputStream.Now that
RawMessageClientInterceptoris omitted when body send mode isNONE,ExternalProcessorClientInterceptordirectly handles the application's request and response types (ReqT,RespT).DataPlaneClientCallandDataPlaneListenerwere generalized to<ReqT, RespT>to avoid compiler-generated synthetic bridge casts toInputStream, which would otherwise result in a runtimeClassCastExceptionwhen sending or receiving application messages.Fixes #13010