control/controlclient: restore aggressive Direct.Close teardown #17416
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
In the earlier http2 package migration (1d93bdc, #17394) I had removed Direct.Close's tracking of the connPool, thinking it wasn't necessary. Some tests (in another repo) are strict and like it to tear down the world and wait, to check for leaked goroutines. And they caught this letting some goroutines idle past Close, even if they'd eventually close down on their own. This restores the connPool accounting and the aggressife close. Updates #17305 Updates #17394 Change-Id: I5fed283a179ff7c3e2be104836bbe58b05130cc7 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
creachadair
approved these changes
Oct 3, 2025
|
|
||
| ncc := NewConn(clientConn.Conn) | ||
|
|
||
| nc.mu.Lock() |
There was a problem hiding this comment.
The lock discipline here is not new, but is very cursed. 😭
There was a problem hiding this comment.
hey, compared to LocalBackend this is straight up normal
There was a problem hiding this comment.
not sure why there's a defer there, though, before the return. Kinda useless.
There was a problem hiding this comment.
hey, compared to LocalBackend this is straight up normal
You're absolutely right but that is a very very low bar 😂
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.
In the earlier http2 package migration (1d93bdc, #17394) I had
removed Direct.Close's tracking of the connPool, thinking it wasn't
necessary.
Some tests (in another repo) are strict and like it to tear down the
world and wait, to check for leaked goroutines. And they caught this
letting some goroutines idle past Close, even if they'd eventually
close down on their own.
This restores the connPool accounting and the aggressife close.
Updates #17305
Updates #17394