]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: update bundled http2, fix Transport memory leak
authorBrad Fitzpatrick <bradfitz@golang.org>
Mon, 1 Feb 2016 22:16:42 +0000 (22:16 +0000)
committerBrad Fitzpatrick <bradfitz@golang.org>
Mon, 1 Feb 2016 23:05:46 +0000 (23:05 +0000)
Updates x/net/http2 to git rev 644ffc for three CLs since the last update:

http2: don't add *Response to activeRes in Transport on Headers.END_STREAM
https://golang.org/cl/19134

http2: add mechanism to send undeclared Trailers mid handler
https://golang.org/cl/19131

http2: remove unused variable
https://golang.org/cl/18936

The first in the list above is the main fix that's necessary. The
other are two are in the git history but along for the cmd/bundle
ride. The middle CL is well-tested, small (mostly comments),
non-tricky, and almost never seen (since nobody really uses Trailers).
The final CL is just deleting an unused global variable.

Fixes #14084 again (with more tests)

Change-Id: Iac51350acee9c51d32bf7779d57e9d5a5482b928
Reviewed-on: https://go-review.googlesource.com/19135
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
src/net/http/clientserver_test.go
src/net/http/h2_bundle.go

index 9b581e73117cc4caf06e6777a722e2dcb4525ae6..fbaa805712b157a50d35c7c9ae99694ae97e8949 100644 (file)
@@ -1001,13 +1001,17 @@ func TestTransportDiscardsUnneededConns(t *testing.T) {
 }
 
 // tests that Transport doesn't retain a pointer to the provided request.
-func TestTransportGCRequest_h1(t *testing.T) { testTransportGCRequest(t, h1Mode) }
-func TestTransportGCRequest_h2(t *testing.T) { testTransportGCRequest(t, h2Mode) }
-func testTransportGCRequest(t *testing.T, h2 bool) {
+func TestTransportGCRequest_Body_h1(t *testing.T)   { testTransportGCRequest(t, h1Mode, true) }
+func TestTransportGCRequest_Body_h2(t *testing.T)   { testTransportGCRequest(t, h2Mode, true) }
+func TestTransportGCRequest_NoBody_h1(t *testing.T) { testTransportGCRequest(t, h1Mode, false) }
+func TestTransportGCRequest_NoBody_h2(t *testing.T) { testTransportGCRequest(t, h2Mode, false) }
+func testTransportGCRequest(t *testing.T, h2, body bool) {
        defer afterTest(t)
        cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {
                ioutil.ReadAll(r.Body)
-               io.WriteString(w, "Hello.")
+               if body {
+                       io.WriteString(w, "Hello.")
+               }
        }))
        defer cst.close()
 
index e7236299e2253d824739005e97a8b52043a5107e..11f33cf3b18d9206d374d4c9c71e13171d8376be 100644 (file)
@@ -2851,8 +2851,6 @@ func (sc *http2serverConn) logf(format string, args ...interface{}) {
        }
 }
 
-var http2uintptrType = reflect.TypeOf(uintptr(0))
-
 // errno returns v's underlying uintptr, else 0.
 //
 // TODO: remove this helper function once http2 can use build
@@ -4220,7 +4218,9 @@ func (rws *http2responseWriterState) declareTrailer(k string) {
 
                return
        }
-       rws.trailers = append(rws.trailers, k)
+       if !http2strSliceContains(rws.trailers, k) {
+               rws.trailers = append(rws.trailers, k)
+       }
 }
 
 // writeChunk writes chunks from the bufio.Writer. But because
@@ -4288,6 +4288,10 @@ func (rws *http2responseWriterState) writeChunk(p []byte) (n int, err error) {
                return 0, nil
        }
 
+       if rws.handlerDone {
+               rws.promoteUndeclaredTrailers()
+       }
+
        endStream := rws.handlerDone && !rws.hasTrailers()
        if len(p) > 0 || endStream {
 
@@ -4308,6 +4312,53 @@ func (rws *http2responseWriterState) writeChunk(p []byte) (n int, err error) {
        return len(p), nil
 }
 
+// TrailerPrefix is a magic prefix for ResponseWriter.Header map keys
+// that, if present, signals that the map entry is actually for
+// the response trailers, and not the response headers. The prefix
+// is stripped after the ServeHTTP call finishes and the values are
+// sent in the trailers.
+//
+// This mechanism is intended only for trailers that are not known
+// prior to the headers being written. If the set of trailers is fixed
+// or known before the header is written, the normal Go trailers mechanism
+// is preferred:
+//    https://golang.org/pkg/net/http/#ResponseWriter
+//    https://golang.org/pkg/net/http/#example_ResponseWriter_trailers
+const http2TrailerPrefix = "Trailer:"
+
+// promoteUndeclaredTrailers permits http.Handlers to set trailers
+// after the header has already been flushed. Because the Go
+// ResponseWriter interface has no way to set Trailers (only the
+// Header), and because we didn't want to expand the ResponseWriter
+// interface, and because nobody used trailers, and because RFC 2616
+// says you SHOULD (but not must) predeclare any trailers in the
+// header, the official ResponseWriter rules said trailers in Go must
+// be predeclared, and then we reuse the same ResponseWriter.Header()
+// map to mean both Headers and Trailers.  When it's time to write the
+// Trailers, we pick out the fields of Headers that were declared as
+// trailers. That worked for a while, until we found the first major
+// user of Trailers in the wild: gRPC (using them only over http2),
+// and gRPC libraries permit setting trailers mid-stream without
+// predeclarnig them. So: change of plans. We still permit the old
+// way, but we also permit this hack: if a Header() key begins with
+// "Trailer:", the suffix of that key is a Trailer. Because ':' is an
+// invalid token byte anyway, there is no ambiguity. (And it's already
+// filtered out) It's mildly hacky, but not terrible.
+//
+// This method runs after the Handler is done and promotes any Header
+// fields to be trailers.
+func (rws *http2responseWriterState) promoteUndeclaredTrailers() {
+       for k, vv := range rws.handlerHeader {
+               if !strings.HasPrefix(k, http2TrailerPrefix) {
+                       continue
+               }
+               trailerKey := strings.TrimPrefix(k, http2TrailerPrefix)
+               rws.declareTrailer(trailerKey)
+               rws.handlerHeader[CanonicalHeaderKey(trailerKey)] = vv
+       }
+       sort.Strings(rws.trailers)
+}
+
 func (w *http2responseWriter) Flush() {
        rws := w.rws
        if rws == nil {
@@ -5611,10 +5662,10 @@ func (rl *http2clientConnReadLoop) processHeaderBlockFragment(frag []byte, strea
                        res.ContentLength = -1
                        res.Body = &http2gzipReader{body: res.Body}
                }
+               rl.activeRes[cs.ID] = cs
        }
 
        cs.resTrailer = &res.Trailer
-       rl.activeRes[cs.ID] = cs
        cs.resc <- http2resAndError{res: res}
        rl.nextRes = nil
        return nil
@@ -6258,8 +6309,16 @@ func http2encodeHeaders(enc *hpack.Encoder, h Header, keys []string) {
        for _, k := range keys {
                vv := h[k]
                k = http2lowerHeader(k)
+               if !http2validHeaderFieldName(k) {
+
+                       continue
+               }
                isTE := k == "transfer-encoding"
                for _, v := range vv {
+                       if !http2validHeaderFieldValue(v) {
+
+                               continue
+                       }
 
                        if isTE && v != "trailers" {
                                continue