net/http: http.Client calls can stall when a persistent connection is slow to close

[aqua]

go1.12's http.Client supports supplying a customized RoundTripper as the transport. A custom Dial can be supplied, e.g., to implement tunneling or proxying through nonstandard protocols, and returns objects implmenting net.Conn, which in turn includes a Close(). The native implementation of Close() on UNIX is a close(2) on the underlying socket, which is a local syscall and rarely blocks, especially in HTTP. However, if the supplied Dial returns a net.Conn with a longer shutdown (e.g. because it's tunneling over another protocol that requires doing a round trip on the underlying protocol to gracefully terminate), it can block for longer - potentially as long as the network RTT to a distant server plus that server's internal processing time.

The http implementation tries to keep persistent connections around, and when a new request is initiated to a "key" (scheme+protocol+host+port), it has an LRU cache of idle connections to choose from in getIdleConn() (https://golang.org/src/net/http/transport.go#L829). Before choosing one, it calls isBroken() on the corresponding http.persistConn (https://golang.org/src/net/http/transport.go#L848) in case the connection had just died an instant before. However, isBroken() requires acquiring the persistConn's mutex (https://golang.org/src/net/http/transport.go#L1533) before returning the nil-ness of persistConn.closed. That same mutex is held by persistConn.ReadLoop() (https://golang.org/src/net/http/transport.go#L1642), which can call pc.readLoopPeekFailLocked(), which calls pc.closeLocked(), which finally calls Close() on the net.Conn. This isn't a deadlock, but it does block with the persistConn.mu lock held for as long as it takes that Close() to finish, and so any calls to getIdleConn() that end up picking that connection will end up blocking as well. getIdleConn() removed the pconn from its idleLRU cache, but did not remove it from its list of idle connections, so it may end up getting picked.

Even for cases without a custom transport, there might be a tail-case optimization here for clients that make repeated use of the same HTTP server endpoint with finite re-usability of the TCP connections that could currently block bystander threads unnecessarily on the close() syscall.

Reference Google-internal bug 128442309.

[gopherbot]

Change https://golang.org/cl/168345 mentions this issue: net/http: make Transport call net.Conn.Close without mutex held

[martin-dos-santos]

any updates on this?