From 6cc5ac4e9a03d73b331eb1d6db98a02e558243b7 Mon Sep 17 00:00:00 2001 From: Gopher Robot Date: Fri, 4 Oct 2024 16:00:50 +0000 Subject: [PATCH 01/11] go.mod: update golang.org/x dependencies Update golang.org/x dependencies to their latest tagged versions. Change-Id: I57613519f6e5795e60d258865e81f6954d672606 Reviewed-on: https://go-review.googlesource.com/c/net/+/617959 Reviewed-by: David Chase Auto-Submit: Gopher Robot Reviewed-by: Dmitri Shuralyov LUCI-TryBot-Result: Go LUCI --- go.mod | 8 ++++---- go.sum | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/go.mod b/go.mod index ffda816a8..77935d3f2 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,8 @@ module golang.org/x/net go 1.18 require ( - golang.org/x/crypto v0.27.0 - golang.org/x/sys v0.25.0 - golang.org/x/term v0.24.0 - golang.org/x/text v0.18.0 + golang.org/x/crypto v0.28.0 + golang.org/x/sys v0.26.0 + golang.org/x/term v0.25.0 + golang.org/x/text v0.19.0 ) diff --git a/go.sum b/go.sum index a1a56bf3d..1b8c61d60 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,8 @@ -golang.org/x/crypto v0.27.0 h1:GXm2NjJrPaiv/h1tb2UH8QfgC/hOf/+z0p6PT8o1w7A= -golang.org/x/crypto v0.27.0/go.mod h1:1Xngt8kV6Dvbssa53Ziq6Eqn0HqbZi5Z6R0ZpwQzt70= -golang.org/x/sys v0.25.0 h1:r+8e+loiHxRqhXVl6ML1nO3l1+oFoWbnlu2Ehimmi34= -golang.org/x/sys v0.25.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/term v0.24.0 h1:Mh5cbb+Zk2hqqXNO7S1iTjEphVL+jb8ZWaqh/g+JWkM= -golang.org/x/term v0.24.0/go.mod h1:lOBK/LVxemqiMij05LGJ0tzNr8xlmwBRJ81PX6wVLH8= -golang.org/x/text v0.18.0 h1:XvMDiNzPAl0jr17s6W9lcaIhGUfUORdGCNsuLmPG224= -golang.org/x/text v0.18.0/go.mod h1:BuEKDfySbSR4drPmRPG/7iBdf8hvFMuRexcpahXilzY= +golang.org/x/crypto v0.28.0 h1:GBDwsMXVQi34v5CCYUm2jkJvu4cbtru2U4TN2PSyQnw= +golang.org/x/crypto v0.28.0/go.mod h1:rmgy+3RHxRZMyY0jjAJShp2zgEdOqj2AO7U0pYmeQ7U= +golang.org/x/sys v0.26.0 h1:KHjCJyddX0LoSTb3J+vWpupP9p0oznkqVk/IfjymZbo= +golang.org/x/sys v0.26.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/term v0.25.0 h1:WtHI/ltw4NvSUig5KARz9h521QvRC8RmF/cuYqifU24= +golang.org/x/term v0.25.0/go.mod h1:RPyXicDX+6vLxogjjRxjgD2TKtmAO6NZBsBRfrOLu7M= +golang.org/x/text v0.19.0 h1:kTxAhCbGbxhK0IwgSKiMO5awPoDQ0RpfiVYBfK860YM= +golang.org/x/text v0.19.0/go.mod h1:BuEKDfySbSR4drPmRPG/7iBdf8hvFMuRexcpahXilzY= From 42b11863606139133313f265e6cf2a4d1d8ca972 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 1 Mar 2023 15:35:42 -0800 Subject: [PATCH 02/11] http2: support ResponseController.EnableFullDuplex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ResponseController.EnableFullDuplex method indicates that an HTTP handler intends to interleave reads from a request body with writes to the response body. Add an EnableFullDuplex method to the ResponseWriter so we don't return a not-supported error. The HTTP/2 server always supports full duplex, so this is a no-op. For golang/go#57786 Change-Id: I6529e6ce01d59b8b48fb67ba7c244255df57c174 Reviewed-on: https://go-review.googlesource.com/c/net/+/472717 Reviewed-by: Cherry Mui LUCI-TryBot-Result: Go LUCI Reviewed-by: Brad Fitzpatrick Reviewed-by: Дарья Бочкар --- http2/server.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/http2/server.go b/http2/server.go index 617b4a476..d6d8ac88b 100644 --- a/http2/server.go +++ b/http2/server.go @@ -2880,6 +2880,11 @@ func (w *responseWriter) SetWriteDeadline(deadline time.Time) error { return nil } +func (w *responseWriter) EnableFullDuplex() error { + // We always support full duplex responses, so this is a no-op. + return nil +} + func (w *responseWriter) Flush() { w.FlushError() } From 5716b9813d2c78aa3bb6e08160517facfb2e84e6 Mon Sep 17 00:00:00 2001 From: cuishuang Date: Fri, 11 Oct 2024 14:23:01 +0800 Subject: [PATCH 03/11] internal/socket: execute gofmt Change-Id: Ifc793d535c31da3ba183ee44e1808e0072d7f099 Reviewed-on: https://go-review.googlesource.com/c/net/+/619595 Reviewed-by: Damien Neil Auto-Submit: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Ian Lance Taylor --- internal/socket/zsys_openbsd_ppc64.go | 28 ++++++++++++------------- internal/socket/zsys_openbsd_riscv64.go | 28 ++++++++++++------------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/internal/socket/zsys_openbsd_ppc64.go b/internal/socket/zsys_openbsd_ppc64.go index cebde7634..3c9576e2d 100644 --- a/internal/socket/zsys_openbsd_ppc64.go +++ b/internal/socket/zsys_openbsd_ppc64.go @@ -4,27 +4,27 @@ package socket type iovec struct { - Base *byte - Len uint64 + Base *byte + Len uint64 } type msghdr struct { - Name *byte - Namelen uint32 - Iov *iovec - Iovlen uint32 - Control *byte - Controllen uint32 - Flags int32 + Name *byte + Namelen uint32 + Iov *iovec + Iovlen uint32 + Control *byte + Controllen uint32 + Flags int32 } type cmsghdr struct { - Len uint32 - Level int32 - Type int32 + Len uint32 + Level int32 + Type int32 } const ( - sizeofIovec = 0x10 - sizeofMsghdr = 0x30 + sizeofIovec = 0x10 + sizeofMsghdr = 0x30 ) diff --git a/internal/socket/zsys_openbsd_riscv64.go b/internal/socket/zsys_openbsd_riscv64.go index cebde7634..3c9576e2d 100644 --- a/internal/socket/zsys_openbsd_riscv64.go +++ b/internal/socket/zsys_openbsd_riscv64.go @@ -4,27 +4,27 @@ package socket type iovec struct { - Base *byte - Len uint64 + Base *byte + Len uint64 } type msghdr struct { - Name *byte - Namelen uint32 - Iov *iovec - Iovlen uint32 - Control *byte - Controllen uint32 - Flags int32 + Name *byte + Namelen uint32 + Iov *iovec + Iovlen uint32 + Control *byte + Controllen uint32 + Flags int32 } type cmsghdr struct { - Len uint32 - Level int32 - Type int32 + Len uint32 + Level int32 + Type int32 } const ( - sizeofIovec = 0x10 - sizeofMsghdr = 0x30 + sizeofIovec = 0x10 + sizeofMsghdr = 0x30 ) From 4783315416d92ff3d4664762748bd21776b42b98 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Mon, 23 Sep 2024 15:06:22 -0700 Subject: [PATCH 04/11] http2: limit 1xx based on size, do not limit when delivered Replace Transport's limit of 5 1xx responses with a limit based on the maximum header size: The total size of all 1xx response headers must not exceed the limit we use on the size of the final response headers. (This differs slightly from the corresponding HTTP/1 change, which imposes a limit on all 1xx response headers *plus* the final response headers. The difference isn't substantial, and this implementation fits better with the HTTP/2 framer.) When the user is reading 1xx responses using a Got1xxResponse client trace hook, disable the limit: Each 1xx response is individually limited by the header size limit, but there is no limit on the total number of responses. The user is responsible for imposing a limit if they want one. For golang/go#65035 Change-Id: I9c19dbf068e0f580789d952f63113b3d21ad86fc Reviewed-on: https://go-review.googlesource.com/c/net/+/615295 Reviewed-by: Cherry Mui LUCI-TryBot-Result: Go LUCI Auto-Submit: Damien Neil Reviewed-by: Brad Fitzpatrick --- http2/transport.go | 41 ++++++++++++++----- http2/transport_test.go | 91 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 11 deletions(-) diff --git a/http2/transport.go b/http2/transport.go index 0c5f64aa8..e989bd19e 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -420,12 +420,12 @@ type clientStream struct { sentHeaders bool // owned by clientConnReadLoop: - firstByte bool // got the first response byte - pastHeaders bool // got first MetaHeadersFrame (actual headers) - pastTrailers bool // got optional second MetaHeadersFrame (trailers) - num1xx uint8 // number of 1xx responses seen - readClosed bool // peer sent an END_STREAM flag - readAborted bool // read loop reset the stream + firstByte bool // got the first response byte + pastHeaders bool // got first MetaHeadersFrame (actual headers) + pastTrailers bool // got optional second MetaHeadersFrame (trailers) + readClosed bool // peer sent an END_STREAM flag + readAborted bool // read loop reset the stream + totalHeaderSize int64 // total size of 1xx headers seen trailer http.Header // accumulated trailers resTrailer *http.Header // client's Response.Trailer @@ -2494,15 +2494,34 @@ func (rl *clientConnReadLoop) handleResponse(cs *clientStream, f *MetaHeadersFra if f.StreamEnded() { return nil, errors.New("1xx informational response with END_STREAM flag") } - cs.num1xx++ - const max1xxResponses = 5 // arbitrary bound on number of informational responses, same as net/http - if cs.num1xx > max1xxResponses { - return nil, errors.New("http2: too many 1xx informational responses") - } if fn := cs.get1xxTraceFunc(); fn != nil { + // If the 1xx response is being delivered to the user, + // then they're responsible for limiting the number + // of responses. if err := fn(statusCode, textproto.MIMEHeader(header)); err != nil { return nil, err } + } else { + // If the user didn't examine the 1xx response, then we + // limit the size of all 1xx headers. + // + // This differs a bit from the HTTP/1 implementation, which + // limits the size of all 1xx headers plus the final response. + // Use the larger limit of MaxHeaderListSize and + // net/http.Transport.MaxResponseHeaderBytes. + limit := int64(cs.cc.t.maxHeaderListSize()) + if t1 := cs.cc.t.t1; t1 != nil && t1.MaxResponseHeaderBytes > limit { + limit = t1.MaxResponseHeaderBytes + } + for _, h := range f.Fields { + cs.totalHeaderSize += int64(h.Size()) + } + if cs.totalHeaderSize > limit { + if VerboseLogs { + log.Printf("http2: 1xx informational responses too large") + } + return nil, errors.New("header list too large") + } } if statusCode == 100 { traceGot100Continue(cs.trace) diff --git a/http2/transport_test.go b/http2/transport_test.go index 498e27932..757a45a7a 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -5421,3 +5421,94 @@ func TestIssue67671(t *testing.T) { res.Body.Close() } } + +func TestTransport1xxLimits(t *testing.T) { + for _, test := range []struct { + name string + opt any + ctxfn func(context.Context) context.Context + hcount int + limited bool + }{{ + name: "default", + hcount: 10, + limited: false, + }, { + name: "MaxHeaderListSize", + opt: func(tr *Transport) { + tr.MaxHeaderListSize = 10000 + }, + hcount: 10, + limited: true, + }, { + name: "MaxResponseHeaderBytes", + opt: func(tr *http.Transport) { + tr.MaxResponseHeaderBytes = 10000 + }, + hcount: 10, + limited: true, + }, { + name: "limit by client trace", + ctxfn: func(ctx context.Context) context.Context { + count := 0 + return httptrace.WithClientTrace(ctx, &httptrace.ClientTrace{ + Got1xxResponse: func(code int, header textproto.MIMEHeader) error { + count++ + if count >= 10 { + return errors.New("too many 1xx") + } + return nil + }, + }) + }, + hcount: 10, + limited: true, + }, { + name: "limit disabled by client trace", + opt: func(tr *Transport) { + tr.MaxHeaderListSize = 10000 + }, + ctxfn: func(ctx context.Context) context.Context { + return httptrace.WithClientTrace(ctx, &httptrace.ClientTrace{ + Got1xxResponse: func(code int, header textproto.MIMEHeader) error { + return nil + }, + }) + }, + hcount: 20, + limited: false, + }} { + t.Run(test.name, func(t *testing.T) { + tc := newTestClientConn(t, test.opt) + tc.greet() + + ctx := context.Background() + if test.ctxfn != nil { + ctx = test.ctxfn(ctx) + } + req, _ := http.NewRequestWithContext(ctx, "GET", "https://dummy.tld/", nil) + rt := tc.roundTrip(req) + tc.wantFrameType(FrameHeaders) + + for i := 0; i < test.hcount; i++ { + if fr, err := tc.fr.ReadFrame(); err != os.ErrDeadlineExceeded { + t.Fatalf("after writing %v 1xx headers: read %v, %v; want idle", i, fr, err) + } + tc.writeHeaders(HeadersFrameParam{ + StreamID: rt.streamID(), + EndHeaders: true, + EndStream: false, + BlockFragment: tc.makeHeaderBlockFragment( + ":status", "103", + "x-field", strings.Repeat("a", 1000), + ), + }) + } + if test.limited { + tc.wantFrameType(FrameRSTStream) + } else { + tc.wantIdle() + } + }) + } +} From 511cc3a40645a2f6ed3d21a1d0803b5057b9aaa1 Mon Sep 17 00:00:00 2001 From: Carlana Johnson Date: Tue, 29 Oct 2024 01:29:34 +0000 Subject: [PATCH 05/11] html: add Node.{Ancestors,ChildNodes,Descendants}() Adds iterators for the parents, immediate children, and all children of a Node respectively. Fixes golang/go#62113 Change-Id: Iab015872cc3a20fe5e7cae3bc90b89cba68cc3f8 GitHub-Last-Rev: d99de580ab8c30bb04c9fee401d6bfc5dd55d1ec GitHub-Pull-Request: golang/net#215 Reviewed-on: https://go-review.googlesource.com/c/net/+/594195 Reviewed-by: Ian Lance Taylor LUCI-TryBot-Result: Go LUCI Auto-Submit: Ian Lance Taylor Reviewed-by: Damien Neil --- html/doc.go | 7 +--- html/example_test.go | 13 +++--- html/iter.go | 56 ++++++++++++++++++++++++++ html/iter_test.go | 96 ++++++++++++++++++++++++++++++++++++++++++++ html/node.go | 4 ++ 5 files changed, 163 insertions(+), 13 deletions(-) create mode 100644 html/iter.go create mode 100644 html/iter_test.go diff --git a/html/doc.go b/html/doc.go index 3a7e5ab17..885c4c593 100644 --- a/html/doc.go +++ b/html/doc.go @@ -78,16 +78,11 @@ example, to process each anchor node in depth-first order: if err != nil { // ... } - var f func(*html.Node) - f = func(n *html.Node) { + for n := range doc.Descendants() { if n.Type == html.ElementNode && n.Data == "a" { // Do something with n... } - for c := n.FirstChild; c != nil; c = c.NextSibling { - f(c) - } } - f(doc) The relevant specifications include: https://html.spec.whatwg.org/multipage/syntax.html and diff --git a/html/example_test.go b/html/example_test.go index 0b06ed773..830f0b27a 100644 --- a/html/example_test.go +++ b/html/example_test.go @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. +//go:build go1.23 + // This example demonstrates parsing HTML data and walking the resulting tree. package html_test @@ -11,6 +13,7 @@ import ( "strings" "golang.org/x/net/html" + "golang.org/x/net/html/atom" ) func ExampleParse() { @@ -19,9 +22,8 @@ func ExampleParse() { if err != nil { log.Fatal(err) } - var f func(*html.Node) - f = func(n *html.Node) { - if n.Type == html.ElementNode && n.Data == "a" { + for n := range doc.Descendants() { + if n.Type == html.ElementNode && n.DataAtom == atom.A { for _, a := range n.Attr { if a.Key == "href" { fmt.Println(a.Val) @@ -29,11 +31,8 @@ func ExampleParse() { } } } - for c := n.FirstChild; c != nil; c = c.NextSibling { - f(c) - } } - f(doc) + // Output: // foo // /bar/baz diff --git a/html/iter.go b/html/iter.go new file mode 100644 index 000000000..54be8fd30 --- /dev/null +++ b/html/iter.go @@ -0,0 +1,56 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +package html + +import "iter" + +// Ancestors returns an iterator over the ancestors of n, starting with n.Parent. +// +// Mutating a Node or its parents while iterating may have unexpected results. +func (n *Node) Ancestors() iter.Seq[*Node] { + _ = n.Parent // eager nil check + + return func(yield func(*Node) bool) { + for p := n.Parent; p != nil && yield(p); p = p.Parent { + } + } +} + +// ChildNodes returns an iterator over the immediate children of n, +// starting with n.FirstChild. +// +// Mutating a Node or its children while iterating may have unexpected results. +func (n *Node) ChildNodes() iter.Seq[*Node] { + _ = n.FirstChild // eager nil check + + return func(yield func(*Node) bool) { + for c := n.FirstChild; c != nil && yield(c); c = c.NextSibling { + } + } + +} + +// Descendants returns an iterator over all nodes recursively beneath +// n, excluding n itself. Nodes are visited in depth-first preorder. +// +// Mutating a Node or its descendants while iterating may have unexpected results. +func (n *Node) Descendants() iter.Seq[*Node] { + _ = n.FirstChild // eager nil check + + return func(yield func(*Node) bool) { + n.descendants(yield) + } +} + +func (n *Node) descendants(yield func(*Node) bool) bool { + for c := range n.ChildNodes() { + if !yield(c) || !c.descendants(yield) { + return false + } + } + return true +} diff --git a/html/iter_test.go b/html/iter_test.go new file mode 100644 index 000000000..cca7f82f5 --- /dev/null +++ b/html/iter_test.go @@ -0,0 +1,96 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +package html + +import ( + "strings" + "testing" +) + +func TestNode_ChildNodes(t *testing.T) { + tests := []struct { + in string + want string + }{ + {"", ""}, + {"", "a"}, + {"a", "a"}, + {"", "a b"}, + {"ac", "a b c"}, + {"ad", "a b d"}, + {"cefi", "a f g h"}, + } + for _, test := range tests { + doc, err := Parse(strings.NewReader(test.in)) + if err != nil { + t.Fatal(err) + } + // Drill to + n := doc.FirstChild.FirstChild.NextSibling + var results []string + for c := range n.ChildNodes() { + results = append(results, c.Data) + } + if got := strings.Join(results, " "); got != test.want { + t.Errorf("ChildNodes = %q, want %q", got, test.want) + } + } +} + +func TestNode_Descendants(t *testing.T) { + tests := []struct { + in string + want string + }{ + {"", ""}, + {"", "a"}, + {"", "a b"}, + {"b", "a b"}, + {"", "a b"}, + {"bd", "a b c d"}, + {"be", "a b c d e"}, + {"dfgj", "a b c d e f g h i j"}, + } + for _, test := range tests { + doc, err := Parse(strings.NewReader(test.in)) + if err != nil { + t.Fatal(err) + } + // Drill to + n := doc.FirstChild.FirstChild.NextSibling + var results []string + for c := range n.Descendants() { + results = append(results, c.Data) + } + if got := strings.Join(results, " "); got != test.want { + t.Errorf("Descendants = %q; want: %q", got, test.want) + } + } +} + +func TestNode_Ancestors(t *testing.T) { + for _, size := range []int{0, 1, 2, 10, 100, 10_000} { + n := buildChain(size) + nParents := 0 + for _ = range n.Ancestors() { + nParents++ + } + if nParents != size { + t.Errorf("number of Ancestors = %d; want: %d", nParents, size) + } + } +} + +func buildChain(size int) *Node { + child := new(Node) + for range size { + parent := child + child = new(Node) + parent.AppendChild(child) + } + return child +} diff --git a/html/node.go b/html/node.go index 1350eef22..77741a195 100644 --- a/html/node.go +++ b/html/node.go @@ -38,6 +38,10 @@ var scopeMarker = Node{Type: scopeMarkerNode} // that it looks like "a Date: Thu, 31 Oct 2024 15:33:43 -0700 Subject: [PATCH 06/11] README: don't recommend go get These days people will just import the packages and the go tool will do the right thing. We don't need to explain it. Add a pointer to the git repo, though. For golang/go#62645 Change-Id: Ia5a16d8d66395e3feee2029ea1c3140b4d3939e7 Reviewed-on: https://go-review.googlesource.com/c/net/+/624175 Auto-Submit: Ian Lance Taylor LUCI-TryBot-Result: Go LUCI Reviewed-by: Damien Neil Reviewed-by: Ian Lance Taylor --- README.md | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index a15f253df..235c8d8b3 100644 --- a/README.md +++ b/README.md @@ -2,17 +2,15 @@ [![Go Reference](https://pkg.go.dev/badge/golang.org/x/net.svg)](https://pkg.go.dev/golang.org/x/net) -This repository holds supplementary Go networking libraries. +This repository holds supplementary Go networking packages. -## Download/Install +## Report Issues / Send Patches -The easiest way to install is to run `go get -u golang.org/x/net`. You can -also manually git clone the repository to `$GOPATH/src/golang.org/x/net`. +This repository uses Gerrit for code changes. To learn how to submit changes to +this repository, see https://go.dev/doc/contribute. -## Report Issues / Send Patches +The git repository is https://go.googlesource.com/net. -This repository uses Gerrit for code changes. To learn how to submit -changes to this repository, see https://golang.org/doc/contribute.html. The main issue tracker for the net repository is located at -https://github.com/golang/go/issues. Prefix your issue with "x/net:" in the +https://go.dev/issues. Prefix your issue with "x/net:" in the subject line, so it is easy to find. From f35fec92ec9213ee211cf45f451a5970386f7978 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 2 Oct 2024 16:45:27 -0700 Subject: [PATCH 07/11] http2: detect hung client connections by confirming stream resets Consider the case of an unresponsive client connection, where the server has stopped responding. We send an infinite sequence of requests to the connection in sequence, each with a timeout. Each request counts against the concurrency limit for the connection while active, but when a request times out we send a RST_STREAM and free up the concurrency slot it was using. We continue to try to send requests to the connection forever (or until the kernel closes the underlying TCP connection, or until ReadIdleTimeout/WriteByteTimeout results in us closing the connection). Defend against this scenario by counting a canceled request against the connection concurrency limit until we confirm the server is responding. Specifically: Track the number of in-flight request cancellations in cc.pendingResets. This total counts against the connection concurrency limit. When sending a RST_STREAM for a canceled request, increment cc.pendingResets. Send a PING frame to the server, unless a PING is already in flight. When receiving a PING response, set cc.pendingResets to 0. A hung connection will be used for at most SETTINGS_MAX_CONCURRENT_STREAMS requests. When StrictMaxConcurrentStreams is false, we will create a new connection after reaching the concurrency limit for a hung one. When StrictMaxConcurrentStreams is true, we will continue to wait for the existing connection until some timeout closes it or it becomes responsive again. For golang/go#59690 Change-Id: I0151f9a594af14b32bcb6005a239fa19eb103704 Reviewed-on: https://go-review.googlesource.com/c/net/+/617655 LUCI-TryBot-Result: Go LUCI Reviewed-by: Brad Fitzpatrick Reviewed-by: Jonathan Amsterdam Reviewed-by: Carlos Amedee --- http2/http2_test.go | 8 +++ http2/transport.go | 71 +++++++++++++++++++--- http2/transport_test.go | 126 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+), 7 deletions(-) diff --git a/http2/http2_test.go b/http2/http2_test.go index b7c946b98..b1e71f153 100644 --- a/http2/http2_test.go +++ b/http2/http2_test.go @@ -283,3 +283,11 @@ func TestNoUnicodeStrings(t *testing.T) { t.Fatal(err) } } + +// must returns v if err is nil, or panics otherwise. +func must[T any](v T, err error) T { + if err != nil { + panic(err) + } + return v +} diff --git a/http2/transport.go b/http2/transport.go index e989bd19e..5d198baa5 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -364,6 +364,14 @@ type ClientConn struct { readIdleTimeout time.Duration pingTimeout time.Duration + // pendingResets is the number of RST_STREAM frames we have sent to the peer, + // without confirming that the peer has received them. When we send a RST_STREAM, + // we bundle it with a PING frame, unless a PING is already in flight. We count + // the reset stream against the connection's concurrency limit until we get + // a PING response. This limits the number of requests we'll try to send to a + // completely unresponsive connection. + pendingResets int + // reqHeaderMu is a 1-element semaphore channel controlling access to sending new requests. // Write to reqHeaderMu to lock it, read from it to unlock. // Lock reqmu BEFORE mu or wmu. @@ -960,7 +968,7 @@ func (cc *ClientConn) State() ClientConnState { return ClientConnState{ Closed: cc.closed, Closing: cc.closing || cc.singleUse || cc.doNotReuse || cc.goAway != nil, - StreamsActive: len(cc.streams), + StreamsActive: len(cc.streams) + cc.pendingResets, StreamsReserved: cc.streamsReserved, StreamsPending: cc.pendingRequests, LastIdle: cc.lastIdle, @@ -992,7 +1000,13 @@ func (cc *ClientConn) idleStateLocked() (st clientConnIdleState) { // writing it. maxConcurrentOkay = true } else { - maxConcurrentOkay = int64(len(cc.streams)+cc.streamsReserved+1) <= int64(cc.maxConcurrentStreams) + // We can take a new request if the total of + // - active streams; + // - reservation slots for new streams; and + // - streams for which we have sent a RST_STREAM and a PING, + // but received no subsequent frame + // is less than the concurrency limit. + maxConcurrentOkay = cc.currentRequestCountLocked() < int(cc.maxConcurrentStreams) } st.canTakeNewRequest = cc.goAway == nil && !cc.closed && !cc.closing && maxConcurrentOkay && @@ -1002,6 +1016,12 @@ func (cc *ClientConn) idleStateLocked() (st clientConnIdleState) { return } +// currentRequestCountLocked reports the number of concurrency slots currently in use, +// including active streams, reserved slots, and reset streams waiting for acknowledgement. +func (cc *ClientConn) currentRequestCountLocked() int { + return len(cc.streams) + cc.streamsReserved + cc.pendingResets +} + func (cc *ClientConn) canTakeNewRequestLocked() bool { st := cc.idleStateLocked() return st.canTakeNewRequest @@ -1578,6 +1598,7 @@ func (cs *clientStream) cleanupWriteRequest(err error) { cs.reqBodyClosed = make(chan struct{}) } bodyClosed := cs.reqBodyClosed + closeOnIdle := cc.singleUse || cc.doNotReuse || cc.t.disableKeepAlives() || cc.goAway != nil cc.mu.Unlock() if mustCloseBody { cs.reqBody.Close() @@ -1602,16 +1623,40 @@ func (cs *clientStream) cleanupWriteRequest(err error) { if cs.sentHeaders { if se, ok := err.(StreamError); ok { if se.Cause != errFromPeer { - cc.writeStreamReset(cs.ID, se.Code, err) + cc.writeStreamReset(cs.ID, se.Code, false, err) } } else { - cc.writeStreamReset(cs.ID, ErrCodeCancel, err) + // We're cancelling an in-flight request. + // + // This could be due to the server becoming unresponsive. + // To avoid sending too many requests on a dead connection, + // we let the request continue to consume a concurrency slot + // until we can confirm the server is still responding. + // We do this by sending a PING frame along with the RST_STREAM + // (unless a ping is already in flight). + // + // For simplicity, we don't bother tracking the PING payload: + // We reset cc.pendingResets any time we receive a PING ACK. + // + // We skip this if the conn is going to be closed on idle, + // because it's short lived and will probably be closed before + // we get the ping response. + ping := false + if !closeOnIdle { + cc.mu.Lock() + if cc.pendingResets == 0 { + ping = true + } + cc.pendingResets++ + cc.mu.Unlock() + } + cc.writeStreamReset(cs.ID, ErrCodeCancel, ping, err) } } cs.bufPipe.CloseWithError(err) // no-op if already closed } else { if cs.sentHeaders && !cs.sentEndStream { - cc.writeStreamReset(cs.ID, ErrCodeNo, nil) + cc.writeStreamReset(cs.ID, ErrCodeNo, false, nil) } cs.bufPipe.CloseWithError(errRequestCanceled) } @@ -1638,7 +1683,7 @@ func (cc *ClientConn) awaitOpenSlotForStreamLocked(cs *clientStream) error { return errClientConnUnusable } cc.lastIdle = time.Time{} - if int64(len(cc.streams)) < int64(cc.maxConcurrentStreams) { + if cc.currentRequestCountLocked() < int(cc.maxConcurrentStreams) { return nil } cc.pendingRequests++ @@ -3065,6 +3110,11 @@ func (rl *clientConnReadLoop) processPing(f *PingFrame) error { close(c) delete(cc.pings, f.Data) } + if cc.pendingResets > 0 { + // See clientStream.cleanupWriteRequest. + cc.pendingResets = 0 + cc.cond.Broadcast() + } return nil } cc := rl.cc @@ -3087,13 +3137,20 @@ func (rl *clientConnReadLoop) processPushPromise(f *PushPromiseFrame) error { return ConnectionError(ErrCodeProtocol) } -func (cc *ClientConn) writeStreamReset(streamID uint32, code ErrCode, err error) { +// writeStreamReset sends a RST_STREAM frame. +// When ping is true, it also sends a PING frame with a random payload. +func (cc *ClientConn) writeStreamReset(streamID uint32, code ErrCode, ping bool, err error) { // TODO: map err to more interesting error codes, once the // HTTP community comes up with some. But currently for // RST_STREAM there's no equivalent to GOAWAY frame's debug // data, and the error codes are all pretty vague ("cancel"). cc.wmu.Lock() cc.fr.WriteRSTStream(streamID, code) + if ping { + var payload [8]byte + rand.Read(payload[:]) + cc.fr.WritePing(false, payload) + } cc.bw.Flush() cc.wmu.Unlock() } diff --git a/http2/transport_test.go b/http2/transport_test.go index 757a45a7a..f6ef295a4 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -2559,6 +2559,9 @@ func testTransportReturnsUnusedFlowControl(t *testing.T, oneDataFrame bool) { } return true }, + func(f *PingFrame) bool { + return true + }, func(f *WindowUpdateFrame) bool { if !oneDataFrame && !sentAdditionalData { t.Fatalf("Got WindowUpdateFrame, don't expect one yet") @@ -5512,3 +5515,126 @@ func TestTransport1xxLimits(t *testing.T) { }) } } + +func TestTransportSendPingWithReset(t *testing.T) { + tc := newTestClientConn(t, func(tr *Transport) { + tr.StrictMaxConcurrentStreams = true + }) + + const maxConcurrent = 3 + tc.greet(Setting{SettingMaxConcurrentStreams, maxConcurrent}) + + // Start several requests. + var rts []*testRoundTrip + for i := 0; i < maxConcurrent+1; i++ { + req := must(http.NewRequest("GET", "https://dummy.tld/", nil)) + rt := tc.roundTrip(req) + if i >= maxConcurrent { + tc.wantIdle() + continue + } + tc.wantFrameType(FrameHeaders) + tc.writeHeaders(HeadersFrameParam{ + StreamID: rt.streamID(), + EndHeaders: true, + BlockFragment: tc.makeHeaderBlockFragment( + ":status", "200", + ), + }) + rt.wantStatus(200) + rts = append(rts, rt) + } + + // Cancel one request. We send a PING frame along with the RST_STREAM. + rts[0].response().Body.Close() + tc.wantRSTStream(rts[0].streamID(), ErrCodeCancel) + pf := readFrame[*PingFrame](t, tc) + tc.wantIdle() + + // Cancel another request. No PING frame, since one is in flight. + rts[1].response().Body.Close() + tc.wantRSTStream(rts[1].streamID(), ErrCodeCancel) + tc.wantIdle() + + // Respond to the PING. + // This finalizes the previous resets, and allows the pending request to be sent. + tc.writePing(true, pf.Data) + tc.wantFrameType(FrameHeaders) + tc.wantIdle() + + // Cancel the last request. We send another PING, since none are in flight. + rts[2].response().Body.Close() + tc.wantRSTStream(rts[2].streamID(), ErrCodeCancel) + tc.wantFrameType(FramePing) + tc.wantIdle() +} + +func TestTransportConnBecomesUnresponsive(t *testing.T) { + // We send a number of requests in series to an unresponsive connection. + // Each request is canceled or times out without a response. + // Eventually, we open a new connection rather than trying to use the old one. + tt := newTestTransport(t) + + const maxConcurrent = 3 + + t.Logf("first request opens a new connection and succeeds") + req1 := must(http.NewRequest("GET", "https://dummy.tld/", nil)) + rt1 := tt.roundTrip(req1) + tc1 := tt.getConn() + tc1.wantFrameType(FrameSettings) + tc1.wantFrameType(FrameWindowUpdate) + hf1 := readFrame[*HeadersFrame](t, tc1) + tc1.writeSettings(Setting{SettingMaxConcurrentStreams, maxConcurrent}) + tc1.wantFrameType(FrameSettings) // ack + tc1.writeHeaders(HeadersFrameParam{ + StreamID: hf1.StreamID, + EndHeaders: true, + EndStream: true, + BlockFragment: tc1.makeHeaderBlockFragment( + ":status", "200", + ), + }) + rt1.wantStatus(200) + rt1.response().Body.Close() + + // Send more requests. + // None receive a response. + // Each is canceled. + for i := 0; i < maxConcurrent; i++ { + t.Logf("request %v receives no response and is canceled", i) + ctx, cancel := context.WithCancel(context.Background()) + req := must(http.NewRequestWithContext(ctx, "GET", "https://dummy.tld/", nil)) + tt.roundTrip(req) + if tt.hasConn() { + t.Fatalf("new connection created; expect existing conn to be reused") + } + tc1.wantFrameType(FrameHeaders) + cancel() + tc1.wantFrameType(FrameRSTStream) + if i == 0 { + tc1.wantFrameType(FramePing) + } + tc1.wantIdle() + } + + // The conn has hit its concurrency limit. + // The next request is sent on a new conn. + req2 := must(http.NewRequest("GET", "https://dummy.tld/", nil)) + rt2 := tt.roundTrip(req2) + tc2 := tt.getConn() + tc2.wantFrameType(FrameSettings) + tc2.wantFrameType(FrameWindowUpdate) + hf := readFrame[*HeadersFrame](t, tc2) + tc2.writeSettings(Setting{SettingMaxConcurrentStreams, maxConcurrent}) + tc2.wantFrameType(FrameSettings) // ack + tc2.writeHeaders(HeadersFrameParam{ + StreamID: hf.StreamID, + EndHeaders: true, + EndStream: true, + BlockFragment: tc2.makeHeaderBlockFragment( + ":status", "200", + ), + }) + rt2.wantStatus(200) + rt2.response().Body.Close() +} From 0aa844c2c8b6054d98e91f074355d5a50528934c Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Mon, 4 Nov 2024 13:54:41 -0800 Subject: [PATCH 08/11] http2: support unencrypted HTTP/2 handoff from net/http Allow net/http to pass unencrypted net.Conns to Server/Transport. We don't have an existing way to pass a conn other than a *tls.Conn into this package, so (ab)use TLSNextProto to pass unencrypted connections: The http2 package adds an "unencrypted_http2" entry to the TLSNextProto maps. The net/http package calls this function with a *tls.Conn wrapping a net.Conn with an UnencryptedNetConn method returning the underlying, unencrypted net.Conn. For golang/go#67816 Change-Id: I31f9c1ba31a17c82c8ed651382bd94193acf09b9 Reviewed-on: https://go-review.googlesource.com/c/net/+/625175 LUCI-TryBot-Result: Go LUCI Reviewed-by: David Chase Reviewed-by: Brad Fitzpatrick --- http2/client_conn_pool.go | 8 +++---- http2/server.go | 29 +++++++++++++++++++++----- http2/transport.go | 44 ++++++++++++++++++++++++++++++++------- http2/unencrypted.go | 32 ++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 17 deletions(-) create mode 100644 http2/unencrypted.go diff --git a/http2/client_conn_pool.go b/http2/client_conn_pool.go index 780968d6c..e81b73e6a 100644 --- a/http2/client_conn_pool.go +++ b/http2/client_conn_pool.go @@ -8,8 +8,8 @@ package http2 import ( "context" - "crypto/tls" "errors" + "net" "net/http" "sync" ) @@ -158,7 +158,7 @@ func (c *dialCall) dial(ctx context.Context, addr string) { // This code decides which ones live or die. // The return value used is whether c was used. // c is never closed. -func (p *clientConnPool) addConnIfNeeded(key string, t *Transport, c *tls.Conn) (used bool, err error) { +func (p *clientConnPool) addConnIfNeeded(key string, t *Transport, c net.Conn) (used bool, err error) { p.mu.Lock() for _, cc := range p.conns[key] { if cc.CanTakeNewRequest() { @@ -194,8 +194,8 @@ type addConnCall struct { err error } -func (c *addConnCall) run(t *Transport, key string, tc *tls.Conn) { - cc, err := t.NewClientConn(tc) +func (c *addConnCall) run(t *Transport, key string, nc net.Conn) { + cc, err := t.NewClientConn(nc) p := c.p p.mu.Lock() diff --git a/http2/server.go b/http2/server.go index d6d8ac88b..832414b45 100644 --- a/http2/server.go +++ b/http2/server.go @@ -306,7 +306,7 @@ func ConfigureServer(s *http.Server, conf *Server) error { if s.TLSNextProto == nil { s.TLSNextProto = map[string]func(*http.Server, *tls.Conn, http.Handler){} } - protoHandler := func(hs *http.Server, c *tls.Conn, h http.Handler) { + protoHandler := func(hs *http.Server, c net.Conn, h http.Handler, sawClientPreface bool) { if testHookOnConn != nil { testHookOnConn() } @@ -323,12 +323,31 @@ func ConfigureServer(s *http.Server, conf *Server) error { ctx = bc.BaseContext() } conf.ServeConn(c, &ServeConnOpts{ - Context: ctx, - Handler: h, - BaseConfig: hs, + Context: ctx, + Handler: h, + BaseConfig: hs, + SawClientPreface: sawClientPreface, }) } - s.TLSNextProto[NextProtoTLS] = protoHandler + s.TLSNextProto[NextProtoTLS] = func(hs *http.Server, c *tls.Conn, h http.Handler) { + protoHandler(hs, c, h, false) + } + // The "unencrypted_http2" TLSNextProto key is used to pass off non-TLS HTTP/2 conns. + // + // A connection passed in this method has already had the HTTP/2 preface read from it. + s.TLSNextProto[nextProtoUnencryptedHTTP2] = func(hs *http.Server, c *tls.Conn, h http.Handler) { + nc, err := unencryptedNetConnFromTLSConn(c) + if err != nil { + if lg := hs.ErrorLog; lg != nil { + lg.Print(err) + } else { + log.Print(err) + } + go c.Close() + return + } + protoHandler(hs, nc, h, true) + } return nil } diff --git a/http2/transport.go b/http2/transport.go index 5d198baa5..af0162f65 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -281,8 +281,8 @@ func configureTransports(t1 *http.Transport) (*Transport, error) { if !strSliceContains(t1.TLSClientConfig.NextProtos, "http/1.1") { t1.TLSClientConfig.NextProtos = append(t1.TLSClientConfig.NextProtos, "http/1.1") } - upgradeFn := func(authority string, c *tls.Conn) http.RoundTripper { - addr := authorityAddr("https", authority) + upgradeFn := func(scheme, authority string, c net.Conn) http.RoundTripper { + addr := authorityAddr(scheme, authority) if used, err := connPool.addConnIfNeeded(addr, t2, c); err != nil { go c.Close() return erringRoundTripper{err} @@ -293,18 +293,37 @@ func configureTransports(t1 *http.Transport) (*Transport, error) { // was unknown) go c.Close() } + if scheme == "http" { + return (*unencryptedTransport)(t2) + } return t2 } - if m := t1.TLSNextProto; len(m) == 0 { - t1.TLSNextProto = map[string]func(string, *tls.Conn) http.RoundTripper{ - "h2": upgradeFn, + if t1.TLSNextProto == nil { + t1.TLSNextProto = make(map[string]func(string, *tls.Conn) http.RoundTripper) + } + t1.TLSNextProto[NextProtoTLS] = func(authority string, c *tls.Conn) http.RoundTripper { + return upgradeFn("https", authority, c) + } + // The "unencrypted_http2" TLSNextProto key is used to pass off non-TLS HTTP/2 conns. + t1.TLSNextProto[nextProtoUnencryptedHTTP2] = func(authority string, c *tls.Conn) http.RoundTripper { + nc, err := unencryptedNetConnFromTLSConn(c) + if err != nil { + go c.Close() + return erringRoundTripper{err} } - } else { - m["h2"] = upgradeFn + return upgradeFn("http", authority, nc) } return t2, nil } +// unencryptedTransport is a Transport with a RoundTrip method that +// always permits http:// URLs. +type unencryptedTransport Transport + +func (t *unencryptedTransport) RoundTrip(req *http.Request) (*http.Response, error) { + return (*Transport)(t).RoundTripOpt(req, RoundTripOpt{allowHTTP: true}) +} + func (t *Transport) connPool() ClientConnPool { t.connPoolOnce.Do(t.initConnPool) return t.connPoolOrDef @@ -538,6 +557,8 @@ type RoundTripOpt struct { // no cached connection is available, RoundTripOpt // will return ErrNoCachedConn. OnlyCachedConn bool + + allowHTTP bool // allow http:// URLs } func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) { @@ -570,7 +591,14 @@ func authorityAddr(scheme string, authority string) (addr string) { // RoundTripOpt is like RoundTrip, but takes options. func (t *Transport) RoundTripOpt(req *http.Request, opt RoundTripOpt) (*http.Response, error) { - if !(req.URL.Scheme == "https" || (req.URL.Scheme == "http" && t.AllowHTTP)) { + switch req.URL.Scheme { + case "https": + // Always okay. + case "http": + if !t.AllowHTTP && !opt.allowHTTP { + return nil, errors.New("http2: unencrypted HTTP/2 not enabled") + } + default: return nil, errors.New("http2: unsupported scheme") } diff --git a/http2/unencrypted.go b/http2/unencrypted.go new file mode 100644 index 000000000..b2de21161 --- /dev/null +++ b/http2/unencrypted.go @@ -0,0 +1,32 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package http2 + +import ( + "crypto/tls" + "errors" + "net" +) + +const nextProtoUnencryptedHTTP2 = "unencrypted_http2" + +// unencryptedNetConnFromTLSConn retrieves a net.Conn wrapped in a *tls.Conn. +// +// TLSNextProto functions accept a *tls.Conn. +// +// When passing an unencrypted HTTP/2 connection to a TLSNextProto function, +// we pass a *tls.Conn with an underlying net.Conn containing the unencrypted connection. +// To be extra careful about mistakes (accidentally dropping TLS encryption in a place +// where we want it), the tls.Conn contains a net.Conn with an UnencryptedNetConn method +// that returns the actual connection we want to use. +func unencryptedNetConnFromTLSConn(tc *tls.Conn) (net.Conn, error) { + conner, ok := tc.NetConn().(interface { + UnencryptedNetConn() net.Conn + }) + if !ok { + return nil, errors.New("http2: TLS conn unexpectedly found in unencrypted handoff") + } + return conner.UnencryptedNetConn(), nil +} From 858db1a8c8f71cbefb8ff2c896ff9aa86d761a47 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Tue, 5 Nov 2024 11:26:18 -0800 Subject: [PATCH 09/11] http2: surface errors occurring very early in a client conn's lifetime When we create a new connection for a request, the request should fail if the connection attempt fails. There is a race condition which can cause this to not happen: - net/http sends a request to a http2.Transport - the http2.Transport returns ErrNoCachedConn - net/http creates a new tls.Conn and passes it to the http2.Transport - the http2.Transport adds the conn to its connection pool - the connection immediately encounters an error - the http2.Transport removes the conn from its connection pool - net/http resends the request to the http2.Transport - the http2.Transport returns ErrNoCachedConn, and the process repeates If the request is sent to the http2.Transport before the connection encounters an error, then the request fails. But otherwise, we get stuck in an infinite loop of the http2.Transport asking for a new connection, receiving one, and throwing it away. To fix this, leave a dead connection in the pool for a short while if it has never had a request sent to it. If a dead connection is used for a new request, return an error and remove the connection from the pool. Change-Id: I64eb15a8f1512a6bda52db423072b945fab6f4b5 Reviewed-on: https://go-review.googlesource.com/c/net/+/625398 Reviewed-by: Brad Fitzpatrick LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam --- http2/clientconn_test.go | 30 ++++++++--- http2/netconn_test.go | 10 +++- http2/transport.go | 86 +++++++++++++++++++++++++---- http2/transport_test.go | 113 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 219 insertions(+), 20 deletions(-) diff --git a/http2/clientconn_test.go b/http2/clientconn_test.go index 92d630514..42d9fd2dc 100644 --- a/http2/clientconn_test.go +++ b/http2/clientconn_test.go @@ -10,6 +10,7 @@ package http2 import ( "bytes" "context" + "crypto/tls" "fmt" "io" "net/http" @@ -112,27 +113,40 @@ func newTestClientConnFromClientConn(t *testing.T, cc *ClientConn) *testClientCo cc: cc, group: cc.t.transportTestHooks.group.(*synctestGroup), } - cli, srv := synctestNetPipe(tc.group) + + // srv is the side controlled by the test. + var srv *synctestNetConn + if cc.tconn == nil { + // If cc.tconn is nil, we're being called with a new conn created by the + // Transport's client pool. This path skips dialing the server, and we + // create a test connection pair here. + cc.tconn, srv = synctestNetPipe(tc.group) + } else { + // If cc.tconn is non-nil, we're in a test which provides a conn to the + // Transport via a TLSNextProto hook. Extract the test connection pair. + if tc, ok := cc.tconn.(*tls.Conn); ok { + // Unwrap any *tls.Conn to the underlying net.Conn, + // to avoid dealing with encryption in tests. + cc.tconn = tc.NetConn() + } + srv = cc.tconn.(*synctestNetConn).peer + } + srv.SetReadDeadline(tc.group.Now()) srv.autoWait = true tc.netconn = srv tc.enc = hpack.NewEncoder(&tc.encbuf) - - // all writes and reads are finished. - // - // cli is the ClientConn's side, srv is the side controlled by the test. - cc.tconn = cli tc.fr = NewFramer(srv, srv) tc.testConnFramer = testConnFramer{ t: t, fr: tc.fr, dec: hpack.NewDecoder(initialHeaderTableSize, nil), } - tc.fr.SetMaxReadFrameSize(10 << 20) t.Cleanup(func() { tc.closeWrite() }) + return tc } @@ -503,6 +517,8 @@ func newTestTransport(t *testing.T, opts ...any) *testTransport { o(tr.t1) case func(*Transport): o(tr) + case *Transport: + tr = o } } tt.tr = tr diff --git a/http2/netconn_test.go b/http2/netconn_test.go index 8a61fbef1..0f1b5fb1f 100644 --- a/http2/netconn_test.go +++ b/http2/netconn_test.go @@ -28,8 +28,11 @@ func synctestNetPipe(group *synctestGroup) (r, w *synctestNetConn) { s2addr := net.TCPAddrFromAddrPort(netip.MustParseAddrPort("127.0.0.1:8001")) s1 := newSynctestNetConnHalf(s1addr) s2 := newSynctestNetConnHalf(s2addr) - return &synctestNetConn{group: group, loc: s1, rem: s2}, - &synctestNetConn{group: group, loc: s2, rem: s1} + r = &synctestNetConn{group: group, loc: s1, rem: s2} + w = &synctestNetConn{group: group, loc: s2, rem: s1} + r.peer = w + w.peer = r + return r, w } // A synctestNetConn is one endpoint of the connection created by synctestNetPipe. @@ -43,6 +46,9 @@ type synctestNetConn struct { // When set, group.Wait is automatically called before reads and after writes. autoWait bool + + // peer is the other endpoint. + peer *synctestNetConn } // Read reads data from the connection. diff --git a/http2/transport.go b/http2/transport.go index af0162f65..f5968f440 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -202,6 +202,20 @@ func (t *Transport) markNewGoroutine() { } } +func (t *Transport) now() time.Time { + if t != nil && t.transportTestHooks != nil { + return t.transportTestHooks.group.Now() + } + return time.Now() +} + +func (t *Transport) timeSince(when time.Time) time.Duration { + if t != nil && t.transportTestHooks != nil { + return t.now().Sub(when) + } + return time.Since(when) +} + // newTimer creates a new time.Timer, or a synthetic timer in tests. func (t *Transport) newTimer(d time.Duration) timer { if t.transportTestHooks != nil { @@ -343,7 +357,7 @@ type ClientConn struct { t *Transport tconn net.Conn // usually *tls.Conn, except specialized impls tlsState *tls.ConnectionState // nil only for specialized impls - reused uint32 // whether conn is being reused; atomic + atomicReused uint32 // whether conn is being reused; atomic singleUse bool // whether being used for a single http.Request getConnCalled bool // used by clientConnPool @@ -609,7 +623,7 @@ func (t *Transport) RoundTripOpt(req *http.Request, opt RoundTripOpt) (*http.Res t.vlogf("http2: Transport failed to get client conn for %s: %v", addr, err) return nil, err } - reused := !atomic.CompareAndSwapUint32(&cc.reused, 0, 1) + reused := !atomic.CompareAndSwapUint32(&cc.atomicReused, 0, 1) traceGotConn(req, cc, reused) res, err := cc.RoundTrip(req) if err != nil && retry <= 6 { @@ -634,6 +648,22 @@ func (t *Transport) RoundTripOpt(req *http.Request, opt RoundTripOpt) (*http.Res } } } + if err == errClientConnNotEstablished { + // This ClientConn was created recently, + // this is the first request to use it, + // and the connection is closed and not usable. + // + // In this state, cc.idleTimer will remove the conn from the pool + // when it fires. Stop the timer and remove it here so future requests + // won't try to use this connection. + // + // If the timer has already fired and we're racing it, the redundant + // call to MarkDead is harmless. + if cc.idleTimer != nil { + cc.idleTimer.Stop() + } + t.connPool().MarkDead(cc) + } if err != nil { t.vlogf("RoundTrip failure: %v", err) return nil, err @@ -652,9 +682,10 @@ func (t *Transport) CloseIdleConnections() { } var ( - errClientConnClosed = errors.New("http2: client conn is closed") - errClientConnUnusable = errors.New("http2: client conn not usable") - errClientConnGotGoAway = errors.New("http2: Transport received Server's graceful shutdown GOAWAY") + errClientConnClosed = errors.New("http2: client conn is closed") + errClientConnUnusable = errors.New("http2: client conn not usable") + errClientConnNotEstablished = errors.New("http2: client conn could not be established") + errClientConnGotGoAway = errors.New("http2: Transport received Server's graceful shutdown GOAWAY") ) // shouldRetryRequest is called by RoundTrip when a request fails to get @@ -793,6 +824,7 @@ func (t *Transport) newClientConn(c net.Conn, singleUse bool) (*ClientConn, erro pingTimeout: conf.PingTimeout, pings: make(map[[8]byte]chan struct{}), reqHeaderMu: make(chan struct{}, 1), + lastActive: t.now(), } var group synctestGroupInterface if t.transportTestHooks != nil { @@ -1041,6 +1073,16 @@ func (cc *ClientConn) idleStateLocked() (st clientConnIdleState) { !cc.doNotReuse && int64(cc.nextStreamID)+2*int64(cc.pendingRequests) < math.MaxInt32 && !cc.tooIdleLocked() + + // If this connection has never been used for a request and is closed, + // then let it take a request (which will fail). + // + // This avoids a situation where an error early in a connection's lifetime + // goes unreported. + if cc.nextStreamID == 1 && cc.streamsReserved == 0 && cc.closed { + st.canTakeNewRequest = true + } + return } @@ -1062,7 +1104,7 @@ func (cc *ClientConn) tooIdleLocked() bool { // times are compared based on their wall time. We don't want // to reuse a connection that's been sitting idle during // VM/laptop suspend if monotonic time was also frozen. - return cc.idleTimeout != 0 && !cc.lastIdle.IsZero() && time.Since(cc.lastIdle.Round(0)) > cc.idleTimeout + return cc.idleTimeout != 0 && !cc.lastIdle.IsZero() && cc.t.timeSince(cc.lastIdle.Round(0)) > cc.idleTimeout } // onIdleTimeout is called from a time.AfterFunc goroutine. It will @@ -1706,7 +1748,12 @@ func (cs *clientStream) cleanupWriteRequest(err error) { // Must hold cc.mu. func (cc *ClientConn) awaitOpenSlotForStreamLocked(cs *clientStream) error { for { - cc.lastActive = time.Now() + if cc.closed && cc.nextStreamID == 1 && cc.streamsReserved == 0 { + // This is the very first request sent to this connection. + // Return a fatal error which aborts the retry loop. + return errClientConnNotEstablished + } + cc.lastActive = cc.t.now() if cc.closed || !cc.canTakeNewRequestLocked() { return errClientConnUnusable } @@ -2253,10 +2300,10 @@ func (cc *ClientConn) forgetStreamID(id uint32) { if len(cc.streams) != slen-1 { panic("forgetting unknown stream id") } - cc.lastActive = time.Now() + cc.lastActive = cc.t.now() if len(cc.streams) == 0 && cc.idleTimer != nil { cc.idleTimer.Reset(cc.idleTimeout) - cc.lastIdle = time.Now() + cc.lastIdle = cc.t.now() } // Wake up writeRequestBody via clientStream.awaitFlowControl and // wake up RoundTrip if there is a pending request. @@ -2316,7 +2363,6 @@ func isEOFOrNetReadError(err error) bool { func (rl *clientConnReadLoop) cleanup() { cc := rl.cc - cc.t.connPool().MarkDead(cc) defer cc.closeConn() defer close(cc.readerDone) @@ -2340,6 +2386,24 @@ func (rl *clientConnReadLoop) cleanup() { } cc.closed = true + // If the connection has never been used, and has been open for only a short time, + // leave it in the connection pool for a little while. + // + // This avoids a situation where new connections are constantly created, + // added to the pool, fail, and are removed from the pool, without any error + // being surfaced to the user. + const unusedWaitTime = 5 * time.Second + idleTime := cc.t.now().Sub(cc.lastActive) + if atomic.LoadUint32(&cc.atomicReused) == 0 && idleTime < unusedWaitTime { + cc.idleTimer = cc.t.afterFunc(unusedWaitTime-idleTime, func() { + cc.t.connPool().MarkDead(cc) + }) + } else { + cc.mu.Unlock() // avoid any deadlocks in MarkDead + cc.t.connPool().MarkDead(cc) + cc.mu.Lock() + } + for _, cs := range cc.streams { select { case <-cs.peerClosed: @@ -3332,7 +3396,7 @@ func traceGotConn(req *http.Request, cc *ClientConn, reused bool) { cc.mu.Lock() ci.WasIdle = len(cc.streams) == 0 && reused if ci.WasIdle && !cc.lastActive.IsZero() { - ci.IdleTime = time.Since(cc.lastActive) + ci.IdleTime = cc.t.timeSince(cc.lastActive) } cc.mu.Unlock() diff --git a/http2/transport_test.go b/http2/transport_test.go index f6ef295a4..100da8bd1 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -5638,3 +5638,116 @@ func TestTransportConnBecomesUnresponsive(t *testing.T) { rt2.wantStatus(200) rt2.response().Body.Close() } + +// Test that the Transport can use a conn provided to it by a TLSNextProto hook. +func TestTransportTLSNextProtoConnOK(t *testing.T) { + t1 := &http.Transport{} + t2, _ := ConfigureTransports(t1) + tt := newTestTransport(t, t2) + + // Create a new, fake connection and pass it to the Transport via the TLSNextProto hook. + cli, _ := synctestNetPipe(tt.group) + cliTLS := tls.Client(cli, tlsConfigInsecure) + go func() { + tt.group.Join() + t1.TLSNextProto["h2"]("dummy.tld", cliTLS) + }() + tt.sync() + tc := tt.getConn() + tc.greet() + + // Send a request on the Transport. + // It uses the conn we provided. + req := must(http.NewRequest("GET", "https://dummy.tld/", nil)) + rt := tt.roundTrip(req) + tc.wantHeaders(wantHeader{ + streamID: 1, + endStream: true, + header: http.Header{ + ":authority": []string{"dummy.tld"}, + ":method": []string{"GET"}, + ":path": []string{"/"}, + }, + }) + tc.writeHeaders(HeadersFrameParam{ + StreamID: 1, + EndHeaders: true, + EndStream: true, + BlockFragment: tc.makeHeaderBlockFragment( + ":status", "200", + ), + }) + rt.wantStatus(200) + rt.wantBody(nil) +} + +// Test the case where a conn provided via a TLSNextProto hook immediately encounters an error. +func TestTransportTLSNextProtoConnImmediateFailureUsed(t *testing.T) { + t1 := &http.Transport{} + t2, _ := ConfigureTransports(t1) + tt := newTestTransport(t, t2) + + // Create a new, fake connection and pass it to the Transport via the TLSNextProto hook. + cli, _ := synctestNetPipe(tt.group) + cliTLS := tls.Client(cli, tlsConfigInsecure) + go func() { + tt.group.Join() + t1.TLSNextProto["h2"]("dummy.tld", cliTLS) + }() + tt.sync() + tc := tt.getConn() + + // The connection encounters an error before we send a request that uses it. + tc.closeWrite() + + // Send a request on the Transport. + // + // It should fail, because we have no usable connections, but not with ErrNoCachedConn. + req := must(http.NewRequest("GET", "https://dummy.tld/", nil)) + rt := tt.roundTrip(req) + if err := rt.err(); err == nil || errors.Is(err, ErrNoCachedConn) { + t.Fatalf("RoundTrip with broken conn: got %v, want an error other than ErrNoCachedConn", err) + } + + // Send the request again. + // This time it should fail with ErrNoCachedConn, + // because the dead conn has been removed from the pool. + rt = tt.roundTrip(req) + if err := rt.err(); !errors.Is(err, ErrNoCachedConn) { + t.Fatalf("RoundTrip after broken conn is used: got %v, want ErrNoCachedConn", err) + } +} + +// Test the case where a conn provided via a TLSNextProto hook immediately encounters an error, +// but no requests are sent which would use the bad connection. +func TestTransportTLSNextProtoConnImmediateFailureUnused(t *testing.T) { + t1 := &http.Transport{} + t2, _ := ConfigureTransports(t1) + tt := newTestTransport(t, t2) + + // Create a new, fake connection and pass it to the Transport via the TLSNextProto hook. + cli, _ := synctestNetPipe(tt.group) + cliTLS := tls.Client(cli, tlsConfigInsecure) + go func() { + tt.group.Join() + t1.TLSNextProto["h2"]("dummy.tld", cliTLS) + }() + tt.sync() + tc := tt.getConn() + + // The connection encounters an error before we send a request that uses it. + tc.closeWrite() + + // Some time passes. + // The dead connection is removed from the pool. + tc.advance(10 * time.Second) + + // Send a request on the Transport. + // + // It should fail with ErrNoCachedConn, because the pool contains no conns. + req := must(http.NewRequest("GET", "https://dummy.tld/", nil)) + rt := tt.roundTrip(req) + if err := rt.err(); !errors.Is(err, ErrNoCachedConn) { + t.Fatalf("RoundTrip after broken conn expires: got %v, want ErrNoCachedConn", err) + } +} From d7f220d3b8964f859b642e7d7c1a0d0973000939 Mon Sep 17 00:00:00 2001 From: jfgiorgi Date: Fri, 1 Nov 2024 23:54:42 +0000 Subject: [PATCH 10/11] quic: add LocalAddr and RemoteAddr to quic.Conn These are missing for quic.Conn. Fixes golang/go#70138 Change-Id: Ia443ffe0e73e143be5c29233a1ceb7cb16951acd GitHub-Last-Rev: a326378fddecb4136100d345993230a970ba8b22 GitHub-Pull-Request: golang/net#225 Reviewed-on: https://go-review.googlesource.com/c/net/+/623157 Reviewed-by: Damien Neil Reviewed-by: David Chase Auto-Submit: Damien Neil LUCI-TryBot-Result: Go LUCI --- quic/conn.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/quic/conn.go b/quic/conn.go index 38e8fe8f4..fbd8b8434 100644 --- a/quic/conn.go +++ b/quic/conn.go @@ -176,6 +176,16 @@ func (c *Conn) String() string { return fmt.Sprintf("quic.Conn(%v,->%v)", c.side, c.peerAddr) } +// LocalAddr returns the local network address, if known. +func (c *Conn) LocalAddr() netip.AddrPort { + return c.localAddr +} + +// RemoteAddr returns the remote network address, if known. +func (c *Conn) RemoteAddr() netip.AddrPort { + return c.peerAddr +} + // confirmHandshake is called when the handshake is confirmed. // https://www.rfc-editor.org/rfc/rfc9001#section-4.1.2 func (c *Conn) confirmHandshake(now time.Time) { From 334afa0d53434157eb708b09ff35a42db2c4531a Mon Sep 17 00:00:00 2001 From: Gopher Robot Date: Thu, 7 Nov 2024 23:20:51 +0000 Subject: [PATCH 11/11] go.mod: update golang.org/x dependencies Update golang.org/x dependencies to their latest tagged versions. Change-Id: Ic3d15c610f766d40730157ea878be90dd9e9c084 Reviewed-on: https://go-review.googlesource.com/c/net/+/626378 Reviewed-by: Dmitri Shuralyov Reviewed-by: David Chase Auto-Submit: Gopher Robot LUCI-TryBot-Result: Go LUCI --- go.mod | 8 ++++---- go.sum | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/go.mod b/go.mod index 77935d3f2..2721bac68 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,8 @@ module golang.org/x/net go 1.18 require ( - golang.org/x/crypto v0.28.0 - golang.org/x/sys v0.26.0 - golang.org/x/term v0.25.0 - golang.org/x/text v0.19.0 + golang.org/x/crypto v0.29.0 + golang.org/x/sys v0.27.0 + golang.org/x/term v0.26.0 + golang.org/x/text v0.20.0 ) diff --git a/go.sum b/go.sum index 1b8c61d60..6868ecce4 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,8 @@ -golang.org/x/crypto v0.28.0 h1:GBDwsMXVQi34v5CCYUm2jkJvu4cbtru2U4TN2PSyQnw= -golang.org/x/crypto v0.28.0/go.mod h1:rmgy+3RHxRZMyY0jjAJShp2zgEdOqj2AO7U0pYmeQ7U= -golang.org/x/sys v0.26.0 h1:KHjCJyddX0LoSTb3J+vWpupP9p0oznkqVk/IfjymZbo= -golang.org/x/sys v0.26.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/term v0.25.0 h1:WtHI/ltw4NvSUig5KARz9h521QvRC8RmF/cuYqifU24= -golang.org/x/term v0.25.0/go.mod h1:RPyXicDX+6vLxogjjRxjgD2TKtmAO6NZBsBRfrOLu7M= -golang.org/x/text v0.19.0 h1:kTxAhCbGbxhK0IwgSKiMO5awPoDQ0RpfiVYBfK860YM= -golang.org/x/text v0.19.0/go.mod h1:BuEKDfySbSR4drPmRPG/7iBdf8hvFMuRexcpahXilzY= +golang.org/x/crypto v0.29.0 h1:L5SG1JTTXupVV3n6sUqMTeWbjAyfPwoda2DLX8J8FrQ= +golang.org/x/crypto v0.29.0/go.mod h1:+F4F4N5hv6v38hfeYwTdx20oUvLLc+QfrE9Ax9HtgRg= +golang.org/x/sys v0.27.0 h1:wBqf8DvsY9Y/2P8gAfPDEYNuS30J4lPHJxXSb/nJZ+s= +golang.org/x/sys v0.27.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/term v0.26.0 h1:WEQa6V3Gja/BhNxg540hBip/kkaYtRg3cxg4oXSw4AU= +golang.org/x/term v0.26.0/go.mod h1:Si5m1o57C5nBNQo5z1iq+XDijt21BDBDp2bK0QI8e3E= +golang.org/x/text v0.20.0 h1:gK/Kv2otX8gz+wn7Rmb3vT96ZwuoxnQlY+HlJVj7Qug= +golang.org/x/text v0.20.0/go.mod h1:D4IsuqiFMhST5bX19pQ9ikHC2GsaKyk/oF+pn3ducp4=