Skip to content

Guard wantConn.ready with a Mutex #1172

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

Closed
wants to merge 1 commit into from
Closed

Guard wantConn.ready with a Mutex #1172

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 12 additions & 2 deletions x/mongo/driver/topology/pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,7 @@ func (p *pool) createConnections(ctx context.Context, wg *sync.WaitGroup) {
// establishment so shutdown doesn't block indefinitely if connectTimeout=0.
err := conn.connect(ctx)
if err != nil {
w.readyMu.Lock()
w.tryDeliver(nil, err)

// If there's an error connecting the new connection, call the handshake error handler
Expand All @@ -855,6 +856,7 @@ func (p *pool) createConnections(ctx context.Context, wg *sync.WaitGroup) {

_ = p.removeConnection(conn, event.ReasonError)
_ = p.closeConnection(conn)
w.readyMu.Unlock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the goal is to remove the connection from the pool before returning an error to checkOut, can we move the call p.removeConnection to above w.tryDeliver instead of adding a mutex and a goroutine?

Copy link
Member Author

@prestonvasquez prestonvasquez Feb 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Doing this does lower the likelihood of encountering the race condition, but it doesn't eliminate it. If there are any handshake errors, then "clear" will be called from the "handshakeErrFn" here. The "handshakeErrFn" will call "tryDeliver" when emptying the idle connections wait queue here. Since this happens before the connection is closed, it will (probably) signal to the checkout function that the connection is "ready" before "removeConnection" finishes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding is that the goal of this change is to make sure that a "Connection closed" log is always sent before the corresponding "Connection checkout failed" log. Is that correct?

If so, I'm not I understand how calling p.removeConnection before w.tryDeliver could lead to any log race condition. Consider the following sequence of calls:

// Remove the connection from the pool, which emits a
// "Connection closed" event/log.
_ = p.removeConnection(conn, event.ReasonError)

// Deliver an error to the waiting checkOut, which will eventually emit a
// "Connection checkout failed" event/log in another goroutine.
w.tryDeliver(nil, err)

// Call the handshake error handler, which may clear the pool.
if p.handshakeErrFn != nil {
	p.handshakeErrFn(err, conn.generation, conn.desc.ServiceID)
}

// Actually close the underlying socket.
_ = p.closeConnection(conn)

By the time p.removeConnection returns, it should already have sent the "Connection closed" log message. There should be no way for the "Connection checkout failed" log message to happen before a "Connection closed" log message.

Copy link
Member Author

Choose a reason for hiding this comment

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

From our conversation offline, re-arranging the "removeConnection" before "tryDeliver" does fix the ordering for "connection closed" and "checkout failed". However, "handshakeErrFn" is responsible for clearing the pool, so this change will cause a different race condition between "connection closed" and "pool cleared" messages. The bottleneck in all of this is "tryDeliver", which is why it was chosen as the place to lock the "ready" channel close.

continue
}

Expand Down Expand Up @@ -1006,7 +1008,8 @@ func compact(arr []*connection) []*connection {
// other and use wantConn to coordinate and agree about the winning outcome.
// Based on https://cs.opensource.google/go/go/+/refs/tags/go1.16.6:src/net/http/transport.go;l=1174-1240
type wantConn struct {
ready chan struct{}
readyMu sync.Mutex // Guards ready
ready chan struct{}

mu sync.Mutex // Guards conn, err
conn *connection
Expand Down Expand Up @@ -1043,7 +1046,14 @@ func (w *wantConn) tryDeliver(conn *connection, err error) bool {
if w.conn == nil && w.err == nil {
panic("x/mongo/driver/topology: internal error: misuse of tryDeliver")
}
close(w.ready)

go func() {
w.readyMu.Lock()
defer w.readyMu.Unlock()

close(w.ready)
}()

return true
}

Expand Down