Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jul 9, 2024
Merged

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c3a3d1c
Update conn state to prevent concurrent connection attempts
arjan-bal Jul 4, 2024
6214c9d
Make callers of resetBackoff() lock the mutex
arjan-bal Jul 8, 2024
ff977b3
Add doc comment for resetTransportAndUnlock
arjan-bal Jul 9, 2024
76ef33f
Merge remote-tracking branch 'source/master' into fix_conn_connect_race
arjan-bal Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions clientconn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,9 +918,8 @@ func (ac *addrConn) connect() error {
ac.mu.Unlock()
return nil
}
ac.mu.Unlock()

ac.resetTransport()
ac.resetTransportAndUnlock()
return nil
}

Expand Down Expand Up @@ -992,11 +991,9 @@ func (ac *addrConn) updateAddrs(addrs []resolver.Address) {
ac.updateConnectivityState(connectivity.Idle, nil)
}

ac.mu.Unlock()

// Since we were connecting/connected, we should start a new connection
// attempt.
go ac.resetTransport()
go ac.resetTransportAndUnlock()
}

// getServerName determines the serverName to be used in the connection
Expand Down Expand Up @@ -1231,8 +1228,10 @@ func (ac *addrConn) adjustParams(r transport.GoAwayReason) {
}
}

func (ac *addrConn) resetTransport() {
ac.mu.Lock()
// resetTransportAndUnlock unconditionally connects the addrConn.
//
// ac.mu must be held by the caller, and this function will guarantee it is released.
func (ac *addrConn) resetTransportAndUnlock() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the doc comment.

Copy link
Contributor Author

@arjan-bal arjan-bal Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@purnesh42H purnesh42H Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

acCtx := ac.ctx
if acCtx.Err() != nil {
ac.mu.Unlock()
Expand Down
Loading