From 9617c6335bca5e4e80949a5b1dbe43273260e8a3 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 6 Jun 2024 15:44:05 -0700 Subject: [PATCH] http2: avoid Transport hang with Connection: close and AllowHTTP 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 Reviewed-by: Jonathan Amsterdam --- http2/transport.go | 4 ---- http2/transport_test.go | 20 ++++++++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/http2/transport.go b/http2/transport.go index 98a49c6b6..61f511f97 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -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 diff --git a/http2/transport_test.go b/http2/transport_test.go index ddeaf6137..498e27932 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -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() + } +}