Harden the HTTP/2 connection-waiter registry against waiter failures and shutdown races#2251
Merged
Conversation
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.
Motivation:
#2227 introduced an event-driven HTTP/2 connection-waiter registry in
ChannelManager, but three issues remained. A waiter throwing an exception could prevent subsequent waiters from being notified and interrupt the connection establishment path, leaking a permit and leaving the request hanging. Requests registering during shutdown could miss both the close notification and timeout, leaving their futures incomplete. Additionally, timed-out or removed waiters left empty per-host sets behind for the lifetime of the client.Modification:
Isolate waiter callbacks in a
notifyHttp2ConnectionWaitertry/catch helper. Add awaitersClosedflag that is latched before the shutdown sweep so new waiters are rejected during shutdown, allowingarm()to fail the request immediately without interacting with the timer. Remove empty waiter sets usingcomputeIfPresent. Added four unit tests covering these scenarios.Result:
Misbehaving waiters can no longer disrupt connection establishment or leak permits, waiters registered during shutdown fail deterministically instead of hanging, and the registry no longer retains empty per-host entries.