Skip to content

ensure responseChannel is drained#660

Merged
koonpeng merged 2 commits into
mainfrom
koonpeng/fix-client-leak
Apr 16, 2026
Merged

ensure responseChannel is drained#660
koonpeng merged 2 commits into
mainfrom
koonpeng/fix-client-leak

Conversation

@koonpeng
Copy link
Copy Markdown
Contributor

There is a potential for deadlocks

if err != nil {
slog.Error("Closing backend connection",
slog.String("ID", *resp.Id), ilog.Err(err))
break
}

if we exit the loop preemptively, we fail to fully consume responseChannel and

might deadlock.

#222 fixes the error handling which may have caused the deadlock. Before, it checks for incorrect backoff.PermanentError which never happens so the responseChannel is always fully consumed.

Signed-off-by: Teo Koon Peng <koonpeng@google.com>
if err != nil {
slog.Error("Closing backend connection",
slog.String("ID", *resp.Id), ilog.Err(err))
// This is also closed in streamToBackend. In most cases this is safe but it depends on the transport.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add more detail for which transport this Close() call would be needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "default" http client doesn't panic. I didn't really tested if it would panic in our case.
In theory, we can also not close here. The server should eventually close the connection, then streamBackend will end up closing hresp.Body. Closing it here would just propagate the disconnect to the backend faster.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks maybe reword as:
// This is also closed in streamToBackend. Closing here too to ensure the disconnect propagates faster in case e.g. the default http client is used.

Copy link
Copy Markdown
Contributor

@ensonic ensonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, thanks & +1

@ensonic ensonic self-requested a review April 16, 2026 07:15
Signed-off-by: Teo Koon Peng <koonpeng@google.com>
@koonpeng koonpeng merged commit 7a4c02f into main Apr 16, 2026
7 checks passed
@koonpeng koonpeng deleted the koonpeng/fix-client-leak branch April 16, 2026 08:20
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.

2 participants