after the connection pool is closed, no new connections should be added

[monkey92t]

linked: #1861

When the connection pool is closed, we will still add new connections to the connection pool, and this part of the connection cannot be closed.

It may also cause errors such as Stats, as described in #1861

I added a count of runGoroutine, I am not sure if adding it is a good decision, although it is only for testing.

@vmihailenco Do you have any ideas?

[vmihailenco]

This looks good, but let's remove runGoroutine 👍

[monkey92t]

I try to do more experiments to check if there are similar problems in the connection pool.

[vmihailenco]

Thanks, and let me know when to take another look.

[monkey92t]

We will create >poolSize connections.

test example:

ctx := context.Background()
var n int32
connPool := pool.NewConnPool(&pool.Options{
	Dialer: func(ctx context.Context) (net.Conn, error) {
		atomic.AddInt32(&n, 1)
		time.Sleep(10 * time.Millisecond)
		return &net.TCPConn{}, nil
	},
	PoolSize:           10,
	PoolTimeout:        time.Hour,
	IdleTimeout:        time.Minute,
	IdleCheckFrequency: time.Minute,
	MinIdleConns:       10,
})
perform(10, func(_ int) {
	cn, err := connPool.Get(ctx)
	if err != nil {
		t.Fatal(err)
	}
	connPool.Put(ctx, cn)
})
t.Log(atomic.LoadInt32(&n))

// poolSize: 10
// output: 20

I think we should reset how addIdleConn works.......

[vmihailenco]

It is expected that addIdleConns can create extra connections. Such connections are maked with pooled = false and are removed on then next Put - see https://github.com/go-redis/redis/blob/e5571ea13ffc16774e02090872024111917cfd59/internal/pool/pool.go#L178-L180

I could not come with a better/cleaner implementation, because the requirement is to not hold the lock when dialing/closing a new connection (which can potentially block for long time).

[monkey92t]

It is expected that addIdleConns can create extra connections. Such connections are maked with pooled = false and are removed on then next Put - see

Yes, I noticed here. but... we should not create a connection with pooled=true when the connection pool is full(p.poolSize == opt.PoolSize).

If we want to fix this, the changes to the connection pool are too large, but it should have the following characteristics:

1. Have the existing efficiency and security of the connection pool.
2. Never create more connections than opt.PoolSize (except pooled=false).
3. After the connection pool is closed, we should not create a new connection, otherwise it will cause the connection to leak.
4. When creating a new connection, it should not be locked, at least the p.connsMu lock should not be used.

[vmihailenco]

I am not sure that is possible with the current design and it looks like a different/separate problem to me. It is expected that number of connections in a pool can exceed PoolSize for a brief period, but the size of a pool should shrink down quick enough to not cause any harm.

I would merge p.closed() checks in this PR since they improve current situation and then would try to come up with a better pool design in a different PR.

[monkey92t]

OK, good

[monkey92t]

I reopened the PR, do you have anything else to add? (check p.closed)

[vmihailenco]

Have a small suggestions but overall looks good.

[monkey92t]

Oh...golangci-lint released a new version... forward

[monkey92t]

This error seems to be a false positive