Fix/mint client side timeout - #571
Open
freevova wants to merge 2 commits into
Open
Conversation
The Mint adapter accepts and parses `:timeout` but never applies it: unary
receives wait on the stream response process with `:infinity`, so the option
only ever reaches the server as the `grpc-timeout` header. When no response can
arrive — the connection going down without notifying the pending request, for
instance — the caller blocks forever even though it asked for a deadline.
Pass the requested timeout down to `build_stream/3` and translate an elapsed
deadline into `DEADLINE_EXCEEDED`. Along the way:
* `:deadline` now takes precedence over `:timeout`, as documented. `recv/2`
always fills in the 10s default under `:timeout`, so an explicit deadline
could otherwise never take effect.
* Both options are resolved by `GRPC.TimeUtils.to_relative/2`, which returns a
float — and a negative one for a deadline already in the past — so the value
is rounded and clamped before it reaches a receive timeout.
* Giving up resets the request, so the server stops working on it, and stops
the process buffering the response, which is linked to the caller rather
than to the connection and would otherwise outlive the call. Both steps are
bounded and best effort: they run after the deadline elapsed, against
processes that may themselves be gone or wedged.
Only unary receives are bounded. Server and bidirectional streams are consumed
lazily by the caller, where a gap between messages is expected rather than a
failure. A client stream awaits its response through a separate `recv/2` call,
which `GRPC.Stub` documents as unbounded even though it fills in the same 10s
default — worth settling separately from this fix.
Note this changes the default behaviour of a unary call that never receives a
response: it now fails after the 10s documented in `GRPC.Stub.call/5` instead of
blocking indefinitely.
The enforced deadline alters a documented default, which is the kind of change this changelog records under `### Behavior Changes`. There was no unreleased heading to file it under, so add one rather than assume the next version number.
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.
The Mint adapter accepts and parses
:timeout, but never applies it.do_receive_data/3waits on the stream response process withGenServer.call(pid, :get_response, :infinity), so the option only ever leaves the client as thegrpc-timeoutheader — a deadline the server enforces. When no response can arrive at all, the caller blocks forever despite having asked for a deadline.We hit this in production. The connection process crashed partway through notifying pending requests on a closed connection, so the callers queued behind it were never told the connection was gone; their background jobs stayed blocked for ~19 hours until the pod was restarted. #559 fixes that particular crash (thank you — 1.0.3 resolved it for us), but the
:infinityreceive means any other path that leaves a caller unnotified ends the same way.What this changes
The requested timeout is passed down to
build_stream/3, and an elapsed deadline becomes aDEADLINE_EXCEEDEDGRPC.RPCError. Two related corrections came out of that::deadlinenow takes precedence over:timeout, asrecv/2documents ("when the request is timeout, will override timeout").recv/2always fills in the 10s default under:timeoutviaKeyword.put_new/3, so an explicit deadline could otherwise never take effect.GRPC.TimeUtils.to_relative/2, which returns a float — and a negative one for a deadline already in the past — so the value is rounded and clamped before it reaches a receive timeout, which requires a non-negative integer.Giving up also cleans up after itself: the request is reset, so the server stops working on it, and the process buffering the response is stopped. That process is linked to the caller rather than to the connection, so nothing else would shut it down — least of all when the connection process is the very thing that went away. Both steps are best effort and bounded: they run after the deadline already elapsed, against processes that may themselves be gone or wedged, so a failure to tidy up must neither replace the error the caller is about to get nor keep it waiting much longer.
ConnectionProcess.cancel/3gained an optional timeout for that; its default is unchanged.Scope
Only unary receives are bounded.
Server and bidirectional streams are consumed lazily by the caller, where a gap between messages is expected rather than a failure.
Client streams are left as they are:
recv/2fills in the unary 10s default for every request type, whileGRPC.Stubdocuments streaming calls as unbounded, so honouring:timeoutthere would silently cut off uploads that legitimately take longer to be answered.Behaviour changes
GRPC.Stub.call/5already documents, instead of blocking indefinitely.DEADLINE_EXCEEDEDrather than the partial message. A unary response is not complete withoutgrpc-status, so I believe this is correct, but it is a deliberate choice rather than an accident.Both are recorded in the changelog. There was no unreleased heading to file them under, so I added one — move the entry wherever you prefer.
Tests
:deadlineresolves into are accepted, and a deadline already in the past is treated as an immediate one:deadlinewins over a long:timeoutdeadline:call throughGRPC.Stubagainst a live server, which is what actually exercises theparse_req_optspipeline the two fixes above live inThe whole suite passes locally (
375 passed, 2 skipped), and reverting the library change makes every new test fail.One adjacent thing, not fixed here
While adding the reset I noticed that
handle_call({:cancel_request, ref}, ...)pops the ref and callsMint.HTTP2.cancel_request/2, but does not calldrop_queued_request_chunks/2the waycancel_dead_stream/4does. Leftover{ref, body, from}entries then survive inrequest_stream_queue, and the nexthandle_continue(:process_request_stream_queue, ...)callsget_window_size/2for a ref Mint no longer knows, which raisesArgumentError. That was reachable before only through an explicitGRPC.Stub.cancel/1; automatic reset on a deadline makes it easier to hit.I have only reproduced the leftover state by injecting it rather than by starving a real send window end to end, so I am flagging it rather than fixing it here.