Skip to content

Commit

Permalink
fix TestPoolBackgroundChecksMinConns and NewConnsCount
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jameshartig committed Jul 20, 2023
1 parent 88b49d4 commit 8dde39d
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 12 deletions.
2 changes: 1 addition & 1 deletion pgxpool/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ func NewWithConfig(ctx context.Context, config *Config) (*Pool, error) {
p.p, err = puddle.NewPool(
&puddle.Config[*connResource]{
Constructor: func(ctx context.Context) (*connResource, error) {
atomic.AddInt64(&p.newConnsCount, 1)
connConfig := p.config.ConnConfig.Copy()

// Connection will continue in background even if Acquire is canceled. Ensure that a connect won't hang forever.
Expand Down Expand Up @@ -475,7 +476,6 @@ func (p *Pool) createIdleResources(parentCtx context.Context, targetResources in

for i := 0; i < targetResources; i++ {
go func() {
atomic.AddInt64(&p.newConnsCount, 1)
err := p.p.CreateResource(ctx)
// Ignore ErrNotAvailable since it means that the pool has become full since we started creating resource.
if err == puddle.ErrNotAvailable {
Expand Down
23 changes: 12 additions & 11 deletions pgxpool/pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"errors"
"fmt"
"os"
"runtime"
"sync/atomic"
"testing"
"time"
Expand Down Expand Up @@ -549,6 +548,7 @@ func TestPoolBackgroundChecksMaxConnLifetime(t *testing.T) {
assert.EqualValues(t, 0, stats.TotalConns())
assert.EqualValues(t, 0, stats.MaxIdleDestroyCount())
assert.EqualValues(t, 1, stats.MaxLifetimeDestroyCount())
assert.EqualValues(t, 1, stats.NewConnsCount())
}

func TestPoolBackgroundChecksMaxConnIdleTime(t *testing.T) {
Expand Down Expand Up @@ -584,14 +584,11 @@ func TestPoolBackgroundChecksMaxConnIdleTime(t *testing.T) {
assert.EqualValues(t, 0, stats.TotalConns())
assert.EqualValues(t, 1, stats.MaxIdleDestroyCount())
assert.EqualValues(t, 0, stats.MaxLifetimeDestroyCount())
assert.EqualValues(t, 1, stats.NewConnsCount())
}

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
// 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")
}
t.Parallel()

ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second)
defer cancel()
Expand All @@ -607,27 +604,31 @@ func TestPoolBackgroundChecksMinConns(t *testing.T) {
defer db.Close()

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)
stats = db.Stat()
}
require.NoError(t, ctx.Err())
require.EqualValues(t, 2, stats.TotalConns())
require.EqualValues(t, 2, stats.IdleConns())
require.EqualValues(t, 0, stats.MaxLifetimeDestroyCount())
require.EqualValues(t, 2, stats.NewConnsCount())

c, err := db.Acquire(ctx)
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)
require.NoError(t, err)
c.Release()

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)
stats = db.Stat()
}
require.NoError(t, ctx.Err())
require.EqualValues(t, 2, stats.TotalConns())
require.EqualValues(t, 0, stats.MaxIdleDestroyCount())
require.EqualValues(t, 3, stats.NewConnsCount())
Expand Down

0 comments on commit 8dde39d

Please sign in to comment.