net/http: bug in late binding

[dvyukov]

Late binding does the following (putIdleConn):

waitingDialer := t.idleConnCh[key]
select {
case waitingDialer <- pconn:
    t.idleMu.Unlock()
    return true
default:
    if waitingDialer != nil {
        delete(t.idleConnCh, key)
    }
}

However, receive in getConn is not atomic with creation of the idleConnCh channel, so it is possible that:

• getConn inserts a chan into idleConnCh
• putIdleConn gets the chan, but the non-blocking send fails
• getConn blocks on receive from idleConnCh

So when we need a conn the most, we actually miss the opportunity to match getConn and putIdleConn.

Another bad consequence is that putIdleConn will delete the chan from t.idleConnCh in this situation. This can disable late binding for a set of concurrent getConn's -- subsequent putIdleConn's won't hand off connections to them.

@bradfitz

[bradfitz]

This whole mechanism is best effort anyway, so I don't think this is a big deal. The logical race (not a data race) window is very small, and subsequent getConn calls will restore any channel that is deleted too early. The network round-trip time is so much larger than the goroutine scheduling time between locking a mutex, doing a map lookup, unlocking a mutex, and doing a receive-or-default-out unbuffered channel read.

I looked into this, but I think any fix would be more gross and invasive than it would be beneficial.

I'd rather not fix this, so I'm going to close it. If you have a minimal fix, I'd be open to looking at it, but I'd rather focus my time on HTTP/2 stuff right now.