Skip to content

Enforce user-set gRPC call timeouts on the client - #3699

Draft
oldergod wants to merge 1 commit into
masterfrom
bquenaudon/grpc-client-timeout-enforcement
Draft

Enforce user-set gRPC call timeouts on the client#3699
oldergod wants to merge 1 commit into
masterfrom
bquenaudon/grpc-client-timeout-enforcement

Conversation

@oldergod

Copy link
Copy Markdown
Member

Problem

A timeout or a deadline set on GrpcCall.timeout or GrpcStreamingCall.timeout is serialized into the grpc-timeout request header, but the client never enforces it.

In RealGrpcCall.initCall, the user-set value stays on an inert Timeout() delegate and never reaches OkHttp. RealGrpcStreamingCall.initCall swaps 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

initCall now 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-timeout header logic is unchanged. No public API change (apiCheck passes without an .api update).

Tests

New GrpcClientTimeoutTest (unary and streaming, duration and deadline). The server receives each request and never responds, and it ignores grpc-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:apiCheck all pass.

Context: #2338 (comment)

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant