fix(connpool): adjust globalIdle based on the number of connections decreased during the Get. (#698)

* fix(connpool): adjust globalIdle based on the number of connections decreased during the Get

Author: ppzqh

pkg/remote/connpool/long_pool.go

@@ -104,17 +104,18 @@ type pool struct {
 	maxIdleTimeout time.Duration // the idle connection will be cleaned if the idle time exceeds maxIdleTimeout.
 }
 
-// Get gets the first active connection from the idleList.
-func (p *pool) Get() (*longConn, bool) {
+// Get gets the first active connection from the idleList. Return the number of connections decreased during the Get.
+func (p *pool) Get() (*longConn, bool, int) {
 	p.mu.Lock()
 	// Get the first active one
-	i := len(p.idleList) - 1
+	n := len(p.idleList)
+	i := n - 1
 	for ; i >= 0; i-- {
 		o := p.idleList[i]
 		if o.IsActive() {
 			p.idleList = p.idleList[:i]
 			p.mu.Unlock()
-			return o, true
+			return o, true, n - i
 		}
 		// inactive object
 		o.Close()
@@ -125,7 +126,7 @@ func (p *pool) Get() (*longConn, bool) {
 	}
 	p.idleList = p.idleList[:i]
 	p.mu.Unlock()
-	return nil, false
+	return nil, false, n - i
 }
 
 // Put puts back a connection to the pool.
@@ -223,9 +224,9 @@ type peer struct {
 // Get gets a connection with dialer and timeout. Dial a new connection if no idle connection in pool is available.
 func (p *peer) Get(d remote.Dialer, timeout time.Duration, reporter Reporter, addr string) (net.Conn, error) {
 	var c net.Conn
-	c, reused := p.pool.Get()
+	c, reused, decNum := p.pool.Get()
+	p.globalIdle.DecN(int64(decNum))
 	if reused {
-		p.globalIdle.Dec()
 		return c, nil
 	}
 	// dial a new connection

pkg/remote/connpool/long_pool_test.go

@@ -70,13 +70,15 @@ func TestPoolReuse(t *testing.T) {
 	test.Assert(t, recycled == true)
 	test.Assert(t, p.Len() == 1)
 
-	c, reused := p.Get()
+	c, reused, decN := p.Get()
 	test.Assert(t, c != nil)
 	test.Assert(t, reused == true)
+	test.Assert(t, decN == 1)
 	count[c] = true
 	test.Assert(t, len(count) == 1)
 }
 
+// TestPoolGetInactiveConn tests the pool with only inactive connection. Get() should return nil.
 func TestPoolGetInactiveConn(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
@@ -105,12 +107,61 @@ func TestPoolGetInactiveConn(t *testing.T) {
 	recycled := p.Put(conn)
 	test.Assert(t, recycled == true)
 	// inactive
-	conn, reused := p.Get()
+	conn, reused, decNum := p.Get()
+	test.Assert(t, decNum == 1)
 	test.Assert(t, conn == nil)
 	test.Assert(t, reused == false)
 	test.Assert(t, closed == true)
 }
 
+// TestPoolGetWithInactiveConn tests the pool with inactive connection. Get() should return the first active one.
+func TestPoolGetWithInactiveConn(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	var (
+		minIdle        = 0
+		maxIdle        = 10
+		maxIdleTimeout = time.Millisecond
+		inactiveNum    = 5
+	)
+
+	p := newPool(minIdle, maxIdle, maxIdleTimeout)
+	// put active conn
+	activeConn := newLongConnForTest(ctrl, mockAddr0)
+	recycled := p.Put(activeConn)
+	test.Assert(t, recycled == true)
+
+	// put inactive conn
+	closed := make([]bool, inactiveNum)
+	for i := 0; i < inactiveNum; i++ {
+		idx := i
+		c := mocksnetpoll.NewMockConnection(ctrl)
+		c.EXPECT().IsActive().Return(false).AnyTimes()
+		c.EXPECT().Close().DoAndReturn(func() error {
+			closed[idx] = true
+			return nil
+		}).AnyTimes()
+		conn := &longConn{
+			Conn:    c,
+			address: mockAddr0,
+		}
+		recycled := p.Put(conn)
+		test.Assert(t, recycled == true)
+	}
+	test.Assert(t, p.Len() == inactiveNum+1)
+
+	// decNum should be inactiveNum + 1
+	conn, reused, decNum := p.Get()
+	test.Assert(t, conn != nil)
+	test.Assert(t, reused == true)
+	test.Assert(t, decNum == inactiveNum+1)
+	// check if all inactive conns have been closed
+	for i := 0; i < inactiveNum; i++ {
+		test.Assert(t, closed[i] == true)
+	}
+}
+
 func TestPoolMaxIdle(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()