http2: fix Server race
With Tom Bergan.
Updates golang/go#20704
Change-Id: Ib71202801f8c72af2f22865899c93df1f3753fdd
Reviewed-on: https://go-review.googlesource.com/46008
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
diff --git a/http2/server.go b/http2/server.go
index 7367b31..eae143d 100644
--- a/http2/server.go
+++ b/http2/server.go
@@ -2252,6 +2252,7 @@
wroteHeader bool // WriteHeader called (explicitly or implicitly). Not necessarily sent to user yet.
sentHeader bool // have we sent the header frame?
handlerDone bool // handler has finished
+ dirty bool // a Write failed; don't reuse this responseWriterState
sentContentLen int64 // non-zero if handler set a Content-Length header
wroteBytes int64
@@ -2333,6 +2334,7 @@
date: date,
})
if err != nil {
+ rws.dirty = true
return 0, err
}
if endStream {
@@ -2354,6 +2356,7 @@
if len(p) > 0 || endStream {
// only send a 0 byte DATA frame if we're ending the stream.
if err := rws.conn.writeDataFromHandler(rws.stream, p, endStream); err != nil {
+ rws.dirty = true
return 0, err
}
}
@@ -2365,6 +2368,9 @@
trailers: rws.trailers,
endStream: true,
})
+ if err != nil {
+ rws.dirty = true
+ }
return len(p), err
}
return len(p), nil
@@ -2504,7 +2510,7 @@
//
// * Handler calls w.Write or w.WriteString ->
// * -> rws.bw (*bufio.Writer) ->
-// * (Handler migth call Flush)
+// * (Handler might call Flush)
// * -> chunkWriter{rws}
// * -> responseWriterState.writeChunk(p []byte)
// * -> responseWriterState.writeChunk (most of the magic; see comment there)
@@ -2543,10 +2549,19 @@
func (w *responseWriter) handlerDone() {
rws := w.rws
+ dirty := rws.dirty
rws.handlerDone = true
w.Flush()
w.rws = nil
- responseWriterStatePool.Put(rws)
+ if !dirty {
+ // Only recycle the pool if all prior Write calls to
+ // the serverConn goroutine completed successfully. If
+ // they returned earlier due to resets from the peer
+ // there might still be write goroutines outstanding
+ // from the serverConn referencing the rws memory. See
+ // issue 20704.
+ responseWriterStatePool.Put(rws)
+ }
}
// Push errors.
diff --git a/http2/server_test.go b/http2/server_test.go
index 638d2a4..437d1c3 100644
--- a/http2/server_test.go
+++ b/http2/server_test.go
@@ -3685,3 +3685,37 @@
<-done
}
}
+
+func TestIssue20704Race(t *testing.T) {
+ if testing.Short() && os.Getenv("GO_BUILDER_NAME") == "" {
+ t.Skip("skipping in short mode")
+ }
+ const (
+ itemSize = 1 << 10
+ itemCount = 100
+ )
+
+ st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
+ for i := 0; i < itemCount; i++ {
+ _, err := w.Write(make([]byte, itemSize))
+ if err != nil {
+ return
+ }
+ }
+ }, optOnlyServer)
+ defer st.Close()
+
+ tr := &Transport{TLSClientConfig: tlsConfigInsecure}
+ defer tr.CloseIdleConnections()
+ cl := &http.Client{Transport: tr}
+
+ for i := 0; i < 1000; i++ {
+ resp, err := cl.Get(st.ts.URL)
+ if err != nil {
+ t.Fatal(err)
+ }
+ // Force a RST stream to the server by closing without
+ // reading the body:
+ resp.Body.Close()
+ }
+}