diff --git a/CHANGELOG.md b/CHANGELOG.md index 02f3d08..b97d785 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Unreleased * Fix: Resource.Destroy no longer removes itself from the pool before its destructor has completed. +* Fix: Prevent crash when pool is closed while resource is being created. # 1.1.1 (April 2, 2020) diff --git a/pool.go b/pool.go index e22b43a..a3b341c 100644 --- a/pool.go +++ b/pool.go @@ -400,6 +400,7 @@ func (p *Pool) CreateResource(ctx context.Context) error { value: value, lastUsedNano: nanotime(), } + p.destructWG.Add(1) p.cond.L.Lock() // If closed while constructing resource then destroy it and return an error @@ -409,7 +410,6 @@ func (p *Pool) CreateResource(ctx context.Context) error { } p.allResources = append(p.allResources, res) p.idleResources = append(p.idleResources, res) - p.destructWG.Add(1) p.cond.L.Unlock() return nil diff --git a/pool_test.go b/pool_test.go index b05e32d..074aa47 100644 --- a/pool_test.go +++ b/pool_test.go @@ -245,6 +245,13 @@ func TestPoolAcquireAllIdle(t *testing.T) { resources[3].Release() } +func TestPoolAcquireAllIdleWhenClosedIsNil(t *testing.T) { + constructor, _ := createConstructor() + pool := puddle.NewPool(constructor, stubDestructor, 10) + pool.Close() + assert.Nil(t, pool.AcquireAllIdle()) +} + func TestPoolCreateResource(t *testing.T) { constructor, counter := createConstructor() pool := puddle.NewPool(constructor, stubDestructor, 10) @@ -280,6 +287,36 @@ func TestPoolCreateResourceReturnsErrorFromFailedResourceCreate(t *testing.T) { assert.Equal(t, errCreateFailed, err) } +func TestPoolCreateResourceReturnsErrorWhenAlreadyClosed(t *testing.T) { + constructor, _ := createConstructor() + pool := puddle.NewPool(constructor, stubDestructor, 10) + pool.Close() + err := pool.CreateResource(context.Background()) + assert.Equal(t, puddle.ErrClosedPool, err) +} + +func TestPoolCreateResourceReturnsErrorWhenClosedWhileCreatingResource(t *testing.T) { + // There is no way to guarantee the correct order of the pool being closed while the resource is being constructed. + // But these sleeps should make it extremely likely. (Ah, the lengths we go for 100% test coverage...) + constructor := func(ctx context.Context) (interface{}, error) { + time.Sleep(500 * time.Millisecond) + return "abc", nil + } + pool := puddle.NewPool(constructor, stubDestructor, 10) + + acquireErrChan := make(chan error) + go func() { + err := pool.CreateResource(context.Background()) + acquireErrChan <- err + }() + + time.Sleep(250 * time.Millisecond) + pool.Close() + + err := <-acquireErrChan + assert.Equal(t, puddle.ErrClosedPool, err) +} + func TestPoolCloseClosesAllIdleResources(t *testing.T) { constructor, _ := createConstructor() @@ -333,6 +370,15 @@ func TestPoolCloseBlocksUntilAllResourcesReleasedAndClosed(t *testing.T) { assert.Equal(t, len(resources), destructorCalls.Value()) } +func TestPoolCloseIsSafeToCallMultipleTimes(t *testing.T) { + constructor, _ := createConstructor() + + p := puddle.NewPool(constructor, stubDestructor, 10) + + p.Close() + p.Close() +} + func TestPoolStatResources(t *testing.T) { startWaitChan := make(chan struct{}) waitingChan := make(chan struct{})