Repository navigation
fix: close the socket when the metadata exchange fails - #568
Merged
Merged
Conversation
This was referenced Sep 18, 2026
enocom
added this pull request to stack #577
September 19, 2026 04:15
enocom
force-pushed
the
telemetry-3-classify-failures
branch
2 times, most recently
from
September 22, 2026 02:58
1aeb14f to
37fef56
Compare
enocom
marked this pull request as ready for review
September 22, 2026 03:00
The metadata exchange runs on an established TLS socket that nothing outside metadata_exchange holds a reference to. A failure there — a socket timeout, a short read, a malformed response, a token refresh error — left that socket open until the garbage collector reclaimed it, so a caller retrying a failing dial in a loop accumulated connections it could not close. ctx.wrap_socket needs no equivalent handling: it detaches the raw socket and closes the file descriptor itself if the handshake fails.
enocom
changed the title
enocom
force-pushed
the
telemetry-3-classify-failures
branch
from
September 23, 2026 03:55
37fef56 to
b24ad19
Compare
Member
Author
|
@rhatgadkar-goog I agree the metadata exchange should remain a private implementation detail. As I started working through this, I realized there's a simpler way to capture the dial result (the various failures and success). So in the meantime, I've converted this into a fix of a problem I discovered -- failed metadata exchanges don't close the socket. This is both a good bug fix and also sets up later PRs in the chain to capture the failure type. PTAL. |
rhatgadkar-goog
approved these changes
Sep 23, 2026
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
2 participants
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 metadata exchange runs on an established TLS socket that nothing
outside
metadata_exchangeholds a reference to. A failure there — asocket timeout, a short read, a malformed response, a token refresh
error — left that socket open until the garbage collector reclaimed it,
so a caller retrying a failing dial in a loop accumulated connections it
could not close.
ctx.wrap_socketneeds no equivalent handling: it detaches the raw socketand closes the file descriptor itself if the handshake fails
(
ssl.SSLSocket._createends in a bareexcept:that callsself.close()).Related to #449.