GODRIVER-3419 Use a semaphore to limit concurrent conn creation #2098
Conversation
API Change ReportNo changes found! |
| func (p *pool) waitForNewConn(ctx context.Context) (*wantConn, *connection, bool) { | ||
| p.createConnectionsCond.L.Lock() | ||
| defer p.createConnectionsCond.L.Unlock() | ||
|
|
||
| for !(p.checkOutWaiting() && p.hasSpace()) && ctx.Err() == nil { | ||
| p.createConnectionsCond.Wait() | ||
| } |
There was a problem hiding this comment.
Waiting on the createConnectionsCond may cause an unbounded number of goroutines to be paused here.
For example, consider a case where the connection pool is full, so p.hasSpace() always returns false, no connections are closing, and all connections are in-use. A new operation calls checkOut and blocks at p.createConnectionsCond.Wait() until something signals the cond. It seems that a subsequent call to queueForNewConn should signal one of the waiting goroutines, but the behavior of sync.Cond.Signal is nondeterministic, so it's not clear if that will always happen.
Instead, we could immediately stop trying to create a new connection if the pool is full. The checkOut behavior would be the same except if the pool is full, that checkOut would no longer trigger a new connection if another connection is closed while the checkOut is waiting.
My intuition is that the latter case is lower risk, but it's not clear if goroutines getting "stuck" would even be a problem. What do you think?
GODRIVER-3419
Summary
This PR refactors the connection pool to eliminate the persistent maxConnection goroutines spawns at pool construction. Here we adopt a more lightweight, on-demand model inspired by @matthewdale's POC c26a3cb. Each new goroutine is responsible for creating exactly one connection and then terminates immediately.
Background & Motivation
While so-far not reproducible locally, the "spawn-and-done" approach suggested by this PR intends to avoid any long-term memory allocation patterns causing the GC to thrash. See ticket for more information.