GODRIVER-1234 deadlock with minPoolSize

Change-Id: I478a49ae07c8106c88db520f42b97d27c1b2448d

Author: iwysiu

x/mongo/driver/topology/connection.go

@@ -45,34 +45,23 @@ type connection struct {
 	compressor       wiremessage.CompressorID
 	zliblevel        int
 	connected        int32 // must be accessed using the sync/atomic package
+	connectDone      chan struct{}
+	connectErr       error
+	config           *connectionConfig
 
 	// pool related fields
 	pool       *pool
 	poolID     uint64
 	generation uint64
 }
 
-// newConnection handles the creation of a connection. It will dial, configure TLS, and perform
-// initialization handshakes.
+// newConnection handles the creation of a connection. It does not connect the connection.
 func newConnection(ctx context.Context, addr address.Address, opts ...ConnectionOption) (*connection, error) {
 	cfg, err := newConnectionConfig(opts...)
 	if err != nil {
 		return nil, err
 	}
 
-	nc, err := cfg.dialer.DialContext(ctx, addr.Network(), addr.String())
-	if err != nil {
-		return nil, ConnectionError{Wrapped: err, init: true}
-	}
-
-	if cfg.tlsConfig != nil {
-		tlsConfig := cfg.tlsConfig.Clone()
-		nc, err = configureTLS(ctx, nc, addr, tlsConfig)
-		if err != nil {
-			return nil, ConnectionError{Wrapped: err, init: true}
-		}
-	}
-
 	var lifetimeDeadline time.Time
 	if cfg.lifeTimeout > 0 {
 		lifetimeDeadline = time.Now().Add(cfg.lifeTimeout)
@@ -82,32 +71,64 @@ func newConnection(ctx context.Context, addr address.Address, opts ...Connection
 
 	c := &connection{
 		id:               id,
-		nc:               nc,
 		addr:             addr,
 		idleTimeout:      cfg.idleTimeout,
 		lifetimeDeadline: lifetimeDeadline,
 		readTimeout:      cfg.readTimeout,
 		writeTimeout:     cfg.writeTimeout,
+		connectDone:      make(chan struct{}),
+		config:           cfg,
+	}
+	atomic.StoreInt32(&c.connected, initialized)
+
+	return c, nil
+}
+
+// connect handles the I/O for a connection. It will dial, configure TLS, and perform
+// initialization handshakes.
+func (c *connection) connect(ctx context.Context) {
+	if !atomic.CompareAndSwapInt32(&c.connected, initialized, connected) {
+		return
+	}
+	defer close(c.connectDone)
+
+	var err error
+	c.nc, err = c.config.dialer.DialContext(ctx, c.addr.Network(), c.addr.String())
+	if err != nil {
+		atomic.StoreInt32(&c.connected, disconnected)
+		c.connectErr = ConnectionError{Wrapped: err, init: true}
+		return
+	}
+
+	if c.config.tlsConfig != nil {
+		tlsConfig := c.config.tlsConfig.Clone()
+		c.nc, err = configureTLS(ctx, c.nc, c.addr, tlsConfig)
+		if err != nil {
+			atomic.StoreInt32(&c.connected, disconnected)
+			c.connectErr = ConnectionError{Wrapped: err, init: true}
+			return
+		}
 	}
-	atomic.StoreInt32(&c.connected, connected)
 
 	c.bumpIdleDeadline()
 
 	// running isMaster and authentication is handled by a handshaker on the configuration instance.
-	if cfg.handshaker != nil {
-		c.desc, err = cfg.handshaker.Handshake(ctx, c.addr, initConnection{c})
+	if c.config.handshaker != nil {
+		c.desc, err = c.config.handshaker.Handshake(ctx, c.addr, initConnection{c})
 		if err != nil {
 			if c.nc != nil {
 				_ = c.nc.Close()
 			}
-			return nil, ConnectionError{Wrapped: err, init: true}
+			atomic.StoreInt32(&c.connected, disconnected)
+			c.connectErr = ConnectionError{Wrapped: err, init: true}
+			return
 		}
-		if cfg.descCallback != nil {
-			cfg.descCallback(c.desc)
+		if c.config.descCallback != nil {
+			c.config.descCallback(c.desc)
 		}
 		if len(c.desc.Compression) > 0 {
 		clientMethodLoop:
-			for _, method := range cfg.compressors {
+			for _, method := range c.config.compressors {
 				for _, serverMethod := range c.desc.Compression {
 					if method != serverMethod {
 						continue
@@ -119,16 +140,20 @@ func newConnection(ctx context.Context, addr address.Address, opts ...Connection
 					case "zlib":
 						c.compressor = wiremessage.CompressorZLib
 						c.zliblevel = wiremessage.DefaultZlibLevel
-						if cfg.zlibLevel != nil {
-							c.zliblevel = *cfg.zlibLevel
+						if c.config.zlibLevel != nil {
+							c.zliblevel = *c.config.zlibLevel
 						}
 					}
 					break clientMethodLoop
 				}
 			}
 		}
 	}
-	return c, nil
+}
+
+func (c *connection) connectWait() error {
+	<-c.connectDone
+	return c.connectErr
 }
 
 func (c *connection) writeWireMessage(ctx context.Context, wm []byte) error {

x/mongo/driver/topology/connection_test.go

@@ -45,20 +45,27 @@ func TestConnection(t *testing.T) {
 					t.Errorf("errors do not match. got %v; want %v", got, want)
 				}
 			})
+		})
+		t.Run("connect", func(t *testing.T) {
 			t.Run("dialer error", func(t *testing.T) {
 				err := errors.New("dialer error")
 				var want error = ConnectionError{Wrapped: err}
-				_, got := newConnection(context.Background(), address.Address(""), WithDialer(func(Dialer) Dialer {
+				conn, got := newConnection(context.Background(), address.Address(""), WithDialer(func(Dialer) Dialer {
 					return DialerFunc(func(context.Context, string, string) (net.Conn, error) { return nil, err })
 				}))
+				if got != nil {
+					t.Errorf("newConnection shouldn't error. got %v; want nil", got)
+				}
+				conn.connect(context.Background())
+				got = conn.connectWait()
 				if !cmp.Equal(got, want, cmp.Comparer(compareErrors)) {
 					t.Errorf("errors do not match. got %v; want %v", got, want)
 				}
 			})
 			t.Run("handshaker error", func(t *testing.T) {
 				err := errors.New("handshaker error")
 				var want error = ConnectionError{Wrapped: err}
-				_, got := newConnection(context.Background(), address.Address(""),
+				conn, got := newConnection(context.Background(), address.Address(""),
 					WithHandshaker(func(Handshaker) Handshaker {
 						return HandshakerFunc(func(context.Context, address.Address, driver.Connection) (description.Server, error) {
 							return description.Server{}, err
@@ -70,14 +77,19 @@ func TestConnection(t *testing.T) {
 						})
 					}),
 				)
+				if got != nil {
+					t.Errorf("newConnection shouldn't error. got %v; want nil", got)
+				}
+				conn.connect(context.Background())
+				got = conn.connectWait()
 				if !cmp.Equal(got, want, cmp.Comparer(compareErrors)) {
 					t.Errorf("errors do not match. got %v; want %v", got, want)
 				}
 			})
 			t.Run("calls description callback", func(t *testing.T) {
 				want := description.Server{Addr: address.Address("1.2.3.4:56789")}
 				var got description.Server
-				_, err := newConnection(context.Background(), address.Address(""),
+				conn, err := newConnection(context.Background(), address.Address(""),
 					withServerDescriptionCallback(func(desc description.Server) { got = desc },
 						WithHandshaker(func(Handshaker) Handshaker {
 							return HandshakerFunc(func(context.Context, address.Address, driver.Connection) (description.Server, error) {
@@ -92,6 +104,9 @@ func TestConnection(t *testing.T) {
 					)...,
 				)
 				noerr(t, err)
+				conn.connect(context.Background())
+				err = conn.connectWait()
+				noerr(t, err)
 				if !cmp.Equal(got, want) {
 					t.Errorf("Server descriptions do not match. got %v; want %v", got, want)
 				}

x/mongo/driver/topology/pool.go

@@ -125,6 +125,9 @@ func (p *pool) connectionInitFunc() interface{} {
 	if err != nil {
 		return nil
 	}
+
+	go c.connect(context.Background())
+
 	return c
 }
 
@@ -321,6 +324,23 @@ func (p *pool) get(ctx context.Context) (*connection, error) {
 
 	connVal := p.conns.Get()
 	if c, ok := connVal.(*connection); ok && connVal != nil {
+		// call connect if not connected
+		if atomic.LoadInt32(&c.connected) == initialized {
+			c.connect(ctx)
+		}
+
+		err := c.connectWait()
+		if err != nil {
+			if p.monitor != nil {
+				p.monitor.Event(&event.PoolEvent{
+					Type:    event.GetFailed,
+					Address: p.address.String(),
+					Reason:  event.ReasonConnectionErrored,
+				})
+			}
+			return nil, err
+		}
+
 		if p.monitor != nil {
 			p.monitor.Event(&event.PoolEvent{
 				Type:         event.GetSucceeded,
@@ -343,6 +363,21 @@ func (p *pool) get(ctx context.Context) (*connection, error) {
 		return nil, ctx.Err()
 	default:
 		c, reason, err := p.makeNewConnection(ctx)
+
+		if err != nil {
+			if p.monitor != nil {
+				p.monitor.Event(&event.PoolEvent{
+					Type:    event.GetFailed,
+					Address: p.address.String(),
+					Reason:  reason,
+				})
+			}
+			return nil, err
+		}
+
+		c.connect(ctx)
+		// wait for conn to be connected
+		err = c.connectWait()
 		if err != nil {
 			if p.monitor != nil {
 				p.monitor.Event(&event.PoolEvent{

x/mongo/driver/topology/server.go

@@ -58,6 +58,7 @@ const (
 	disconnecting
 	connected
 	connecting
+	initialized
 )
 
 func connectionStateString(state int32) string {
@@ -70,6 +71,8 @@ func connectionStateString(state int32) string {
 		return "Connected"
 	case 3:
 		return "Connecting"
+	case 4:
+		return "Initialized"
 	}
 
 	return ""
@@ -493,6 +496,10 @@ func (s *Server) heartbeat(conn *connection) (description.Server, *connection) {
 			}))
 
 			conn, err = newConnection(ctx, s.address, opts...)
+
+			conn.connect(ctx)
+
+			err := conn.connectWait()
 			if err == nil {
 				descPtr = &conn.desc
 			}

x/mongo/driver/topology/topology_test.go

@@ -15,6 +15,7 @@ import (
 
 	"go.mongodb.org/mongo-driver/x/mongo/driver"
 	"go.mongodb.org/mongo-driver/x/mongo/driver/address"
+	"go.mongodb.org/mongo-driver/x/mongo/driver/connstring"
 	"go.mongodb.org/mongo-driver/x/mongo/driver/description"
 )
 
@@ -438,3 +439,19 @@ func TestSessionTimeout(t *testing.T) {
 		}
 	})
 }
+
+func TestMinPoolSize(t *testing.T) {
+	connStr := connstring.ConnString{
+		Hosts:          []string{"localhost:27017"},
+		MinPoolSize:    10,
+		MinPoolSizeSet: true,
+	}
+	topo, err := New(WithConnString(func(connstring.ConnString) connstring.ConnString { return connStr }))
+	if err != nil {
+		t.Errorf("topology.New shouldn't error. got: %v", err)
+	}
+	err = topo.Connect()
+	if err != nil {
+		t.Errorf("topology.Connect shouldn't error. got: %v", err)
+	}
+}