Enforce user-set gRPC call timeouts on the client - #3699
Draft
oldergod wants to merge 1 commit into
Draft
Conversation
A timeout or a deadline set on GrpcCall.timeout or GrpcStreamingCall.timeout was serialized into the grpc-timeout request header, but the client never enforced it. When the server or a proxy ignores that header, or when the connection stalls, nothing bounded the call on the client. grpc-java sends the header and also runs a client-side deadline timer. Wire only did the header half. Now initCall copies the user-set timeout and deadline onto the OkHttp per-call timeout before the call starts. OkHttp cancels the call when the bound is reached. Behavior is unchanged when the user sets no value. The header logic is unchanged. Co-authored-by: Benoît Quenaudon <benoit@quenaudon.com> Signed-off-by: Benoît Quenaudon <benoit@quenaudon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A timeout or a deadline set on
GrpcCall.timeoutorGrpcStreamingCall.timeoutis serialized into thegrpc-timeoutrequest header, but the client never enforces it.In
RealGrpcCall.initCall, the user-set value stays on an inertTimeout()delegate and never reaches OkHttp.RealGrpcStreamingCall.initCallswaps the delegate after header serialization, which discards the user-set value. In both cases, only the server sees the bound.If the server or a proxy ignores
grpc-timeout, or if the connection stalls, nothing bounds the call on the client. For streaming calls the situation is worse: OkHttp does not apply its read timeout to a stalled duplex call, so such a call can hang without any bound at all.grpc-java does both halves: it sends the header and it runs a client-side deadline timer that cancels the call. Wire only did the header half.
Fix
initCallnow copies the user-set timeout and deadline onto the OkHttp per-call timeout before the call starts, then sets the delegate as before. OkHttp cancels the call when the bound is reached, which matches grpc-java semantics.When the user sets no value, both copies are no-ops and behavior is unchanged: the OkHttpClient defaults apply. The
grpc-timeoutheader logic is unchanged. No public API change (apiCheckpasses without an.apiupdate).Tests
New
GrpcClientTimeoutTest(unary and streaming, duration and deadline). The server receives each request and never responds, and it ignoresgrpc-timeout. The client has no call timeout and a 10 s read timeout. Each test sets a 500 ms per-call bound.On master, the unary tests fail after 10 s (the read timeout fires, not the per-call bound, and the call is not canceled) and the streaming tests block past the 30 s test limit. With the fix, all 4 tests pass in about 0.5 s and the calls cancel client-side.
:wire-grpc-tests:test(121 tests),spotlessCheck, and:wire-grpc-client:apiCheckall pass.Context: #2338 (comment)