close Pool when failed to Connect it (#64)

* return initial connection errors and wait for error handlers to complete

* close pool if Connect errors

* fix how we collect and return errors

Author: alovak

pool.go

@@ -53,17 +53,38 @@ func (p *Pool) handleError(err error) {
 		return
 	}
 
-	go p.Opts.ErrorHandler(err)
+	p.wg.Add(1)
+	go func() {
+		defer p.wg.Done()
+		p.Opts.ErrorHandler(err)
+	}()
 }
 
 // Connect creates poll of connections by calling Factory method and connect them all
 func (p *Pool) Connect() error {
+	// We need to close pool (with all potentially running goroutines) if
+	// connection creation fails. Example of such situation is when we
+	// successfully created 2 connections, but 3rd failed and minimum
+	// connections is 3.
+	// Because `Close` uses same mutex as `Connect` we need to unlock it
+	// before calling `Close`.  That's why we use `connectErr` variable and
+	// `defer` statement here, before the next `defer` which unlocks mutex.
+	var connectErr error
+	defer func() {
+		if connectErr != nil {
+			p.Close()
+		}
+	}()
+
 	p.mu.Lock()
 	defer p.mu.Unlock()
 	if p.isClosed {
 		return errors.New("pool is closed")
 	}
 
+	// errors from initial connections creation
+	var errs []error
+
 	// build connections
 	for _, addr := range p.Addrs {
 		conn, err := p.Factory(addr)
@@ -76,7 +97,8 @@ func (p *Pool) Connect() error {
 
 		err = conn.Connect()
 		if err != nil {
-			p.handleError(fmt.Errorf("connecting to %s: %w", conn.addr, err))
+			errs = append(errs, fmt.Errorf("connecting to %s: %w", addr, err))
+			p.handleError(fmt.Errorf("failed to connect to %s: %w", conn.addr, err))
 			p.wg.Add(1)
 			go p.recreateConnection(conn)
 			continue
@@ -85,11 +107,16 @@ func (p *Pool) Connect() error {
 		p.connections = append(p.connections, conn)
 	}
 
-	if len(p.connections) < p.Opts.MinConnections {
-		return fmt.Errorf("minimum %d connections is required, established: %d", p.Opts.MinConnections, len(p.connections))
+	if len(p.connections) >= p.Opts.MinConnections {
+		return nil
 	}
 
-	return nil
+	if len(errs) == 0 {
+		connectErr = fmt.Errorf("minimum %d connections is required, established: %d", p.Opts.MinConnections, len(p.connections))
+	} else {
+		connectErr = fmt.Errorf("minimum %d connections is required, established: %d, errors: %w", p.Opts.MinConnections, len(p.connections), errors.Join(errs...))
+	}
+	return connectErr
 }
 
 // Connections returns copy of all connections from the pool

pool_options.go

@@ -20,7 +20,8 @@ type PoolOptions struct {
 	MaxReconnectWait time.Duration
 
 	// ErrorHandler is called in a goroutine with the errors that can't be
-	// returned to the caller
+	// returned to the caller. Don't block in this function as it will
+	// block the connection pool Close() method.
 	ErrorHandler func(err error)
 
 	// MinConnections is the number of connections required to be established when

pool_test.go

@@ -230,9 +230,9 @@ func TestPool(t *testing.T) {
 		require.NoError(t, err)
 
 		err = pool.Connect()
-		defer pool.Close()
-
-		require.EqualError(t, err, "minimum 3 connections is required, established: 2")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "minimum 3 connections is required, established: 2")
+		require.Zero(t, len(pool.Connections()), "all connections should be closed")
 	})
 
 	t.Run("Connect() returns no error when established >= MinConnections and later re-establish failed connections", func(t *testing.T) {