clientconn: Wait for all goroutines on close #8666
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8666 +/- ##
==========================================
- Coverage 83.38% 82.87% -0.52%
==========================================
Files 418 418
Lines 32367 32423 +56
==========================================
- Hits 26988 26869 -119
- Misses 4014 4048 +34
- Partials 1365 1506 +141
🚀 New features to boost your workflow:
|
balancer_wrapper.go
Outdated
| healthData *healthData | ||
|
|
||
| shutdownMu sync.Mutex | ||
| shutdown chan struct{} |
There was a problem hiding this comment.
Could you change this to shutdownCh to indicate it is a channel?
balancer_wrapper.go
Outdated
|
|
||
| func (acbw *acBalancerWrapper) UpdateAddresses(addrs []resolver.Address) { | ||
| acbw.ac.updateAddrs(addrs) | ||
| acbw.goFunc(func(shutdown <-chan struct{}) { |
There was a problem hiding this comment.
Is there a reason we changed this to run the acbw.ac.updateAddrs(addrs) function in a go routine??
There was a problem hiding this comment.
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:
| 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)There was a problem hiding this comment.
I've changed it this way. Seems to be clearer.
| ac.mu.Lock() | ||
| defer ac.mu.Unlock() | ||
| acMu := &ac.mu | ||
| acMu.Lock() |
There was a problem hiding this comment.
Is there a reason the original code did not work? this seems unnecessarily complicated and does the same thing. Or am I missing something.
There was a problem hiding this comment.
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.
twz123
force-pushed
the
clientconn-close-waitfor-goroutines
branch
from
e45e9d7 to
e8bb2b4
Compare
twz123
force-pushed
the
clientconn-close-waitfor-goroutines
branch
from
e8bb2b4 to
3fa606c
Compare
|
@twz123 Apologies for how long this review has dragged on, but hopefully once these are split into smaller ones, we should be able to move faster. Thanks for your understanding. |
Goroutines spawned during balancer swaps could outlive the call to Balancer.Close(). Monitor these via a wait group and wait for them to finish before returning from Close(). This prevents any noticeable side effects that could otherwise occur after Close() returns. RELEASE NOTES: - Closing a graceful switch balancer will now block until all pending goroutines complete. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
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 <twieczorek@mirantis.com>
twz123
force-pushed
the
clientconn-close-waitfor-goroutines
branch
from
3fa606c to
00deb21
Compare
Done: #8746
No worries. I understand that these things take time. |
4 participants
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: