From eb142698d01bf6bc4a747d69a6986c475d8826bc Mon Sep 17 00:00:00 2001 From: monkey92t Date: Wed, 28 Jul 2021 11:19:40 +0800 Subject: [PATCH 1/3] Added maxBadConnRetries to limit the number of times to obtain broken connections. Signed-off-by: monkey92t --- internal/pool/export_test.go | 13 ++++++++- internal/pool/pool.go | 11 +++++++- internal/pool/pool_test.go | 51 +++++++++++++++++++++++++++++++++++- 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/internal/pool/export_test.go b/internal/pool/export_test.go index de7a644ea..d1103a31e 100644 --- a/internal/pool/export_test.go +++ b/internal/pool/export_test.go @@ -1,7 +1,18 @@ package pool -import "time" +import ( + "net" + "time" +) func (cn *Conn) SetCreatedAt(tm time.Time) { cn.createdAt = tm } + +func (cn *Conn) NetConn() net.Conn { + return cn.netConn +} + +func MaxBadConnRetries() int { + return maxBadConnRetries +} diff --git a/internal/pool/pool.go b/internal/pool/pool.go index 577923a79..46fd6d73f 100644 --- a/internal/pool/pool.go +++ b/internal/pool/pool.go @@ -11,6 +11,11 @@ import ( "github.com/go-redis/redis/v8/internal" ) +// maxBadConnRetries is the maximum number of attempts to obtain a valid connection from the connection pool. +// When this number is exceeded, a new connection is created directly and +// no longer obtained from the connection pool. +const maxBadConnRetries = 2 + var ( // ErrClosed performs any operation on the closed client will return this error. ErrClosed = errors.New("redis: client is closed") @@ -235,7 +240,7 @@ func (p *ConnPool) Get(ctx context.Context) (*Conn, error) { return nil, err } - for { + for i := 0; i < maxBadConnRetries; i++ { p.connsMu.Lock() cn := p.popIdle() p.connsMu.Unlock() @@ -253,6 +258,10 @@ func (p *ConnPool) Get(ctx context.Context) (*Conn, error) { return cn, nil } + // After the connection pool is empty or after trying maxBadConnRetries times, + // we still get a broken connection. + // At this time, we directly create a new connection. + atomic.AddUint32(&p.stats.Misses, 1) newcn, err := p.newConn(ctx, true) diff --git a/internal/pool/pool_test.go b/internal/pool/pool_test.go index 6c94fc27a..98a958663 100644 --- a/internal/pool/pool_test.go +++ b/internal/pool/pool_test.go @@ -2,6 +2,7 @@ package pool_test import ( "context" + "net" "sync" "testing" "time" @@ -27,7 +28,7 @@ var _ = Describe("ConnPool", func() { }) AfterEach(func() { - connPool.Close() + _ = connPool.Close() }) It("should unblock client when conn is removed", func() { @@ -81,6 +82,54 @@ var _ = Describe("ConnPool", func() { }) }) +var _ = Describe("bad conn", func() { + ctx := context.Background() + var opt *pool.Options + var connPool *pool.ConnPool + + BeforeEach(func() { + opt = &pool.Options{ + Dialer: dummyDialer, + PoolSize: 10, + PoolTimeout: time.Hour, + IdleTimeout: time.Millisecond, + IdleCheckFrequency: time.Millisecond, + } + connPool = pool.NewConnPool(opt) + }) + + AfterEach(func() { + _ = connPool.Close() + }) + + It("should maxBadConnRetries", func() { + var err error + conns := make([]*pool.Conn, opt.PoolSize) + for i := 0; i < opt.PoolSize; i++ { + conns[i], err = connPool.Get(ctx) + Expect(err).NotTo(HaveOccurred()) + } + for i := 0; i < opt.PoolSize; i++ { + connPool.Put(ctx, conns[i]) + } + + var newConn *net.TCPConn + opt.Dialer = func(ctx context.Context) (net.Conn, error) { + newConn = &net.TCPConn{} + return newConn, nil + } + + conn, err := connPool.Get(ctx) + Expect(err).NotTo(HaveOccurred()) + Expect(conn.NetConn()).To(Equal(newConn)) + + stats := connPool.Stats() + Expect(stats.IdleConns).To(Equal(uint32(opt.PoolSize - pool.MaxBadConnRetries()))) + Expect(stats.Misses).To(Equal(uint32(opt.PoolSize + 1))) + Expect(stats.Hits).To(Equal(uint32(0))) + }) +}) + var _ = Describe("MinIdleConns", func() { const poolSize = 100 ctx := context.Background() From ea6ce27bf347d18cd4fcefae83b7d57c10945b65 Mon Sep 17 00:00:00 2001 From: monkey92t Date: Wed, 28 Jul 2021 11:33:08 +0800 Subject: [PATCH 2/3] update doc Signed-off-by: monkey92t --- internal/pool/pool.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/pool/pool.go b/internal/pool/pool.go index 46fd6d73f..19df4f44b 100644 --- a/internal/pool/pool.go +++ b/internal/pool/pool.go @@ -12,8 +12,7 @@ import ( ) // maxBadConnRetries is the maximum number of attempts to obtain a valid connection from the connection pool. -// When this number is exceeded, a new connection is created directly and -// no longer obtained from the connection pool. +// When this number is exceeded, a new connection is created directly. const maxBadConnRetries = 2 var ( From 6e755a43807a330353272d5dab416fed0e570193 Mon Sep 17 00:00:00 2001 From: monkey92t Date: Wed, 28 Jul 2021 11:39:28 +0800 Subject: [PATCH 3/3] update test Signed-off-by: monkey92t --- internal/pool/pool_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/pool/pool_test.go b/internal/pool/pool_test.go index 98a958663..0e33781c9 100644 --- a/internal/pool/pool_test.go +++ b/internal/pool/pool_test.go @@ -92,8 +92,8 @@ var _ = Describe("bad conn", func() { Dialer: dummyDialer, PoolSize: 10, PoolTimeout: time.Hour, - IdleTimeout: time.Millisecond, - IdleCheckFrequency: time.Millisecond, + IdleTimeout: time.Minute, + IdleCheckFrequency: 10 * time.Second, } connPool = pool.NewConnPool(opt) }) @@ -110,6 +110,8 @@ var _ = Describe("bad conn", func() { Expect(err).NotTo(HaveOccurred()) } for i := 0; i < opt.PoolSize; i++ { + // Damage it. + _ = conns[i].Close() connPool.Put(ctx, conns[i]) }