fix TestPoolBackgroundChecksMinConns and NewConnsCount

Previously it was checking TotalConns but that includes ConstructingConns.
Instead it should directly check IdleConns so the next Acquire takes one of
those and doesn't make a 3rd connection. The check against the context was
also wrong which prevented this from timing out after 2 minutes.

This also fixes a bug where NewConnsCount was not correctly counting
connections created by Acquire directly.

Fixes #1690
pull/1697/head
James Hartig 2023-07-19 23:23:14 -04:00 committed by Jack Christensen
parent f90e86fd8d
commit e665f74c99
2 changed files with 13 additions and 12 deletions

View File

@ -199,6 +199,7 @@ func NewWithConfig(ctx context.Context, config *Config) (*Pool, error) {
p.p, err = puddle.NewPool( p.p, err = puddle.NewPool(
&puddle.Config[*connResource]{ &puddle.Config[*connResource]{
Constructor: func(ctx context.Context) (*connResource, error) { Constructor: func(ctx context.Context) (*connResource, error) {
atomic.AddInt64(&p.newConnsCount, 1)
connConfig := p.config.ConnConfig.Copy() connConfig := p.config.ConnConfig.Copy()
// Connection will continue in background even if Acquire is canceled. Ensure that a connect won't hang forever. // Connection will continue in background even if Acquire is canceled. Ensure that a connect won't hang forever.
@ -475,7 +476,6 @@ func (p *Pool) createIdleResources(parentCtx context.Context, targetResources in
for i := 0; i < targetResources; i++ { for i := 0; i < targetResources; i++ {
go func() { go func() {
atomic.AddInt64(&p.newConnsCount, 1)
err := p.p.CreateResource(ctx) err := p.p.CreateResource(ctx)
// Ignore ErrNotAvailable since it means that the pool has become full since we started creating resource. // Ignore ErrNotAvailable since it means that the pool has become full since we started creating resource.
if err == puddle.ErrNotAvailable { if err == puddle.ErrNotAvailable {

View File

@ -5,7 +5,6 @@ import (
"errors" "errors"
"fmt" "fmt"
"os" "os"
"runtime"
"sync/atomic" "sync/atomic"
"testing" "testing"
"time" "time"
@ -549,6 +548,7 @@ func TestPoolBackgroundChecksMaxConnLifetime(t *testing.T) {
assert.EqualValues(t, 0, stats.TotalConns()) assert.EqualValues(t, 0, stats.TotalConns())
assert.EqualValues(t, 0, stats.MaxIdleDestroyCount()) assert.EqualValues(t, 0, stats.MaxIdleDestroyCount())
assert.EqualValues(t, 1, stats.MaxLifetimeDestroyCount()) assert.EqualValues(t, 1, stats.MaxLifetimeDestroyCount())
assert.EqualValues(t, 1, stats.NewConnsCount())
} }
func TestPoolBackgroundChecksMaxConnIdleTime(t *testing.T) { func TestPoolBackgroundChecksMaxConnIdleTime(t *testing.T) {
@ -584,14 +584,11 @@ func TestPoolBackgroundChecksMaxConnIdleTime(t *testing.T) {
assert.EqualValues(t, 0, stats.TotalConns()) assert.EqualValues(t, 0, stats.TotalConns())
assert.EqualValues(t, 1, stats.MaxIdleDestroyCount()) assert.EqualValues(t, 1, stats.MaxIdleDestroyCount())
assert.EqualValues(t, 0, stats.MaxLifetimeDestroyCount()) assert.EqualValues(t, 0, stats.MaxLifetimeDestroyCount())
assert.EqualValues(t, 1, stats.NewConnsCount())
} }
func TestPoolBackgroundChecksMinConns(t *testing.T) { func TestPoolBackgroundChecksMinConns(t *testing.T) {
// jackc: I don't have a good way to investigate this as I don't develop on Windows. Problem started with t.Parallel()
// c513e2e435dca8acde76a4a0ed4856b9946b14e0, but I have no idea how. Help wanted. Test disabled on Windows for now.
if runtime.GOOS == "windows" {
t.Skip("Test always runs for 10m and is killed on CI. See https://github.com/jackc/pgx/issues/1690")
}
ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second)
defer cancel() defer cancel()
@ -607,27 +604,31 @@ func TestPoolBackgroundChecksMinConns(t *testing.T) {
defer db.Close() defer db.Close()
stats := db.Stat() stats := db.Stat()
for !(stats.TotalConns() == 2 && stats.MaxLifetimeDestroyCount() == 0 && stats.NewConnsCount() == 2) || ctx.Err() != nil { for !(stats.IdleConns() == 2 && stats.MaxLifetimeDestroyCount() == 0 && stats.NewConnsCount() == 2) && ctx.Err() == nil {
time.Sleep(50 * time.Millisecond) time.Sleep(50 * time.Millisecond)
stats = db.Stat() stats = db.Stat()
} }
require.NoError(t, ctx.Err()) require.EqualValues(t, 2, stats.IdleConns())
require.EqualValues(t, 2, stats.TotalConns())
require.EqualValues(t, 0, stats.MaxLifetimeDestroyCount()) require.EqualValues(t, 0, stats.MaxLifetimeDestroyCount())
require.EqualValues(t, 2, stats.NewConnsCount()) require.EqualValues(t, 2, stats.NewConnsCount())
c, err := db.Acquire(ctx) c, err := db.Acquire(ctx)
require.NoError(t, err) require.NoError(t, err)
stats = db.Stat()
require.EqualValues(t, 1, stats.IdleConns())
require.EqualValues(t, 0, stats.MaxLifetimeDestroyCount())
require.EqualValues(t, 2, stats.NewConnsCount())
err = c.Conn().Close(ctx) err = c.Conn().Close(ctx)
require.NoError(t, err) require.NoError(t, err)
c.Release() c.Release()
stats = db.Stat() stats = db.Stat()
for !(stats.TotalConns() == 2 && stats.MaxIdleDestroyCount() == 0 && stats.NewConnsCount() == 3) || ctx.Err() != nil { for !(stats.IdleConns() == 2 && stats.MaxIdleDestroyCount() == 0 && stats.NewConnsCount() == 3) && ctx.Err() == nil {
time.Sleep(50 * time.Millisecond) time.Sleep(50 * time.Millisecond)
stats = db.Stat() stats = db.Stat()
} }
require.NoError(t, ctx.Err())
require.EqualValues(t, 2, stats.TotalConns()) require.EqualValues(t, 2, stats.TotalConns())
require.EqualValues(t, 0, stats.MaxIdleDestroyCount()) require.EqualValues(t, 0, stats.MaxIdleDestroyCount())
require.EqualValues(t, 3, stats.NewConnsCount()) require.EqualValues(t, 3, stats.NewConnsCount())