fix: memory leak on account of pending dials #790
Merged
+38
−17
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
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
ipochi
force-pushed
the
imran/fix-mem-leak-pending-dials
branch
from
82146cd to
1ccdfbe
Compare
k8s-ci-robot
added
size/M
cheftako
reviewed
Oct 17, 2025
pkg/server/tunnel.go
Outdated
| klog.ErrorS(err, "no tunnels available") | ||
| conn.Write([]byte(fmt.Sprintf("HTTP/1.1 500 Internal Server Error\r\nContent-Type: text/plain\r\n\r\ncurrently no tunnels available: %v", err))) | ||
| conn.Close() | ||
| // conn.Close() is handled by the top-level defer |
Contributor
There was a problem hiding this comment.
(non blocking) Can we fix the comment to indicate is should be closed by the closeOnce above?
cheftako
reviewed
Oct 17, 2025
| established := false | ||
| defer func() { | ||
| if !established { | ||
| if t.Server.PendingDial.Remove(random) != nil { |
Contributor
There was a problem hiding this comment.
A little concerned that established does not guarantee that random is in PendingDial. If we got a response (DIAL_CLS or DIAL_RSP) but the select below goes to a different case than connection.connected, established will be false but random will be absent from PendingDial and I do not think Remove() is resilient to that case.
Contributor
Author
There was a problem hiding this comment.
@cheftako I hear your concern but Remove is using delete which treats removal of a non-existent entry as a no-op
Contributor
Author
|
/test pull-apiserver-network-proxy-test-master |
This commit fixes a goroutine leak in the HTTP-Connect tunnel (tunnel.go) that could occur during connection setup. The leak happened when a backend agent disconnected at a very specific time: after the server sent a DIAL_REQ but before the connection was fully established. In this scenario, the cleanup logic was never called, and the handler goroutine would hang forever. I've refactored ServeHTTP to make it more robust and prevent this leak: Added a deferred cleanup function: A defer block now acts as a safety net. It uses a flag (established) to track whether the connection succeeded. If the function exits for any reason before the connection is established, this deferred code runs and guarantees that the pending dial is removed from our tracking map. Fixed a race condition with a single select: The old code had separate, racy checks for different failure modes. I've replaced this with a single, atomic select block that waits for all possible outcomes at once: a successful connection, the client disconnecting, or the agent's context being cancelled. This makes the logic much safer and easier to follow. Signed-off-by: Imran Pochi <[email protected]>
ipochi
force-pushed
the
imran/fix-mem-leak-pending-dials
branch
from
61b8654 to
e12c96f
Compare
Contributor
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako, ipochi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ipochi
pushed a commit
to kinvolk/apiserver-network-proxy
that referenced
this pull request
Oct 30, 2025
…k-pending-dials fix: memory leak on account of pending dials
ipochi
pushed a commit
to kinvolk/apiserver-network-proxy
that referenced
this pull request
Oct 30, 2025
…k-pending-dials fix: memory leak on account of pending dials
ipochi
pushed a commit
to kinvolk/apiserver-network-proxy
that referenced
this pull request
Oct 30, 2025
…k-pending-dials fix: memory leak on account of pending dials
This was referenced Oct 30, 2025
k8s-ci-robot
added a commit
that referenced
this pull request
Oct 31, 2025
Backporting #790 to release-0.32 from kinvolk/imran/fix-mem-leak-pending-dials
k8s-ci-robot
added a commit
that referenced
this pull request
Oct 31, 2025
Backporting #790 to release-0.31 from kinvolk/imran/fix-mem-leak-pending-dials
k8s-ci-robot
added a commit
that referenced
this pull request
Oct 31, 2025
Backporting #790 to release-0.33 from kinvolk/imran/fix-mem-leak-pending-dials
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Fixes: #789