client: rework resolver and balancer wrappers to avoid deadlock #6804

dfawley · 2023-11-16T06:06:13Z

Balancer wrapper is now recreated for each trip out of idle mode (instead of supporting its own idle mode).
Removed mutexes or limited as much as possible.
All calls into resolver/balancer are done in serializer, with no mutexes held.
Calls from old resolver instances are ignored.
ExitIdleCond is no longer needed by closing the idlenessMgr at the start of Close.
Constructors figure out their own parameters from cc instead of being passed in.
Handle resolver.Build->cc.updateResolverState->balancer.UpdateState->resolver.ResolveNow properly
Idle Manager starts in idle state.
All channel idleness management goes through idle manager -- e.g. Connect().
Add lock and closed state to balancer_wrapper to prevent a race where the
balancer creates a subchannel and we enter idle mode during that process.
Split resolver wrapper into constructor and start method so the serializer is
available during resolver.Build.
Add EnterIdleModeForTesting to idleness manager and use idlenessMgr methods
in the idleness test instead of methods on the ClientConn which should not be
called directly.

RELEASE NOTES:

client: fix race that could cause a deadlock while entering idle mode and receiving a name resolver update

codecov · 2023-11-16T06:08:54Z

Codecov Report

Merging #6804 (a4ef6d7) into master (7935c4f) will increase coverage by 0.18%.
Report is 6 commits behind head on master.
The diff coverage is 85.97%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6804      +/-   ##
==========================================
+ Coverage   83.40%   83.59%   +0.18%     
==========================================
  Files         285      285              
  Lines       30879    30763     -116     
==========================================
- Hits        25754    25715      -39     
+ Misses       4053     3987      -66     
+ Partials     1072     1061      -11

Files	Coverage Δ
internal/channelz/funcs.go	`95.02% <100.00%> (+0.04%)`	⬆️
picker_wrapper.go	`88.88% <50.00%> (-1.81%)`	⬇️
internal/idle/idle.go	`88.40% <92.45%> (+26.18%)`	⬆️
clientconn.go	`92.20% <86.95%> (-0.67%)`	⬇️
balancer_wrapper.go	`82.92% <87.95%> (+2.26%)`	⬆️
resolver_wrapper.go	`85.04% <77.61%> (-9.60%)`	⬇️

... and 23 files with indirect coverage changes

- All channel idleness management goes through idle manager -- e.g. Connect(). - Add lock and closed state to balancer_wrapper to prevent a race where the balancer creates a subchannel and we enter idle mode during that process. - Split resolver wrapper into constructor and start method so the serializer is available during resolver.Build. - Add EnterIdleModeForTesting to idleness manager and use idlenessMgr methods in the idleness test instead of methods on the ClientConn which should not be called directly.

…ays exist

dfawley · 2023-11-17T05:29:57Z

@zasweq I may have misunderstood, so if @arvindbr8 can review this tomorrow then feel free to ignore. Otherwise if you could review this next week so we can have it done in time to cherry-pick it to the release branch, that would be great. Thanks!

zasweq

Did a style pass. Technically, I have a very minute understanding of the original Exit Idle code, and this fix. I think since Easwar is back next week he should be primary reviewer since he wrote the ExitIdle code. We should also have a technical sync where you briefly describe to us the original code, all the issues, the fix and why it overall fixes.

balancer_wrapper.go

resolver_wrapper.go

balancer_wrapper.go

clientconn.go

internal/idle/idle.go

resolver_wrapper.go

…orTesting

internal/idle/idle.go

easwars · 2023-11-28T20:59:58Z

@dfawley : I've only looked through the idleness code so far. But assigning back to you to make the requested changes while I will continue to review other changes.

dfawley

All comments address (I think!)

balancer_wrapper.go

internal/idle/idle.go

zasweq

LGTM module some nits

balancer_wrapper.go

clientconn.go

resolver_balancer_ext_test.go

balancer_wrapper.go

clientconn.go

… the ac being torn down

easwars

Minor comments.

easwars · 2023-12-05T00:18:39Z

balancer_wrapper.go

