http2: close Transport connection on write errors
When a new connection is created, and a write error occurs during the
initial exchange, the connection must be closed. There is no guarantee
that the caller will close the connection.
When a connection with an existing write error is used or being used, it
will stay in use until its read loop completes. Requests will continue
to use this connection and fail when writing its header. These
connections should be closed to force the cleanup in its readLoop.
Fixes golang/go#39337
Change-Id: I45e1293309e40629531f4cbb69387864f4f71bc2
Reviewed-on: https://go-review.googlesource.com/c/net/+/240337
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Trust: Damien Neil <dneil@google.com>
diff --git a/http2/transport.go b/http2/transport.go
index 4ec3266..4182f52 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -689,6 +689,7 @@
cc.inflow.add(transportDefaultConnFlow + initialWindowSize)
cc.bw.Flush()
if cc.werr != nil {
+ cc.Close()
return nil, cc.werr
}
@@ -1080,6 +1081,15 @@
bodyWriter := cc.t.getBodyWriterState(cs, body)
cs.on100 = bodyWriter.on100
+ defer func() {
+ cc.wmu.Lock()
+ werr := cc.werr
+ cc.wmu.Unlock()
+ if werr != nil {
+ cc.Close()
+ }
+ }()
+
cc.wmu.Lock()
endStream := !hasBody && !hasTrailers
werr := cc.writeHeaders(cs.ID, endStream, int(cc.maxFrameSize), hdrs)
diff --git a/http2/transport_test.go b/http2/transport_test.go
index 2fdb117..2255bc4 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -4795,3 +4795,65 @@
}
}
}
+
+type fakeConnErr struct {
+ net.Conn
+ writeErr error
+ closed bool
+}
+
+func (fce *fakeConnErr) Write(b []byte) (n int, err error) {
+ return 0, fce.writeErr
+}
+
+func (fce *fakeConnErr) Close() error {
+ fce.closed = true
+ return nil
+}
+
+// issue 39337: close the connection on a failed write
+func TestTransportNewClientConnCloseOnWriteError(t *testing.T) {
+ tr := &Transport{}
+ writeErr := errors.New("write error")
+ fakeConn := &fakeConnErr{writeErr: writeErr}
+ _, err := tr.NewClientConn(fakeConn)
+ if err != writeErr {
+ t.Fatalf("expected %v, got %v", writeErr, err)
+ }
+ if !fakeConn.closed {
+ t.Error("expected closed conn")
+ }
+}
+
+func TestTransportRoundtripCloseOnWriteError(t *testing.T) {
+ req, err := http.NewRequest("GET", "https://dummy.tld/", nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {}, optOnlyServer)
+ defer st.Close()
+
+ tr := &Transport{TLSClientConfig: tlsConfigInsecure}
+ defer tr.CloseIdleConnections()
+ cc, err := tr.dialClientConn(st.ts.Listener.Addr().String(), false)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ writeErr := errors.New("write error")
+ cc.wmu.Lock()
+ cc.werr = writeErr
+ cc.wmu.Unlock()
+
+ _, err = cc.RoundTrip(req)
+ if err != writeErr {
+ t.Fatalf("expected %v, got %v", writeErr, err)
+ }
+
+ cc.mu.Lock()
+ closed := cc.closed
+ cc.mu.Unlock()
+ if !closed {
+ t.Fatal("expected closed")
+ }
+}
