From: Brad Fitzpatrick <bradfitz@golang.org>
Date: Wed, 23 May 2018 22:30:41 +0000 (+0000)
Subject: net/http: conservatively flush Transport request headers by default
X-Git-Tag: go1.11beta1~317
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=8e5cb0da1b7c6d7c18a60ff0c688538dbe27f8f3;p=gostls13.git

net/http: conservatively flush Transport request headers by default

This changes the http.Transport to flush the bufio.Writer between
writing the request headers and the body.

That wasn't done in the past to minimize the number of TCP packets on
the wire, but that's just an optimization, and it causes problems when
servers are waiting for the headers and the client is blocked on
something before reading the body.

Instead, only do the don't-flush optimization if we know we're not
going to block, whitelisting a set of common in-memory Request.Body
types. (the same set of types special-cased by http.NewRequest)

Fixes #22088

Change-Id: I7717750aa6df32dd3eb92d181b45bc7af24b1144
Reviewed-on: https://go-review.googlesource.com/114316
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
---

diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go
index 632c58249a..c7171a0109 100644
--- a/src/net/http/transfer.go
+++ b/src/net/http/transfer.go
@@ -13,6 +13,7 @@ import (
 	"io/ioutil"
 	"net/http/internal"
 	"net/textproto"
+	"reflect"
 	"sort"
 	"strconv"
 	"strings"
@@ -105,6 +106,17 @@ func newTransferWriter(r interface{}) (t *transferWriter, err error) {
 		if t.ContentLength < 0 && len(t.TransferEncoding) == 0 && t.shouldSendChunkedRequestBody() {
 			t.TransferEncoding = []string{"chunked"}
 		}
+		// If there's a body, conservatively flush the headers
+		// to any bufio.Writer we're writing to, just in case
+		// the server needs the headers early, before we copy
+		// the body and possibly block. We make an exception
+		// for the common standard library in-memory types,
+		// though, to avoid unnecessary TCP packets on the
+		// wire. (Issue 22088.)
+		if t.ContentLength != 0 && !isKnownInMemoryReader(t.Body) {
+			t.FlushHeaders = true
+		}
+
 		atLeastHTTP11 = true // Transport requests are always 1.1 or 2.0
 	case *Response:
 		t.IsResponse = true
@@ -1009,3 +1021,19 @@ func (fr finishAsyncByteRead) Read(p []byte) (n int, err error) {
 	}
 	return
 }
+
+var nopCloserType = reflect.TypeOf(ioutil.NopCloser(nil))
+
+// isKnownInMemoryReader reports whether r is a type known to not
+// block on Read. Its caller uses this as an optional optimization to
+// send fewer TCP packets.
+func isKnownInMemoryReader(r io.Reader) bool {
+	switch r.(type) {
+	case *bytes.Reader, *bytes.Buffer, *strings.Reader:
+		return true
+	}
+	if reflect.TypeOf(r) == nopCloserType {
+		return isKnownInMemoryReader(reflect.ValueOf(r).Field(0).Interface().(io.Reader))
+	}
+	return false
+}
diff --git a/src/net/http/transfer_test.go b/src/net/http/transfer_test.go
index 48cd540b9f..9a201aab2d 100644
--- a/src/net/http/transfer_test.go
+++ b/src/net/http/transfer_test.go
@@ -6,7 +6,9 @@ package http
 
 import (
 	"bufio"
+	"bytes"
 	"io"
+	"io/ioutil"
 	"strings"
 	"testing"
 )
@@ -62,3 +64,29 @@ func TestFinalChunkedBodyReadEOF(t *testing.T) {
 		t.Errorf("buf = %q; want %q", buf, want)
 	}
 }
+
+func TestDetectInMemoryReaders(t *testing.T) {
+	pr, _ := io.Pipe()
+	tests := []struct {
+		r    io.Reader
+		want bool
+	}{
+		{pr, false},
+
+		{bytes.NewReader(nil), true},
+		{bytes.NewBuffer(nil), true},
+		{strings.NewReader(""), true},
+
+		{ioutil.NopCloser(pr), false},
+
+		{ioutil.NopCloser(bytes.NewReader(nil)), true},
+		{ioutil.NopCloser(bytes.NewBuffer(nil)), true},
+		{ioutil.NopCloser(strings.NewReader("")), true},
+	}
+	for i, tt := range tests {
+		got := isKnownInMemoryReader(tt.r)
+		if got != tt.want {
+			t.Logf("%d: got = %v; want %v", i, got, tt.want)
+		}
+	}
+}
diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 693d3f5b81..5e35812c7b 100644
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -3264,8 +3264,8 @@ func TestTransportFlushesBodyChunks(t *testing.T) {
 	defer res.Body.Close()
 
 	want := []string{
-		"POST / HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: x\r\nTransfer-Encoding: chunked\r\nAccept-Encoding: gzip\r\n\r\n" +
-			"5\r\nnum0\n\r\n",
+		"POST / HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: x\r\nTransfer-Encoding: chunked\r\nAccept-Encoding: gzip\r\n\r\n",
+		"5\r\nnum0\n\r\n",
 		"5\r\nnum1\n\r\n",
 		"5\r\nnum2\n\r\n",
 		"0\r\n\r\n",
@@ -3275,6 +3275,40 @@ func TestTransportFlushesBodyChunks(t *testing.T) {
 	}
 }
 
+// Issue 22088: flush Transport request headers if we're not sure the body won't block on read.
+func TestTransportFlushesRequestHeader(t *testing.T) {
+	defer afterTest(t)
+	gotReq := make(chan struct{})
+	cst := newClientServerTest(t, h1Mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+		close(gotReq)
+	}))
+	defer cst.close()
+
+	pr, pw := io.Pipe()
+	req, err := NewRequest("POST", cst.ts.URL, pr)
+	if err != nil {
+		t.Fatal(err)
+	}
+	gotRes := make(chan struct{})
+	go func() {
+		defer close(gotRes)
+		res, err := cst.tr.RoundTrip(req)
+		if err != nil {
+			t.Error(err)
+			return
+		}
+		res.Body.Close()
+	}()
+
+	select {
+	case <-gotReq:
+		pw.Close()
+	case <-time.After(5 * time.Second):
+		t.Fatal("timeout waiting for handler to get request")
+	}
+	<-gotRes
+}
+
 // Issue 11745.
 func TestTransportPrefersResponseOverWriteError(t *testing.T) {
 	if testing.Short() {