Skip to content

Commit

Permalink
http2: avoid Transport hang with Connection: close and AllowHTTP
Browse files Browse the repository at this point in the history
CL 111835 changed Transport stream ID numbering to start at
stream 3 when AllowHTTP is set. This was based on a
misunderstanding:

When a connection upgrades an HTTP/1.1 request to HTTP/2,
the initial HTTP/1.1 request occupies stream 1.
However, Transport does not perform HTTP protocol upgrades.
When using a Transport to send unencrypted HTTP/2 requests,
the entire connection uses HTTP/2, the first request is
sent as HTTP/2, and there is no reason not to use stream 1
for this request.

Starting from stream 3 is mostly harmless,
but ClientConn.idleStateLocked assumes that client streams
start from 1. This causes it to misidentify new single-use
connections as having already sent a request (when AllowHTTP
is set), and therefore not suitable for use.

Revert to always starting stream IDs at 1.

Fixes golang/go#67671

Change-Id: I97c89de4ae49623d916f9dbd200f8252d2fd4247
Reviewed-on: https://go-review.googlesource.com/c/net/+/591275
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
  • Loading branch information
neild committed Jun 12, 2024
1 parent 66e838c commit 9617c63
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 4 deletions.
4 changes: 0 additions & 4 deletions http2/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -827,10 +827,6 @@ func (t *Transport) newClientConn(c net.Conn, singleUse bool) (*ClientConn, erro
cc.henc.SetMaxDynamicTableSizeLimit(t.maxEncoderHeaderTableSize())
cc.peerMaxHeaderTableSize = initialHeaderTableSize

if t.AllowHTTP {
cc.nextStreamID = 3
}

if cs, ok := c.(connectionStater); ok {
state := cs.ConnectionState()
cc.tlsState = &state
Expand Down
20 changes: 20 additions & 0 deletions http2/transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5401,3 +5401,23 @@ func TestIssue66763Race(t *testing.T) {

<-donec
}

// Issue 67671: Sending a Connection: close request on a Transport with AllowHTTP
// set caused a the transport to wedge.
func TestIssue67671(t *testing.T) {
ts := newTestServer(t, func(w http.ResponseWriter, r *http.Request) {})
tr := &Transport{
TLSClientConfig: tlsConfigInsecure,
AllowHTTP: true,
}
defer tr.CloseIdleConnections()
req, _ := http.NewRequest("GET", ts.URL, nil)
req.Close = true
for i := 0; i < 2; i++ {
res, err := tr.RoundTrip(req)
if err != nil {
t.Fatal(err)
}
res.Body.Close()
}
}

0 comments on commit 9617c63

Please sign in to comment.