bugfix: prevent segfaults in connection_pool

After the patch ConnectionPool.getNextConnection() does not return
(nil, nil). It always return a connection or an error.

The check Connection.ConnectedNow() does not have sence because
the connection may be closed right after the GetNextConnection()
call. The code just complicates the logic and does not protect
against anything.

A chain of two atomic operations IsEmpty() + GetNextConnection()
wrong because it leads too a race condition.

Author: oleg-jukovec

connection_pool/connection_pool.go

@@ -130,13 +130,20 @@ func (connPool *ConnectionPool) ConnectedNow(mode Mode) (bool, error) {
 	if connPool.getState() != connConnected {
 		return false, nil
 	}
-
-	conn, err := connPool.getNextConnection(mode)
-	if err != nil || conn == nil {
-		return false, err
+	switch mode {
+	case ANY:
+		return !connPool.anyPool.IsEmpty(), nil
+	case RW:
+		return !connPool.rwPool.IsEmpty(), nil
+	case RO:
+		return !connPool.roPool.IsEmpty(), nil
+	case PreferRW:
+		fallthrough
+	case PreferRO:
+		return !connPool.rwPool.IsEmpty() || !connPool.roPool.IsEmpty(), nil
+	default:
+		return false, ErrNoHealthyInstance
 	}
-
-	return conn.ConnectedNow(), nil
 }
 
 // ConfiguredTimeout gets timeout of current connection.
@@ -751,49 +758,34 @@ func (connPool *ConnectionPool) getNextConnection(mode Mode) (*tarantool.Connect
 
 	switch mode {
 	case ANY:
-		if connPool.anyPool.IsEmpty() {
-			return nil, ErrNoHealthyInstance
+		if next := connPool.anyPool.GetNextConnection(); next != nil {
+			return next, nil
 		}
-
-		return connPool.anyPool.GetNextConnection(), nil
-
 	case RW:
-		if connPool.rwPool.IsEmpty() {
-			return nil, ErrNoRwInstance
+		if next := connPool.rwPool.GetNextConnection(); next != nil {
+			return next, nil
 		}
-
-		return connPool.rwPool.GetNextConnection(), nil
-
+		return nil, ErrNoRwInstance
 	case RO:
-		if connPool.roPool.IsEmpty() {
-			return nil, ErrNoRoInstance
+		if next := connPool.roPool.GetNextConnection(); next != nil {
+			return next, nil
 		}
-
-		return connPool.roPool.GetNextConnection(), nil
-
+		return nil, ErrNoRoInstance
 	case PreferRW:
-		if !connPool.rwPool.IsEmpty() {
-			return connPool.rwPool.GetNextConnection(), nil
+		if next := connPool.rwPool.GetNextConnection(); next != nil {
+			return next, nil
 		}
-
-		if !connPool.roPool.IsEmpty() {
-			return connPool.roPool.GetNextConnection(), nil
+		if next := connPool.roPool.GetNextConnection(); next != nil {
+			return next, nil
 		}
-
-		return nil, ErrNoHealthyInstance
-
 	case PreferRO:
-		if !connPool.roPool.IsEmpty() {
-			return connPool.roPool.GetNextConnection(), nil
+		if next := connPool.roPool.GetNextConnection(); next != nil {
+			return next, nil
 		}
-
-		if !connPool.rwPool.IsEmpty() {
-			return connPool.rwPool.GetNextConnection(), nil
+		if next := connPool.rwPool.GetNextConnection(); next != nil {
+			return next, nil
 		}
-
-		return nil, ErrNoHealthyInstance
 	}
-
 	return nil, ErrNoHealthyInstance
 }
 

connection_pool/round_robin.go

@@ -1,6 +1,7 @@
 package connection_pool
 
 import (
+	"math"
 	"sync"
 	"sync/atomic"
 
@@ -66,29 +67,18 @@ func (r *RoundRobinStrategy) GetNextConnection() *tarantool.Connection {
 	r.mutex.RLock()
 	defer r.mutex.RUnlock()
 
-	// We want to iterate through the elements in a circular order
-	// so the first element in cycle is connections[next]
-	// and the last one is connections[next + length].
-	next := r.nextIndex()
-	cycleLen := len(r.conns) + next
-	for i := next; i < cycleLen; i++ {
-		idx := i % len(r.conns)
-		if r.conns[idx].ConnectedNow() {
-			if i != next {
-				atomic.StoreUint64(&r.current, uint64(idx))
-			}
-			return r.conns[idx]
-		}
+	if r.size == 0 {
+		return nil
 	}
-
-	return nil
+	return r.conns[r.nextIndex()]
 }
 
 func NewEmptyRoundRobin(size int) *RoundRobinStrategy {
 	return &RoundRobinStrategy{
 		conns:       make([]*tarantool.Connection, 0, size),
 		indexByAddr: make(map[string]int),
 		size:        0,
+		current:     math.MaxUint64, // next value is 0
 	}
 }
 

connection_pool/round_robin_test.go

@@ -52,4 +52,20 @@ func TestRoundRobinAddDuplicateDelete(t *testing.T) {
 	}
 }
 
+func TestRoundRobinGetNextConnection(t *testing.T) {
+	rr := NewEmptyRoundRobin(10)
+
+	addrs := []string{validAddr1, validAddr2}
+	conns := []*tarantool.Connection{&tarantool.Connection{}, &tarantool.Connection{}}
 
+	for i, addr := range addrs {
+		rr.AddConn(addr, conns[i])
+	}
+
+	expectedConns := []*tarantool.Connection{conns[0], conns[1], conns[0], conns[1]}
+	for i, expected := range expectedConns {
+		if rr.GetNextConnection() != expected {
+			t.Errorf("Unexpected connection on %d call", i)
+		}
+	}
+}