diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f3be2de8..125081630 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release. - Mode type description in the connection_pool subpackage (#208) - Missed Role type constants in the connection_pool subpackage (#208) - ConnectionPool does not close UnknownRole connections (#208) +- Segmentation faults in ConnectionPool requests after disconnect (#208) ## [1.8.0] - 2022-08-17 diff --git a/connection_pool/connection_pool.go b/connection_pool/connection_pool.go index e424fe85e..0fcd88d07 100644 --- a/connection_pool/connection_pool.go +++ b/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 } diff --git a/connection_pool/connection_pool_test.go b/connection_pool/connection_pool_test.go index b6380219d..a01bf5fd2 100644 --- a/connection_pool/connection_pool_test.go +++ b/connection_pool/connection_pool_test.go @@ -208,6 +208,42 @@ func TestClose(t *testing.T) { require.Nil(t, err) } +func TestRequestOnClosed(t *testing.T) { + server1 := servers[0] + server2 := servers[1] + + connPool, err := connection_pool.Connect([]string{server1, server2}, connOpts) + require.Nilf(t, err, "failed to connect") + require.NotNilf(t, connPool, "conn is nil after Connect") + + defer connPool.Close() + + test_helpers.StopTarantoolWithCleanup(instances[0]) + test_helpers.StopTarantoolWithCleanup(instances[1]) + + args := test_helpers.CheckStatusesArgs{ + ConnPool: connPool, + Mode: connection_pool.ANY, + Servers: []string{server1, server2}, + ExpectedPoolStatus: false, + ExpectedStatuses: map[string]bool{ + server1: false, + server2: false, + }, + } + err = test_helpers.Retry(test_helpers.CheckPoolStatuses, args, defaultCountRetry, defaultTimeoutRetry) + require.Nil(t, err) + + _, err = connPool.Ping(connection_pool.ANY) + require.NotNilf(t, err, "err is nil after Ping") + + err = test_helpers.RestartTarantool(&instances[0]) + require.Nilf(t, err, "failed to restart tarantool") + + err = test_helpers.RestartTarantool(&instances[1]) + require.Nilf(t, err, "failed to restart tarantool") +} + func TestCall17(t *testing.T) { roles := []bool{false, true, false, false, true} diff --git a/connection_pool/round_robin.go b/connection_pool/round_robin.go index 9e512bfbb..b83d877d9 100644 --- a/connection_pool/round_robin.go +++ b/connection_pool/round_robin.go @@ -2,17 +2,16 @@ package connection_pool import ( "sync" - "sync/atomic" "github.com/tarantool/go-tarantool" ) type RoundRobinStrategy struct { conns []*tarantool.Connection - indexByAddr map[string]int + indexByAddr map[string]uint mutex sync.RWMutex - size int - current uint64 + size uint + current uint } func (r *RoundRobinStrategy) GetConnByAddr(addr string) *tarantool.Connection { @@ -66,29 +65,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), + indexByAddr: make(map[string]uint), size: 0, + current: 0, } } @@ -105,6 +93,8 @@ func (r *RoundRobinStrategy) AddConn(addr string, conn *tarantool.Connection) { } } -func (r *RoundRobinStrategy) nextIndex() int { - return int(atomic.AddUint64(&r.current, uint64(1)) % uint64(len(r.conns))) +func (r *RoundRobinStrategy) nextIndex() uint { + ret := r.current % r.size + r.current++ + return ret } diff --git a/connection_pool/round_robin_test.go b/connection_pool/round_robin_test.go index 128df3bb0..61df120f0 100644 --- a/connection_pool/round_robin_test.go +++ b/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) + } + } +}