From a80b84f01f9fc0d3e6f08445ba21f7e07880775e Mon Sep 17 00:00:00 2001 From: Max Riveiro Date: Wed, 13 Jul 2022 18:09:42 +0300 Subject: [PATCH] fix: handle panic in ringShards Hash function when Ring got closed Fixes #2126 --- ring.go | 26 ++++++++++++-------------- ring_test.go | 15 +++++++++++++++ 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/ring.go b/ring.go index 9e24ec5..d42bfab 100644 --- a/ring.go +++ b/ring.go @@ -259,10 +259,11 @@ func (c *ringShards) Hash(key string) string { var hash string c.mu.RLock() + defer c.mu.RUnlock() + if c.numShard > 0 { hash = c.hash.Get(key) } - c.mu.RUnlock() return hash } @@ -271,27 +272,22 @@ func (c *ringShards) GetByKey(key string) (*ringShard, error) { key = hashtag.Key(key) c.mu.RLock() + defer c.mu.RUnlock() if c.closed { - c.mu.RUnlock() return nil, pool.ErrClosed } if c.numShard == 0 { - c.mu.RUnlock() return nil, errRingShardsDown } hash := c.hash.Get(key) if hash == "" { - c.mu.RUnlock() return nil, errRingShardsDown } - shard := c.shards[hash] - c.mu.RUnlock() - - return shard, nil + return c.shards[hash], nil } func (c *ringShards) GetByName(shardName string) (*ringShard, error) { @@ -300,9 +296,9 @@ func (c *ringShards) GetByName(shardName string) (*ringShard, error) { } c.mu.RLock() - shard := c.shards[shardName] - c.mu.RUnlock() - return shard, nil + defer c.mu.RUnlock() + + return c.shards[shardName], nil } func (c *ringShards) Random() (*ringShard, error) { @@ -357,9 +353,9 @@ func (c *ringShards) rebalance() { func (c *ringShards) Len() int { c.mu.RLock() - l := c.numShard - c.mu.RUnlock() - return l + defer c.mu.RUnlock() + + return c.numShard } func (c *ringShards) Close() error { @@ -377,8 +373,10 @@ func (c *ringShards) Close() error { firstErr = err } } + c.hash = nil c.shards = nil + c.numShard = 0 c.list = nil return firstErr diff --git a/ring_test.go b/ring_test.go index 50b13ff..1a6ec84 100644 --- a/ring_test.go +++ b/ring_test.go @@ -114,6 +114,21 @@ var _ = Describe("Redis Ring", func() { }) Describe("pipeline", func() { + It("doesn't panic closed ring, returns error", func() { + pipe := ring.Pipeline() + for i := 0; i < 3; i++ { + err := pipe.Set(ctx, fmt.Sprintf("key%d", i), "value", 0).Err() + Expect(err).NotTo(HaveOccurred()) + } + + Expect(ring.Close()).NotTo(HaveOccurred()) + + Expect(func() { + _, execErr := pipe.Exec(ctx) + Expect(execErr).To(HaveOccurred()) + }).NotTo(Panic()) + }) + It("distributes keys", func() { pipe := ring.Pipeline() for i := 0; i < 100; i++ {