[net] http2: close client connections after receiving GOAWAY
800 views
Skip to first unread message
Damien Neil (Gerrit)
Sep 20, 2022, 4:31:07 PM9/20/22
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Cherry Mui, Gopher Robot, Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Damien Neil submitted this change.
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: http2/transport_test.go
Insertions: 29, Deletions: 23.
@@ -5930,16 +5930,27 @@
testTransportClosesConnAfterGoAway(t, 1)
}
+// testTransportClosesConnAfterGoAway verifies that the transport
+// closes a connection after reading a GOAWAY from it.
+//
+// lastStream is the last stream ID in the GOAWAY frame.
+// When 0, the transport (unsuccessfully) retries the request (stream 1);
+// when 1, the transport reads the response after receiving the GOAWAY.
func testTransportClosesConnAfterGoAway(t *testing.T, lastStream uint32) {
ct := newClientTester(t)
- done := make(chan struct{})
-
+ var wg sync.WaitGroup
+ wg.Add(1)
ct.client = func() error {
- defer close(done)
+ defer wg.Done()
req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
- ct.tr.RoundTrip(req)
- var err error
+ res, err := ct.tr.RoundTrip(req)
+ if err == nil {
+ res.Body.Close()
+ }
+ if gotErr, wantErr := err != nil, lastStream == 0; gotErr != wantErr {
+ t.Errorf("RoundTrip got error %v (want error: %v)", err, wantErr)
+ }
if err = ct.cc.Close(); err == nil {
err = fmt.Errorf("expected error on Close")
} else if strings.Contains(err.Error(), "use of closed network") {
@@ -5949,33 +5960,28 @@
}
ct.server = func() error {
+ defer wg.Wait()
ct.greet()
hf, err := ct.firstHeaders()
if err != nil {
return fmt.Errorf("server failed reading HEADERS: %v", err)
}
-
if err := ct.fr.WriteGoAway(lastStream, ErrCodeNo, nil); err != nil {
return fmt.Errorf("server failed writing GOAWAY: %v", err)
}
-
- // Send a valid response to first request.
- var buf bytes.Buffer
- enc := hpack.NewEncoder(&buf)
- enc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
- ct.fr.WriteHeaders(HeadersFrameParam{
- StreamID: hf.StreamID,
- EndHeaders: true,
- EndStream: true,
- BlockFragment: buf.Bytes(),
- })
-
- select {
- case <-done:
- return nil
- case <-time.After(5 * time.Second):
- return fmt.Errorf("timed out")
+ if lastStream > 0 {
+ // Send a valid response to first request.
+ var buf bytes.Buffer
+ enc := hpack.NewEncoder(&buf)
+ enc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
+ ct.fr.WriteHeaders(HeadersFrameParam{
+ StreamID: hf.StreamID,
+ EndHeaders: true,
+ EndStream: true,
+ BlockFragment: buf.Bytes(),
+ })
}
+ return nil
}
ct.run()
```
Approvals:
Gopher Robot: TryBots succeeded
Brad Fitzpatrick: Looks good to me, approved
Cherry Mui: Looks good to me, but someone else must approve
Damien Neil: Run TryBots
http2: close client connections after receiving GOAWAY
Once a connection has received a GOAWAY from the server,
close it after the last outstanding request on the connection
completes.
We're lax about when we call ClientConn.closeConn, frequently
closing the underlying net.Conn multiple times. Stop propagating
errors on closing the net.Conn up through ClientConn.Close and
ClientConn.Shutdown, since these errors are likely to be caused
by double-closing the connection rather than a real fault.
Fixes golang/go#39752.
Change-Id: I06d59e6daa6331c3091e1d49cdbeac313f17e6bd
Reviewed-on: https://go-review.googlesource.com/c/net/+/429060
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Brad Fitzpatrick <brad...@golang.org>
Reviewed-by: Cherry Mui <cher...@google.com>
Run-TryBot: Damien Neil <dn...@google.com>
---
M http2/transport.go
M http2/transport_test.go
2 files changed, 103 insertions(+), 11 deletions(-)
diff --git a/http2/transport.go b/http2/transport.go
index 90fdc28..e9aba43 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -258,7 +258,8 @@
// HTTP/2 server.
type ClientConn struct {
t *Transport
- tconn net.Conn // usually *tls.Conn, except specialized impls
+ tconn net.Conn // usually *tls.Conn, except specialized impls
+ tconnClosed bool
tlsState *tls.ConnectionState // nil only for specialized impls
reused uint32 // whether conn is being reused; atomic
singleUse bool // whether being used for a single http.Request
@@ -921,10 +922,10 @@
cc.closeIfIdle()
}
-func (cc *ClientConn) closeConn() error {
+func (cc *ClientConn) closeConn() {
t := time.AfterFunc(250*time.Millisecond, cc.forceCloseConn)
defer t.Stop()
- return cc.tconn.Close()
+ cc.tconn.Close()
}
// A tls.Conn.Close can hang for a long time if the peer is unresponsive.
@@ -990,7 +991,8 @@
shutdownEnterWaitStateHook()
select {
case <-done:
- return cc.closeConn()
+ cc.closeConn()
+ return nil
case <-ctx.Done():
cc.mu.Lock()
// Free the goroutine above
@@ -1027,7 +1029,7 @@
// closes the client connection immediately. In-flight requests are interrupted.
// err is sent to streams.
-func (cc *ClientConn) closeForError(err error) error {
+func (cc *ClientConn) closeForError(err error) {
cc.mu.Lock()
cc.closed = true
for _, cs := range cc.streams {
@@ -1035,7 +1037,7 @@
}
cc.cond.Broadcast()
cc.mu.Unlock()
- return cc.closeConn()
+ cc.closeConn()
}
// Close closes the client connection immediately.
@@ -1043,16 +1045,17 @@
// In-flight requests are interrupted. For a graceful shutdown, use Shutdown instead.
func (cc *ClientConn) Close() error {
err := errors.New("http2: client connection force closed via ClientConn.Close")
- return cc.closeForError(err)
+ cc.closeForError(err)
+ return nil
}
// closes the client connection immediately. In-flight requests are interrupted.
-func (cc *ClientConn) closeForLostPing() error {
+func (cc *ClientConn) closeForLostPing() {
err := errors.New("http2: client connection lost")
if f := cc.t.CountError; f != nil {
f("conn_close_lost_ping")
}
- return cc.closeForError(err)
+ cc.closeForError(err)
}
// errRequestCanceled is a copy of net/http's errRequestCanceled because it's not
@@ -2005,7 +2008,7 @@
// wake up RoundTrip if there is a pending request.
cc.cond.Broadcast()
- closeOnIdle := cc.singleUse || cc.doNotReuse || cc.t.disableKeepAlives()
+ closeOnIdle := cc.singleUse || cc.doNotReuse || cc.t.disableKeepAlives() || cc.goAway != nil
if closeOnIdle && cc.streamsReserved == 0 && len(cc.streams) == 0 {
if VerboseLogs {
cc.vlogf("http2: Transport closing idle conn %p (forSingleUse=%v, maxStream=%v)", cc, cc.singleUse, cc.nextStreamID-2)
@@ -2674,7 +2677,6 @@
if fn := cc.t.CountError; fn != nil {
fn("recv_goaway_" + f.ErrCode.stringToken())
}
-
}
cc.setGoAway(f)
return nil
diff --git a/http2/transport_test.go b/http2/transport_test.go
index bf6683b..7d69c7d 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -5922,3 +5922,67 @@
}
resp.Body.Close()
}
+
+func TestTransportClosesConnAfterGoAwayNoStreams(t *testing.T) {
+ testTransportClosesConnAfterGoAway(t, 0)
+}
+func TestTransportClosesConnAfterGoAwayLastStream(t *testing.T) {
+ testTransportClosesConnAfterGoAway(t, 1)
+}
+
+// testTransportClosesConnAfterGoAway verifies that the transport
+// closes a connection after reading a GOAWAY from it.
+//
+// lastStream is the last stream ID in the GOAWAY frame.
+// When 0, the transport (unsuccessfully) retries the request (stream 1);
+// when 1, the transport reads the response after receiving the GOAWAY.
+func testTransportClosesConnAfterGoAway(t *testing.T, lastStream uint32) {
+ ct := newClientTester(t)
+
+ var wg sync.WaitGroup
+ wg.Add(1)
+ ct.client = func() error {
+ defer wg.Done()
+ req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
+ res, err := ct.tr.RoundTrip(req)
+ if err == nil {
+ res.Body.Close()
+ }
+ if gotErr, wantErr := err != nil, lastStream == 0; gotErr != wantErr {
+ t.Errorf("RoundTrip got error %v (want error: %v)", err, wantErr)
+ }
+ if err = ct.cc.Close(); err == nil {
+ err = fmt.Errorf("expected error on Close")
+ } else if strings.Contains(err.Error(), "use of closed network") {
+ err = nil
+ }
+ return err
+ }
+
+ ct.server = func() error {
+ defer wg.Wait()
+ ct.greet()
+ hf, err := ct.firstHeaders()
+ if err != nil {
+ return fmt.Errorf("server failed reading HEADERS: %v", err)
+ }
+ if err := ct.fr.WriteGoAway(lastStream, ErrCodeNo, nil); err != nil {
+ return fmt.Errorf("server failed writing GOAWAY: %v", err)
+ }
+ if lastStream > 0 {
+ // Send a valid response to first request.
+ var buf bytes.Buffer
+ enc := hpack.NewEncoder(&buf)
+ enc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
+ ct.fr.WriteHeaders(HeadersFrameParam{
+ StreamID: hf.StreamID,
+ EndHeaders: true,
+ EndStream: true,
+ BlockFragment: buf.Bytes(),
+ })
+ }
+ return nil
+ }
+
+ ct.run()
+}
To view, visit change 429060. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages