grpc: hold ac.mu while calling resetTransport to prevent concurrent connection attempts #7390

arjan-bal · 2024-07-04T07:51:36Z

This change ensures that caller of resetTransport() keep holding the mutex instead of releasing it and having resetTransport() re-acquire it. This ensures that no concurrent requests are able to start once caller of resetTransport does some validation and calls resetTransport.

See #7365 (comment) for more details.

Tested

Verified that Test/AuthorityRevive no longer flakes for 100000 attempts with the change.

Fixes: #7365

RELEASE NOTES:

client: fix race that could lead to orphaned connections and associated resources.

codecov · 2024-07-04T07:54:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.35%. Comparing base (daab563) to head (76ef33f).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7390      +/-   ##
==========================================
- Coverage   81.51%   81.35%   -0.17%     
==========================================
  Files         348      348              
  Lines       26744    26741       -3     
==========================================
- Hits        21801    21754      -47     
- Misses       3764     3793      +29     
- Partials     1179     1194      +15

Files	Coverage Δ
clientconn.go	`92.40% <100.00%> (-0.99%)`	⬇️

... and 23 files with indirect coverage changes

purnesh42H · 2024-07-04T08:17:59Z

clientconn.go

@@ -918,6 +918,9 @@ func (ac *addrConn) connect() error {
 		ac.mu.Unlock()
 		return nil
 	}
+	// Update the state to ensure no concurrent requests start once the lock
+	// is released.
+	ac.updateConnectivityState(connectivity.Connecting, nil)


We should probably file a bug for this otherwise it will be a behavior change?

Can you look at #7365 (comment) for the details of this bug?

What change in behaviour are you concerned about?

Oh I was just asking with respect to release notes because currently the bug points to test flake. Anyways, I just checked it doesn't matter because release notes refer to the fix PR and not the issue. Although in the release notes, we should prefix the package

balancer: Fix race condition that could lead to multiple transports being created in parallel

Also, it looks like without your fix, there is a case where resetTransport can error out and return without updating the connectivity state

grpc-go/clientconn.go

Line 1237 in bdd707e

if acCtx.Err() != nil {

May be we can make the resetTransport() in the same critical section instead of releasing lock and aquiring again in resetTransport()?

I don't see any benefit in adding the acCtcx.Err check in connect because even resetTransport sets the the state to Connecting and releases the lock:

grpc-go/clientconn.go

Lines 1262 to 1263 in bdd707e

ac.updateConnectivityState(connectivity.Connecting, nil)

ac.mu.Unlock()

This means that the context can be cancelled (and subsequently addrConn shutdown) after the channel is in connecting state even without the change.

IIUC we just need to ensure that we don't set connecting state after the channel enters shutdown.

The test for shutdown state on top should be enough protection to ensure shutdown state comes only after we enter connecting.

May be we can make the resetTransport() in the same critical section instead of releasing lock and aquiring again in resetTransport()?

We could rename resetTransport to resetTransportLocked and expect the callers to hold the lock while calling this method. However, resetTransport releases the lock temporarily. Add to this that ac.updateAddrs calls resetTransport in a new go routine so it can't hold the lock till resetTransport completes. It feels a little risky to make that change. I don't know for sure, but I feel we could end up in a situation where the lock is not released correctly resulting in a deadlock.

I don't want do make that change as the first option.

I don't see any benefit in adding the acCtcx.Err check in connect because even resetTransport sets the the state to Connecting and releases the lock

From the code it looks like in case of acCtcx.Err, resetTransport() doesn't update the state and return so state will be still idle but after your fix in case of acCtcx.Err state will be updated to connecting. Am I missing something?

From my understanding, ac.ctx is used to control the creation of remote connections while ac.state is used to synchronize all the state transitions for the addrConn. ac.connect() doesn't deal with creating remote connections, so it doesn't need to check ac.ctx.Err(). It needs to ensure the transition to Connecting is valid, which it does by locking the mutex and verifying that ac.state != Shutdown

ac.ctx is used to avoid doing throw away work which takes significant time (creating a remote conn).

Please let me know if your understanding is different.

Discussed offline: it doesn't matter if resetTransport() returns error after state being updated to connecting

purnesh42H · 2024-07-05T04:33:55Z

Release notes needs to be prefixed with "package name:" in this case balancer:?

arjan-bal · 2024-07-05T06:32:57Z

Release notes needs to be prefixed with ":" in this case balancer:?

This is a change in the outermost grpc package. Added the package name in the release notes.

purnesh42H · 2024-07-08T10:15:53Z

is Test/ServerSideXDS_WithValidAndInvalidSecurityConfiguration failure related to change or just a flake?

arjan-bal · 2024-07-08T10:48:16Z

is Test/ServerSideXDS_WithValidAndInvalidSecurityConfiguration failure related to change or just a flake?

It's a flake, there's an existing issue for this and I've commented on the issue about this failure: #6914

purnesh42H

lgtm

dfawley · 2024-07-08T19:39:58Z

RELEASE NOTES:

Fix race condition that could lead to multiple transports being created in parallel.

What is the user-visible symptom here? A memory leak? Or just an extra connection that was attempted, but that will quickly go away on its own anyway? (Or will the extra connection stick around until the channel is closed?)

dfawley · 2024-07-08T19:42:18Z

clientconn.go

@@ -1231,8 +1228,7 @@ func (ac *addrConn) adjustParams(r transport.GoAwayReason) {
 	}
 }

