delegatingresolver: Stop calls into delegates once the parent resolver is closed. #8195
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8195 +/- ##
==========================================
+ Coverage 82.06% 82.08% +0.02%
==========================================
Files 410 410
Lines 40233 40233
==========================================
+ Hits 33018 33027 +9
+ Misses 5854 5848 -6
+ Partials 1361 1358 -3
🚀 New features to boost your workflow:
|
arjan-bal
changed the title
arjan-bal
force-pushed
the
fix-delegating-resolver-race
branch
from
f593348 to
d62e1ce
Compare
| @@ -93,6 +97,11 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti | |||
| r := &delegatingResolver{ | |||
| target: target, | |||
| cc: cc, | |||
| // Child resolvers may send a state update as soon as they're built. | |||
| // Initialize children with no-op resolvers. When both children are nil, | |||
| // the delegatingResolver is considered closed. | |||
There was a problem hiding this comment.
Related: do we not need to hold childMu when calling Build?
There was a problem hiding this comment.
Yes, we need to. Updates from one child can trigger ResolveNow calls in the other child. This also avoids the need to initialize children here. Added code to acquire the mutex and release it.
|
|
||
| // childMu serializes calls into child resolvers. It also protects access to | ||
| // the following fields. | ||
| childMu sync.Mutex |
There was a problem hiding this comment.
Can we not share mu for this? Why not?
There was a problem hiding this comment.
childMu is being used to serialize calls into the child resolvers, similar to the serailzer queue in resolver_wrapper.go.
If we re-use the existing mutex, we would need to hold mu while calling childResolver.ResolveNow(). If this calls cc.UpdateState() synchronously, it will try to re-acquire mu and deadlock.
muis used to serializeresolver -> channelcalls.childMuis used to serializechannel -> resolvercalls.
| go func() { | ||
| r.childMu.Lock() | ||
| defer r.childMu.Unlock() | ||
| if !r.isClosedLocked() { |
There was a problem hiding this comment.
Probably a nit, but IMO it is simpler to delete the helper and here just check if r.targetResolver != nil since that's what really matters.
There was a problem hiding this comment.
Deleted the helper and added a check.
| go func() { | ||
| r.childMu.Lock() | ||
| defer r.childMu.Unlock() | ||
| if !r.isClosedLocked() { |
There was a problem hiding this comment.
Removed the helper function.
arjan-bal
changed the title
arjan-bal
added
Area: Resolvers/Balancers
Fixes: #8168
While handling updates from child resolvers, the delegating resolver locks it's mutex. If the channel returns an error on updating the state, the delegating resolver calls
ResolveNowon the other child. This can result in a deadlock if the other child attempts to callcc.UpdateStatesynchronously while handling theResolveNowcall. This change makes delegatingResovler callResolveNowin a new goroutine to avoid such deadlocks.This change introduced a second mutex in delegating resolver to serialize calls into child resolvers. This ensures no calls to
ResolveNoware made after the child resolvers are closed.RELEASE NOTES:
UpdateStatesynchronously while handling aResolveNowcall.