@@ -184,115 +168,49 @@ func (ccb *ccBalancerWrapper) buildLoadBalancingPolicy(name string) {
 	ccb.curBalancerName = builder.Name()
 }

+// close initiates async shutdown of the wrapper.  To determine the wrapper has
+// finished shutting down, the channel should block on ccb.serializer.Done()
+// without cc.mu held.


Nit: Could we also please mention here that the caller (channel) should hold cc.mu when invoking close().

easwars · 2023-12-05T00:19:44Z

balancer_wrapper.go

 // ccBalancerWrapper sits between the ClientConn and the Balancer.
 //
 // ccBalancerWrapper implements methods corresponding to the ones on the
 // balancer.Balancer interface. The ClientConn is free to call these methods
 // concurrently and the ccBalancerWrapper ensures that calls from the ClientConn
-// to the Balancer happen synchronously and in order.
+// to the Balancer happen in order by performing them in the serializer, without
+// any mutexes held.


We do hold ccb.mu in close(), and require that the caller hold cc.mu when calling into close(). Would it be better to have that spelled out here?

This comment is about how the calls into the plugin, themselves, are made without cc.mu held. Not that calls into ccb must not hold cc.mu.

Probably we should just document the functions. (Done, and it looks like the only one that matters either way is close, which needs cc.mu.)

easwars · 2023-12-05T00:22:31Z

balancer_wrapper.go

