From 2fc75666b4ad1dbd99fb56d4bfea8c8dd1641285 Mon Sep 17 00:00:00 2001 From: Oleg Jukovec Date: Thu, 18 Aug 2022 14:06:16 +0300 Subject: [PATCH] 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 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. Part of #208 --- CHANGELOG.md | 1 + connection_pool/connection_pool.go | 66 +++++++++++-------------- connection_pool/connection_pool_test.go | 36 ++++++++++++++ connection_pool/round_robin.go | 34 +++++-------- connection_pool/round_robin_test.go | 16 ++++++ 5 files changed, 94 insertions(+), 59 deletions(-) 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) + } + } +}