Keep the receive-handler consumer alive until ServeHTTP returns - #1313
Open
imu2008 wants to merge 1 commit into
Open
Keep the receive-handler consumer alive until ServeHTTP returns#1313imu2008 wants to merge 1 commit into
imu2008 wants to merge 1 commit into
Conversation
Protocol.ServeHTTP hands the inbound message over on the unbuffered Protocol.incoming channel and blocks there until a consumer picks it up. NewHTTPReceiveHandler starts exactly one consumer goroutine per ServeHTTP call, so the two have to stay paired. That consumer waited on the request context. When a client disconnects during the hand-off the consumer exits while the message it was meant to pick up is still pending, and the pairing is permanently offset by one: from then on every request is served by the *next* request's consumer, so each one blocks until further traffic arrives. On a low-traffic endpoint this surfaces as requests hanging until the client or gateway times out, while high traffic hides it completely. Restarting the process is the only way to clear it. Wait on a context that is cancelled when ServeHTTP returns instead. While a message is pending its producer has not returned yet, so at least one consumer is guaranteed to still be waiting for it. The cancellation cannot simply be dropped: Protocol.ServeHTTP has paths that return without sending anything (rate limiting, OPTIONS, GET) and Protocol.incoming is never closed, so an unconditionally waiting consumer would block forever on every such request. Cancelling on return covers both cases. Fixes cloudevents#1224 Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: winger <panhuawenmail@gmail.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.
Fixes #1224.
The invariant that was broken
Protocol.ServeHTTPhands the inbound message over on the unbufferedProtocol.incomingchannel and blocks there until a consumer picks it up.
NewHTTPReceiveHandlerstarts exactlyone consumer goroutine per
ServeHTTPcall, so the two have to stay paired.That consumer waited on the request context. When a client disconnects during the
hand-off, the consumer exits while the message it was meant to pick up is still pending, and
the pairing is permanently offset by one: from then on every request is served by the next
request's consumer, so each one blocks until further traffic arrives.
That is also why it is easy to miss — on a busy endpoint the next request arrives within
milliseconds and nothing looks wrong; on a quiet one requests hang until the client or the
gateway gives up. Restarting the process is the only way to clear it.
The fix
Wait on a context that is cancelled when
ServeHTTPreturns, instead of the request context.While a message is pending its producer has not returned yet, so at least one consumer is
guaranteed to still be waiting for it.
Why the cancellation cannot simply be dropped
This looked like the obvious fix at first and it is wrong:
Protocol.ServeHTTPhas paths thatreturn without sending anything —
!ok→ 429)http.MethodOptionshttp.MethodGetProtocol.incomingis never closed, andnet/httpcancels the request context when thehandler returns, so today that cancellation is what lets the consumer of such a request exit.
An unconditionally waiting consumer would block forever on every OPTIONS request, GET request
and rate-limited request.
Cancelling on
ServeHTTPreturn covers both cases: it releases consumers whose request neverproduced a message, and it keeps alive those whose message is still in flight.
Scope
Two lines of behaviour change in
v2/client/http_receiver.go; the rest of the diff is thecomment explaining the invariant. No change to
protocol/http, no new exported API, andnothing that requires a newer Go version than the module already declares.
Invokestill receivesreq.Context(), so the context seen by receiver functions isunchanged. The new context is only ever passed to
Respond, which uses it solely for theselect— it reads no values and propagates no deadline — and its lifetime is bounded by thehandler via
defer, so a consumer can never outlive itsServeHTTPcall. Deriving it fromcontext.Background()rather than from the request context keeps this working on the Goversion the module declares (
context.WithoutCancelis Go 1.21).Test
TestEventReceiverServeHTTP_ConsumerPairinginv2/client/http_receiver_test.go.It serves a request whose context is already cancelled, then two healthy ones, and asserts
each
ServeHTTPreturns and each event reaches the receiver function. Without the fix thefirst
ServeHTTPnever returns — its own consumer is gone and no further request follows totake the message — so the test fails on a timeout:
With the fix it passes immediately. The full
v2suite passes;gofmtandgo vetare clean.I deliberately did not add a goroutine-count assertion for the no-send paths — it tends to be
flaky in shared test binaries. The reasoning is captured in the comment instead, and the
existing
TestEventReceiverServeHTTP_Options/_Webhooktests cover those pathsfunctionally.