Guard wantConn.ready with a Mutex

[prestonvasquez]

Summary

Add a mutex to guard the "ready" channel for a "wantConn" object. This allows synchronously closing a connection before acknowledging that a checkout is "ready".

Here is an alternative solution without locks:

x/mongo/driver/topology/pool.go
diff --git a/x/mongo/driver/topology/pool.go b/x/mongo/driver/topology/pool.go
index efbea595..71cc106a 100644
--- a/x/mongo/driver/topology/pool.go
+++ b/x/mongo/driver/topology/pool.go
@@ -469,6 +469,9 @@ func (p *pool) checkOut(ctx context.Context) (conn *connection, err error) {
 	select {
 	case <-w.ready:
 		if w.err != nil {
+			if w.closed != nil {
+				<-w.closed
+			}
 			if p.monitor != nil {
 				p.monitor.Event(&event.PoolEvent{
 					Type:    event.GetFailed,
@@ -841,6 +844,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.closed = make(chan struct{}, 1)
 			w.tryDeliver(nil, err)
 
 			// If there's an error connecting the new connection, call the handshake error handler
@@ -855,6 +859,8 @@ func (p *pool) createConnections(ctx context.Context, wg *sync.WaitGroup) {
 
 			_ = p.removeConnection(conn, event.ReasonError)
 			_ = p.closeConnection(conn)
+			close(w.closed)
+
 			continue
 		}
 
@@ -1006,7 +1012,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{}
+	ready  chan struct{}
+	closed chan struct{}
 
 	mu   sync.Mutex // Guards conn, err
 	conn *connection

Background & Motivation

The CMAP tests for logging expects that a checkout failure occur after a connection is closed. The Go Driver does not always meet this expectation, as there is a race condition between when a connection closes and when a checkout is signaled as "ready". Despite the fact that the CMAP specifications do not explicitly state that a connection must be closed before propagating a checkout failure, it is a reasonable expectation. Furthermore, future CMAP work will have this expectation as well, for example.

[matthewdale]

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?

[prestonvasquez]

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.

[matthewdale]

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.

[prestonvasquez]

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.

[prestonvasquez]

Closing, see #1165 for solution.