-func (ac *addrConn) resetTransport() {
-	ac.mu.Lock()
+func (ac *addrConn) resetTransportAndUnlock() {


Please add a short comment here:

// resetTransportAndUnlock unconditionally connects the addrConn. // // ac.mu must be held by the caller, and this function will guarantee it is released.

ac.mu must be held by the caller, and this function will guarantee it is released

should we have code check for this as well?

Add the doc comment.

should we have code check for this as well?

We don't verify if the mutex is locked in other functions in the code which assume the caller has the lock. These functions have the suffix Locked in the name.

There is a TryLock() method on sync/mutex, but its use is discouraged.

resetTransportAndUnlock is a private method and we run tests with race detector so we can catch incorrect usage.

When the resetTransportAndUnlock method tries to unlock a mutex that isn't locked, it will panic.

i meant just having a doc doesn't enforce the mutex should be locked. Unlocking a mutex that is not locked in Go will result in a runtime panic.

Can you suggest how to enforce the locking?
How is the caller expected to handle the error if resetTransportAndUnlock doesn't panic?
Since its a private method, doesn't a panic make the failure more visible and ensure incorrect usages are caught by tests?

yeah it will probably require to implement custom mutex with some boolean field but since we have precedences of methods (e.g https://github.com/grpc/grpc-go/blob/master/clientconn.go#L728) unlocking mutex, devs will be aware of these

I think the name of the function and the comment should be sufficient for this.

arjan-bal · 2024-07-09T06:38:44Z

What is the user-visible symptom here? A memory leak? Or just an extra connection that was attempted, but that will quickly go away on its own anyway? (Or will the extra connection stick around until the channel is closed?)

What I'm seeing in the test is that only one transport is closed when the subConn is updated (ac.transport) and the other transport is orphaned. The orphaned transports get closed when the server is shutdown at the end of the test. Updated the release notes.

…onnection attempts (grpc#7390)

Update conn state to prevent concurrent connection attempts

c3a3d1c

arjan-bal added the Type: Bug label Jul 4, 2024

arjan-bal added this to the 1.66 Release milestone Jul 4, 2024

arjan-bal changed the title ~~grpc: Update conn state to prevent concurrent connection attempts~~ grpc: Update conn state early to prevent concurrent connection attempts Jul 4, 2024

arjan-bal requested review from purnesh42H and dfawley July 4, 2024 07:58

arjan-bal assigned purnesh42H Jul 4, 2024

purnesh42H reviewed Jul 4, 2024

View reviewed changes

arjan-bal requested a review from purnesh42H July 4, 2024 12:02

purnesh42H approved these changes Jul 8, 2024

View reviewed changes

arjan-bal assigned dfawley and arjan-bal and unassigned purnesh42H and dfawley Jul 8, 2024

arjan-bal changed the title ~~grpc: Update conn state early to prevent concurrent connection attempts~~ grpc: Hold mutex while calling resetTransport to prevent concurrent connection attempts Jul 8, 2024

Make callers of resetBackoff() lock the mutex

6214c9d

arjan-bal force-pushed the fix_conn_connect_race branch from 375bdd7 to 6214c9d Compare July 8, 2024 19:18

arjan-bal assigned dfawley and unassigned arjan-bal Jul 8, 2024

dfawley approved these changes Jul 8, 2024

View reviewed changes

Add doc comment for resetTransportAndUnlock

ff977b3

Merge remote-tracking branch 'source/master' into fix_conn_connect_race

76ef33f

dfawley changed the title ~~grpc: Hold mutex while calling resetTransport to prevent concurrent connection attempts~~ grpc: hold ac.mu while calling resetTransport to prevent concurrent connection attempts Jul 9, 2024

dfawley merged commit 45d44a7 into grpc:master Jul 9, 2024
12 of 13 checks passed

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 11, 2024

grpc: hold ac.mu while calling resetTransport to prevent concurrent c…

d33bc20

…onnection attempts (grpc#7390)

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024

grpc: hold ac.mu while calling resetTransport to prevent concurrent c…

e21a4c0

…onnection attempts (grpc#7390)

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024

grpc: hold ac.mu while calling resetTransport to prevent concurrent c…

86dd3a8

…onnection attempts (grpc#7390)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc: hold ac.mu while calling resetTransport to prevent concurrent connection attempts #7390

grpc: hold ac.mu while calling resetTransport to prevent concurrent connection attempts #7390

arjan-bal commented Jul 4, 2024 •

edited by dfawley

Loading

codecov bot commented Jul 4, 2024 •

edited

Loading

purnesh42H Jul 4, 2024

arjan-bal Jul 4, 2024

purnesh42H Jul 4, 2024

purnesh42H Jul 4, 2024

arjan-bal Jul 4, 2024

arjan-bal Jul 4, 2024 •

edited

Loading

purnesh42H Jul 5, 2024

arjan-bal Jul 5, 2024

purnesh42H Jul 8, 2024 •

edited

Loading

purnesh42H commented Jul 5, 2024 •

edited

Loading

arjan-bal commented Jul 5, 2024

purnesh42H commented Jul 8, 2024

arjan-bal commented Jul 8, 2024

purnesh42H left a comment

dfawley commented Jul 8, 2024

dfawley Jul 8, 2024

purnesh42H Jul 9, 2024

arjan-bal Jul 9, 2024

arjan-bal Jul 9, 2024 •

edited

Loading

purnesh42H Jul 9, 2024 •

edited

Loading

arjan-bal Jul 9, 2024

purnesh42H Jul 9, 2024

dfawley Jul 9, 2024

arjan-bal commented Jul 9, 2024 •

edited

Loading

	ac.updateConnectivityState(connectivity.Connecting, nil)
	ac.mu.Unlock()

grpc: hold ac.mu while calling resetTransport to prevent concurrent connection attempts #7390

grpc: hold ac.mu while calling resetTransport to prevent concurrent connection attempts #7390

Conversation

arjan-bal commented Jul 4, 2024 • edited by dfawley Loading

Tested

codecov bot commented Jul 4, 2024 • edited Loading

Codecov Report

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

arjan-bal Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

purnesh42H commented Jul 5, 2024 • edited Loading

arjan-bal commented Jul 5, 2024

purnesh42H commented Jul 8, 2024

arjan-bal commented Jul 8, 2024

purnesh42H left a comment

Choose a reason for hiding this comment

dfawley commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjan-bal Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

purnesh42H Jul 9, 2024 • edited Loading

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

arjan-bal commented Jul 9, 2024 • edited Loading

arjan-bal commented Jul 4, 2024 •

edited by dfawley

Loading

codecov bot commented Jul 4, 2024 •

edited

Loading

arjan-bal Jul 4, 2024 •

edited

Loading

purnesh42H Jul 8, 2024 •

edited

Loading

purnesh42H commented Jul 5, 2024 •

edited

Loading

arjan-bal Jul 9, 2024 •

edited

Loading

purnesh42H Jul 9, 2024 •

edited

Loading

arjan-bal commented Jul 9, 2024 •

edited

Loading