From 37350dbb1ef22ec7343d62a5b5851881e159ac94 Mon Sep 17 00:00:00 2001 From: Alex Vanin Date: Wed, 29 Jan 2025 19:57:29 +0300 Subject: [PATCH 1/2] [#326] pool: Fix panic that causes mutex deadlock Two concurrent 'deleteClientFromMap' calls for the same client may produce panic and deadlock. First goroutine acquires lock, removes element from the map, releases lock. Second goroutine acquires lock, and throws panic while trying to call 'close()' on empty struct. Lock is never released and it causes deadlock for other goroutines. Signed-off-by: Alex Vanin --- pool/tree/pool.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pool/tree/pool.go b/pool/tree/pool.go index ddfdc0e..b77d63a 100644 --- a/pool/tree/pool.go +++ b/pool/tree/pool.go @@ -1009,8 +1009,10 @@ func (p *Pool) addClientToMap(hash uint64, cl client) { func (p *Pool) deleteClientFromMap(hash uint64) { p.mutex.Lock() - _ = p.clientMap[hash].close() - delete(p.clientMap, hash) + if cli, ok := p.clientMap[hash]; ok { + _ = cli.close() + delete(p.clientMap, hash) + } p.mutex.Unlock() } -- 2.45.3 From 2786fadb256e94ef2772be1883874687aeb50335 Mon Sep 17 00:00:00 2001 From: Alex Vanin Date: Wed, 29 Jan 2025 20:12:54 +0300 Subject: [PATCH 2/2] [#326] pool: Add test for concurrent client deletion Signed-off-by: Alex Vanin --- pool/tree/pool_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pool/tree/pool_test.go b/pool/tree/pool_test.go index 607e037..5814a77 100644 --- a/pool/tree/pool_test.go +++ b/pool/tree/pool_test.go @@ -10,6 +10,7 @@ import ( cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id" cidtest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id/test" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/netmap" + netmaptest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/netmap/test" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pool" "git.frostfs.info/TrueCloudLab/hrw" "github.com/nspcc-dev/neo-go/pkg/crypto/keys" @@ -416,6 +417,21 @@ func TestRetryContainerNodes(t *testing.T) { }) } +func TestDeleteClientTwice(t *testing.T) { + p := Pool{ + clientMap: makeClientMap([]netmap.NodeInfo{netmaptest.NodeInfo()}), + } + // emulate concurrent requests as consecutive requests + // to delete the same client from the map twice + for idToDelete := range p.clientMap { + p.deleteClientFromMap(idToDelete) + require.NotPanics(t, func() { + p.deleteClientFromMap(idToDelete) + }) + } + require.Empty(t, p.clientMap) +} + func makeInnerPool(nodes [][]string) []*innerPool { res := make([]*innerPool, len(nodes)) -- 2.45.3