git » go-net » commit 3797cd8

http2: make Transport prefer HTTP response header recv before body write error

author Brad Fitzpatrick
2016-07-16 23:29:31 UTC
committer Brad Fitzpatrick
2016-07-17 22:45:10 UTC
parent be28236b9034a52ec04f37f16f355b4bde7807e2
http2: make Transport prefer HTTP response header recv before body write error

Fixes flakes like:
https://build.golang.org/log/56e0561f9f6a740b75538692a1c3288f08949dd0

I reproduced the flake above with a time.Sleep(50 * time.Millisecond)
before ClientConn.RoundTrip's final for{select{...}} loop (to give
multiple channels time to become readable on the first select), and
then ran (on linux/amd64) with:

go test -v -run=TestTransportReqBodyAfterResponse_403 -count=50 -race

Before, many failed. With this CL it never fails. (crosses fingers)

Fixes golang/go#16102 (again)

Change-Id: I93fde46846cd09445e53c140e9668382965d95c5
Reviewed-on: https://go-review.googlesource.com/24984
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
http2/transport.go +31 -21 
diff --git a/http2/transport.go b/http2/transport.go
index de3f5fe..2b1f3a4 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -747,30 +747,34 @@ func (cc *ClientConn) RoundTrip(req *http.Request) (*http.Response, error) {
 	bodyWritten := false
 	ctx := reqContext(req)
 
+	handleReadLoopResponse := func(re resAndError) (*http.Response, error) {
+		res := re.res
+		if re.err != nil || res.StatusCode > 299 {
+			// On error or status code 3xx, 4xx, 5xx, etc abort any
+			// ongoing write, assuming that the server doesn't care
+			// about our request body. If the server replied with 1xx or
+			// 2xx, however, then assume the server DOES potentially
+			// want our body (e.g. full-duplex streaming:
+			// golang.org/issue/13444). If it turns out the server
+			// doesn't, they'll RST_STREAM us soon enough.  This is a
+			// heuristic to avoid adding knobs to Transport.  Hopefully
+			// we can keep it.
+			bodyWriter.cancel()
+			cs.abortRequestBodyWrite(errStopReqBodyWrite)
+		}
+		if re.err != nil {
+			cc.forgetStreamID(cs.ID)
+			return nil, re.err
+		}
+		res.Request = req
+		res.TLS = cc.tlsState
+		return res, nil
+	}
+
 	for {
 		select {
 		case re := <-readLoopResCh:
-			res := re.res
-			if re.err != nil || res.StatusCode > 299 {
-				// On error or status code 3xx, 4xx, 5xx, etc abort any
-				// ongoing write, assuming that the server doesn't care
-				// about our request body. If the server replied with 1xx or
-				// 2xx, however, then assume the server DOES potentially
-				// want our body (e.g. full-duplex streaming:
-				// golang.org/issue/13444). If it turns out the server
-				// doesn't, they'll RST_STREAM us soon enough.  This is a
-				// heuristic to avoid adding knobs to Transport.  Hopefully
-				// we can keep it.
-				bodyWriter.cancel()
-				cs.abortRequestBodyWrite(errStopReqBodyWrite)
-			}
-			if re.err != nil {
-				cc.forgetStreamID(cs.ID)
-				return nil, re.err
-			}
-			res.Request = req
-			res.TLS = cc.tlsState
-			return res, nil
+			return handleReadLoopResponse(re)
 		case <-respHeaderTimer:
 			cc.forgetStreamID(cs.ID)
 			if !hasBody || bodyWritten {
@@ -804,6 +808,12 @@ func (cc *ClientConn) RoundTrip(req *http.Request) (*http.Response, error) {
 			// forgetStreamID.
 			return nil, cs.resetErr
 		case err := <-bodyWriter.resc:
+			// Prefer the read loop's response, if available. Issue 16102.
+			select {
+			case re := <-readLoopResCh:
+				return handleReadLoopResponse(re)
+			default:
+			}
 			if err != nil {
 				return nil, err
 			}