@@ -313,10 +231,6 @@ func (ccb *ccBalancerWrapper) RemoveSubConn(sc balancer.SubConn) {
 }

 func (ccb *ccBalancerWrapper) UpdateAddresses(sc balancer.SubConn, addrs []resolver.Address) {
-	if ccb.isIdleOrClosed() {


Why do we not want to check if the balancer is closed here? UpdateAddresses could lead to transport creation, right.

It's more relevant whether the SubConn/addrConn was closed and not whether the balancer was closed. And we do that check here:

grpc-go/clientconn.go

Line 1031 in 93389b7

if ac.state == connectivity.Shutdown ||

easwars · 2023-12-05T00:31:26Z

resolver_balancer_ext_test.go

@@ -0,0 +1,264 @@
+/*
+ *
+ * Copyright 2014 gRPC authors.


easwars · 2023-12-05T00:33:52Z

resolver_balancer_ext_test.go

+// the channel enters idle mode.
+func (s) TestEnterIdleDuringResolverUpdateState(t *testing.T) {
+	enterIdle := internal.EnterIdleModeForTesting.(func(*grpc.ClientConn))
+	const name = "testeidrus"


Nit: In each of these tests, could we use t.Name() as the resolver name instead? I think a shortened name could be very confusing for someone looking at a failed test.

I tried that originally, but it doesn't work because of case/slashes. I can fix it, though. Done.

…#6804)

client: rework resolver and balancer wrappers to avoid deadlock

aaee0c9

dfawley added the Type: Bug label Nov 16, 2023

dfawley added this to the 1.61 Release milestone Nov 16, 2023

dfawley requested a review from arvindbr8 November 16, 2023 06:06

dfawley assigned arvindbr8 Nov 16, 2023

dfawley added 4 commits November 16, 2023 13:50

remove commented code; ensure balancerWrapper and resolverWrapper alw…

2cbebb0

…ays exist

delete idle field fully

b81a0a4

fix idle time calculation

d3c7779

dfawley requested a review from zasweq November 17, 2023 05:27

dfawley assigned zasweq Nov 17, 2023

zasweq reviewed Nov 22, 2023

View reviewed changes

zasweq requested review from easwars and removed request for arvindbr8 November 22, 2023 19:31

zasweq assigned easwars and unassigned arvindbr8 and zasweq Nov 22, 2023

add test for resolver build error on second build; fix EnterIdleModeF…

63b2d0d

…orTesting

easwars reviewed Nov 28, 2023

View reviewed changes

internal/idle/idle.go Outdated Show resolved Hide resolved

internal/idle/idle.go Outdated Show resolved Hide resolved

internal/idle/idle.go Outdated Show resolved Hide resolved

internal/idle/idle.go Show resolved Hide resolved

internal/idle/idle.go Outdated Show resolved Hide resolved

easwars assigned dfawley and unassigned easwars Nov 28, 2023

review comments

7ae0904

dfawley commented Nov 28, 2023

View reviewed changes

dfawley assigned easwars and zasweq and unassigned dfawley Nov 28, 2023

zasweq approved these changes Nov 29, 2023

View reviewed changes

easwars removed their assignment Nov 30, 2023

atomic

42abdcd

dfawley assigned easwars and unassigned dfawley Nov 30, 2023

easwars reviewed Dec 1, 2023

View reviewed changes

resolver_balancer_ext_test.go Show resolved Hide resolved

easwars reviewed Dec 1, 2023

View reviewed changes

easwars assigned dfawley and unassigned easwars Dec 1, 2023

dfawley added 2 commits November 30, 2023 19:36

deadlock test

1b8d4b0

review comments

4696fae

dfawley assigned easwars and unassigned dfawley Dec 1, 2023

dfawley added 4 commits December 1, 2023 09:16

add tests for balancer operations after closure

944f2f5

stop calling acbw through cc in teardown; call directly into acbw for…

1d0c2b3

… the ac being torn down

more direct

bda70db

update err and test

16464a2

easwars approved these changes Dec 5, 2023

View reviewed changes

easwars assigned dfawley and unassigned easwars Dec 5, 2023

dfawley added 3 commits December 5, 2023 09:26

name and date

8918856

comment

e920eaf

idle_test fix and tweaks

a4ef6d7

dfawley merged commit 5d7453e into grpc:master Dec 5, 2023
14 checks passed

dfawley deleted the idlerace2 branch December 5, 2023 19:02

dfawley added a commit to dfawley/grpc-go that referenced this pull request Dec 7, 2023

client: rework resolver and balancer wrappers to avoid deadlock (grpc…

4357bac

…#6804)

dfawley modified the milestones: 1.61 Release, 1.60 Release Dec 7, 2023

dfawley added a commit that referenced this pull request Dec 7, 2023

Cherry-pick #6804 and dependencies to v1.60.x release branch (#6838)

cb6581d

nockty mentioned this pull request Dec 11, 2023

Downgrade grpc to version 1.58.2 planetscale/vtprotobuf#121

Merged

IvanLaktyunkin mentioned this pull request Dec 28, 2023

Potential GRPC-go deadlock between exit and enterIdleMode grpc/grpc#35398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: rework resolver and balancer wrappers to avoid deadlock #6804

client: rework resolver and balancer wrappers to avoid deadlock #6804

dfawley commented Nov 16, 2023 •

edited

codecov bot commented Nov 16, 2023 •

edited

dfawley commented Nov 17, 2023

zasweq left a comment

easwars commented Nov 28, 2023

dfawley left a comment

zasweq left a comment

easwars left a comment

easwars Dec 5, 2023

dfawley Dec 5, 2023

easwars Dec 5, 2023

dfawley Dec 5, 2023 •

edited

easwars Dec 5, 2023

dfawley Dec 5, 2023

easwars Dec 5, 2023

dfawley Dec 5, 2023

easwars Dec 5, 2023

dfawley Dec 5, 2023 •

edited

client: rework resolver and balancer wrappers to avoid deadlock #6804

client: rework resolver and balancer wrappers to avoid deadlock #6804

Conversation

dfawley commented Nov 16, 2023 • edited

codecov bot commented Nov 16, 2023 • edited

Codecov Report

dfawley commented Nov 17, 2023

zasweq left a comment

Choose a reason for hiding this comment

easwars commented Nov 28, 2023

dfawley left a comment

Choose a reason for hiding this comment

zasweq left a comment

Choose a reason for hiding this comment

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley Dec 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley Dec 5, 2023 • edited

Choose a reason for hiding this comment

dfawley commented Nov 16, 2023 •

edited

codecov bot commented Nov 16, 2023 •

edited

dfawley Dec 5, 2023 •

edited

dfawley Dec 5, 2023 •

edited