HostClient: lost wakeup between AcquireConn pool check and waiter registration
Summary
HostClient.AcquireConn can miss a returned connection between checking the pool and registering its waiter when MaxConnWaitTimeout > 0. The request can then wait until its timeout and return ErrNoFreeConns (or ErrTimeout when the request timeout overrides the wait timeout), even though a connection is idle.
Verified against current master, commit 8bb4f7a4fdd42017d2d1a7b24d2e0f19df71b8c4.
Interleaving
For example, with MaxConns: 1 and the sole connection currently in use:
- Goroutine A calls
AcquireConn, observes no idle connections and a full pool, and releasesconnsLock. - Before A calls
queueForIdle, goroutine B callsReleaseConn. There is no registered waiter yet, so B appends the connection toc.conns. - A calls
queueForIdle, which acquires the lock but only appends the waiter; it does not recheck idle connections. - A waits on
w.readywhile the connection is already idle. Without a subsequent pool event that delivers a connection, A times out.
The relevant pool check, unlock, and subsequent waiter registration are separate critical sections. ReleaseConn can run between them.
There is an analogous capacity case: if an in-use connection closes and decConnsCount frees capacity during this gap, the subsequently registered waiter does not initiate a replacement dial despite capacity being available.
This is a synchronization/lost-wakeup race, not necessarily a data race detectable by go test -race.
Deterministic reproducer
Place the following test in the repository root, in package fasthttp. It models the legal interleaving at the internal waiter-registration boundary, avoiding timing-dependent goroutine scheduling. It does not drive a complete HTTP request.
package fasthttp
import (
"testing"
"time"
)
func TestWaiterRegistrationRechecksIdle(t *testing.T) {
c := &HostClient{
MaxConns: 1,
MaxConnWaitTimeout: time.Second,
connsCount: 1,
}
// AcquireConn has observed no idle connections and a full pool,
// then unlocked connsLock, but has not called queueForIdle yet.
cc := &clientConn{}
c.ReleaseConn(cc)
// Resume the pending AcquireConn at its waiter-registration step.
w := &wantConn{ready: make(chan struct{}, 1)}
c.queueForIdle(w)
select {
case <-w.ready:
if w.conn != cc || w.err != nil {
t.Fatalf("unexpected delivery: conn=%p err=%v", w.conn, w.err)
}
default:
t.Fatalf("waiter remains blocked despite %d idle connection(s)", len(c.conns))
}
}Run:
go test -run '^TestWaiterRegistrationRechecksIdle$' -count=1Observed on the master commit above:
--- FAIL: TestWaiterRegistrationRechecksIdle (0.00s)
waiter_registration_test.go:28: waiter remains blocked despite 1 idle connection(s)Expected: the pending acquisition should consume the available connection instead of remaining queued.
Possible fix
Make the decision to wait atomic with the pool check, or recheck both idle connections and available connection capacity under connsLock immediately before enqueueing. The latter would deliver an idle connection, reserve capacity and start a dial, or enqueue only when neither is available. Pool strategy and cancellation behavior need to be preserved.
The waiter-registration gap appears to date back to #764. The fixes in #1831 (delivery past a cancelled waiter) and #2266 (failed-waiter bookkeeping) address different races and leave this gap in current master.
Source: valyala/fasthttp