From 26cd25c14be177320a5914e51e4f0775b344dc05 Mon Sep 17 00:00:00 2001 From: Michael Tharp Date: Mon, 16 Dec 2019 21:52:20 +0000 Subject: [PATCH 1/2] Fix deadlock race when acquire is cancelled If the waiter goroutine completes before the section guarding canceledAcquireCount has run, then there is a deadlock because there is nothing that can unlock the mutex. Scheduling the second goroutine ensures that one of the two can release the lock and keep things moving. --- pool.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pool.go b/pool.go index 2bce46c..5f57feb 100644 --- a/pool.go +++ b/pool.go @@ -303,10 +303,6 @@ func (p *Pool) Acquire(ctx context.Context) (*Resource, error) { select { case <-ctx.Done(): - p.cond.L.Lock() - p.canceledAcquireCount += 1 - p.cond.L.Unlock() - // Allow goroutine waiting for signal to exit. Re-signal since we couldn't // do anything with it. Another goroutine might be waiting. go func() { @@ -315,6 +311,9 @@ func (p *Pool) Acquire(ctx context.Context) (*Resource, error) { p.cond.L.Unlock() }() + p.cond.L.Lock() + p.canceledAcquireCount += 1 + p.cond.L.Unlock() return nil, ctx.Err() case <-waitChan: } From f458c9c0f8e3b37d14f94f75b574cc9198a27163 Mon Sep 17 00:00:00 2001 From: Jack Christensen Date: Sat, 25 Jan 2020 16:17:17 -0600 Subject: [PATCH 2/2] Make stress test more stressful Detect bug fixed by 26cd25c14be177320a5914e51e4f0775b344dc05. See also https://github.com/jackc/puddle/pull/2. --- pool_test.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/pool_test.go b/pool_test.go index 837e546..7ce41e9 100644 --- a/pool_test.go +++ b/pool_test.go @@ -8,6 +8,7 @@ import ( "math/rand" "net" "os" + "runtime" "sync" "testing" "time" @@ -514,7 +515,12 @@ func TestStress(t *testing.T) { destructorCalls.Next() } - pool := puddle.NewPool(constructor, destructor, 10) + poolSize := runtime.NumCPU() + if poolSize < 4 { + poolSize = 4 + } + + pool := puddle.NewPool(constructor, destructor, int32(poolSize)) finishChan := make(chan struct{}) wg := &sync.WaitGroup{} @@ -547,7 +553,7 @@ func TestStress(t *testing.T) { }, // Acquire possibly canceled by context func() { - ctx, cancel := context.WithTimeout(context.Background(), time.Duration(rand.Int63n(200))*time.Millisecond) + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(rand.Int63n(2000))*time.Nanosecond) defer cancel() res, err := pool.Acquire(ctx) if err != nil { @@ -557,7 +563,7 @@ func TestStress(t *testing.T) { return } - time.Sleep(time.Duration(rand.Int63n(100)) * time.Millisecond) + time.Sleep(time.Duration(rand.Int63n(2000)) * time.Nanosecond) releaseOrDestroyOrHijack(res) }, // AcquireAllIdle (though under heavy load this will almost certainly always get an empty slice) @@ -569,7 +575,9 @@ func TestStress(t *testing.T) { }, } - for i := 0; i < 100; i++ { + workerCount := int(poolSize) * 2 + + for i := 0; i < workerCount; i++ { wg.Add(1) go func() { for {