clientconn: Wait for all goroutines on close #8666

twz123 · 2025-10-21T20:34:32Z

Three goroutines could outlive a call to ClientConn.close(). Add mechanics to cancel them and wait for them to complete when closing a client connection.

Fixes #8655.

RELEASE NOTES:

Closing a client connection will cancel all pending goroutines and block until they complete.

codecov · 2025-10-21T20:38:20Z

Codecov Report

❌ Patch coverage is 91.04478% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.22%. Comparing base (50c6321) to head (3fa606c).

Files with missing lines	Patch %	Lines
clientconn.go	84.21%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8666      +/-   ##
==========================================
+ Coverage   83.21%   83.22%   +0.01%     
==========================================
  Files         419      419              
  Lines       32427    32468      +41     
==========================================
+ Hits        26985    27023      +38     
- Misses       4054     4058       +4     
+ Partials     1388     1387       -1

Files with missing lines	Coverage Δ
balancer_wrapper.go	`85.34% <100.00%> (+1.77%)`	⬆️
internal/balancer/gracefulswitch/gracefulswitch.go	`87.86% <100.00%> (+1.39%)`	⬆️
internal/testutils/pipe_listener.go	`85.71% <100.00%> (+1.93%)`	⬆️
clientconn.go	`90.06% <84.21%> (-0.08%)`	⬇️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

eshitachandwani · 2025-11-04T09:04:58Z

balancer_wrapper.go

 	healthData *healthData
+
+	shutdownMu    sync.Mutex
+	shutdown      chan struct{}


Could you change this to shutdownCh to indicate it is a channel?

eshitachandwani · 2025-11-04T19:52:42Z

balancer_wrapper.go


 func (acbw *acBalancerWrapper) UpdateAddresses(addrs []resolver.Address) {
-	acbw.ac.updateAddrs(addrs)
+	acbw.goFunc(func(shutdown <-chan struct{}) {


Is there a reason we changed this to run the acbw.ac.updateAddrs(addrs) function in a go routine??

It's basically a "bubbled-up" goroutine. Previously, the goroutine was spawned in updateAddrs itself (line 1021). But, as we now need to track those, I figured it would be most appropriate to do it here. Another option would be to somehow push this down into updateAddrs itself, by passing the acBalancerWrapper pointer, or a function pointer to acbw.goFunc or sth. along those lines, and then use that to spawn the goroutine there:

Suggested change

acbw.goFunc(func(shutdown <-chan struct{}) {

acbw.ac.updateAddrs(acbw, addrs)

Then we could write line 1021 of updateAddrs like so:

acbw.goFunc(ac.resetTransportAndUnlock)

I've changed it this way. Seems to be clearer.

eshitachandwani · 2025-11-05T09:46:04Z

clientconn.go

-	ac.mu.Lock()
-	defer ac.mu.Unlock()
+	acMu := &ac.mu
+	acMu.Lock()


Is there a reason the original code did not work? this seems unnecessarily complicated and does the same thing. Or am I missing something.

It's a way to make the defer do the unlock conditionally, as the code might need to unlock it before returning (see lines 1441 and 1442). We can achieve the same e.g. with a boolean variable, if you prefer.

I think using a pointer for this is good, because, if you forget the nil check, it will panic with an easy to understand stack trace. Whereas if you forget to check a boolean, you'd do a double-unlock and there's a chance that you get weirder problems.

easwars · 2025-11-13T22:30:13Z

internal/balancer/gracefulswitch/gracefulswitch.go

+	gsb.pendingSwaps.Add(1)
 	go func() {
+		defer gsb.pendingSwaps.Done()


How about using the newly added Go method on the sync.WaitGroup to run goroutines on it?

This would require a Go bump to 1.25. The go.mod file still states go 1.24.0.

easwars · 2025-11-13T22:31:16Z

internal/balancer/gracefulswitch/gracefulswitch.go

 	// balancerCurrent.
 	currentMu sync.Mutex
+
+	pendingSwaps sync.WaitGroup


Can we name this something else? This is actually waiting for the swap, but for the swapped-out balancer to be closed. I can't think of something nice. If you can't come up with something nicer either, then a comment would be useful. Thanks.

I went with a generic "activeGoroutines" and added a comment.

easwars · 2025-11-13T22:35:27Z

internal/balancer/gracefulswitch/gracefulswitch.go

 	cur := gsb.balancerCurrent
 	gsb.balancerCurrent = gsb.balancerPending
 	gsb.balancerPending = nil
+	gsb.pendingSwaps.Add(1)


While you are here, would you mind adding a comment to balancerWrapper.UpdateState (which is the only place from which swap is called), where it is currently checking if the update if from the current or pending balancer, and returning early if that is not the case.

I'm talking about this check:

grpc-go/internal/balancer/gracefulswitch/gracefulswitch.go

Line 327 in c0de489

if !bw.gsb.balancerCurrentOrPending(bw) {

This check ensures that when there is a race between UpdateState and Close, and if Close wins that race and runs first, UpdateState does not actually swap balancers. This guarantees that swap does not spawn a goroutine after the gracefulswitch balancer is closed. So, if we add a comment near that check explaining this, that would be great.

Thanks.

Tried to add something helpful there.

Three goroutines could outlive a call to ClientConn.close(). Add mechanics to cancel them and wait for them to complete when closing a client connection. RELEASE NOTES: - Closing a client connection will cancel all pending goroutines and block until they complete. Signed-off-by: Tom Wieczorek <[email protected]>

eshitachandwani added this to the 1.77 Release milestone Oct 22, 2025

eshitachandwani added the Type: Bug label Oct 22, 2025

eshitachandwani self-requested a review October 22, 2025 08:26

eshitachandwani self-assigned this Oct 22, 2025

eshitachandwani requested a review from easwars October 27, 2025 07:45

eshitachandwani assigned easwars Oct 27, 2025

arjan-bal modified the milestones: 1.77 Release, 1.78 Release Oct 30, 2025

eshitachandwani reviewed Nov 5, 2025

View reviewed changes

eshitachandwani assigned twz123 and unassigned eshitachandwani Nov 5, 2025

twz123 force-pushed the clientconn-close-waitfor-goroutines branch from e45e9d7 to e8bb2b4 Compare November 5, 2025 12:28

easwars reviewed Nov 13, 2025

View reviewed changes

twz123 force-pushed the clientconn-close-waitfor-goroutines branch from e8bb2b4 to 3fa606c Compare November 20, 2025 08:10

	acbw.goFunc(func(shutdown <-chan struct{}) {
	acbw.ac.updateAddrs(acbw, addrs)

clientconn: Wait for all goroutines on close #8666

Are you sure you want to change the base?

clientconn: Wait for all goroutines on close #8666

Uh oh!

Conversation

twz123 commented Oct 21, 2025

Uh oh!

codecov bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 21, 2025 •

edited

Loading