http2: revert Transport's strict interpretation of MAX_CONCURRENT_STR…

…EAMS

And add the http2.Transport.StrictMaxConcurrentStreams bool knob to
behavior being reverted.

In CL 53250 for golang/go#13774 (for Go 1.10) we changed the HTTP/2
Transport's policy such that a server's advertisement of a
MAX_CONCURRENT_STREAMS value meant that it was a maximum for the
entire process, instead of just a single connection.

We thought that was a reasonable interpretation of the spec and
provided nice safety against slamming a server from a bunch of
goroutines doing concurrent requests, but it's been largely
unpopular (see golang/go#27044). It's also different behavior from
HTTP/1 and because you're usually not sure which protocol version
you're going to get, you need to limit your outbound HTTP requests
anyway in case you're hitting an HTTP/1 server.

And nowadays we have the Go 1.11 Transport.MaxConnsPerHost knob too
(CL 71272 for golang/go#13957). It doesn't yet work for HTTP/2, but it
will in either Go 1.12 or Go 1.13 (golang/go#27753)

After this is bundled into net/http's, the default HTTP client will
have this knob set false, restoring the old Go 1.9 behavior where new
TCP connections are created as necessary. Users wanting the strict
behavior and import golang.org/x/net/http2 themselves and make a
Transport with StrictMaxConcurrentStreams set to true. Or they can set
Transport.MaxConnsPerHost, once that works for HTTP/2.

Updates golang/go#27044 (fixes after bundle into std)

Change-Id: I4efdad7698feaf674ee8e01032d2dfa5c2f8a3a8
Reviewed-on: https://go-review.googlesource.com/c/151857
Reviewed-by: Andrew Bonventre <[email protected]>

http2/transport.go

@@ -97,6 +97,16 @@ type Transport struct {
 	// to mean no limit.
 	MaxHeaderListSize uint32
 
+	// StrictMaxConcurrentStreams controls whether the server's
+	// SETTINGS_MAX_CONCURRENT_STREAMS should be respected
+	// globally. If false, new TCP connections are created to the
+	// server as needed to keep each under the per-connection
+	// SETTINGS_MAX_CONCURRENT_STREAMS limit. If true, the
+	// server's SETTINGS_MAX_CONCURRENT_STREAMS is interpreted as
+	// a global limit and callers of RoundTrip block when needed,
+	// waiting for their turn.
+	StrictMaxConcurrentStreams bool
+
 	// t1, if non-nil, is the standard library Transport using
 	// this transport. Its settings are used (but not its
 	// RoundTrip method, etc).
@@ -711,8 +721,19 @@ func (cc *ClientConn) idleStateLocked() (st clientConnIdleState) {
 	if cc.singleUse && cc.nextStreamID > 1 {
 		return
 	}
-	st.canTakeNewRequest = cc.goAway == nil && !cc.closed && !cc.closing &&
-		int64(cc.nextStreamID)+int64(cc.pendingRequests) < math.MaxInt32
+	var maxConcurrentOkay bool
+	if cc.t.StrictMaxConcurrentStreams {
+		// We'll tell the caller we can take a new request to
+		// prevent the caller from dialing a new TCP
+		// connection, but then we'll block later before
+		// writing it.
+		maxConcurrentOkay = true
+	} else {
+		maxConcurrentOkay = int64(len(cc.streams)+1) < int64(cc.maxConcurrentStreams)
+	}
+
+	st.canTakeNewRequest = cc.goAway == nil && !cc.closed && !cc.closing && maxConcurrentOkay &&
+		int64(cc.nextStreamID)+2*int64(cc.pendingRequests) < math.MaxInt32
 	st.freshConn = cc.nextStreamID == 1 && st.canTakeNewRequest
 	return
 }

http2/transport_test.go

@@ -3529,6 +3529,8 @@ func TestTransportResponseDataBeforeHeaders(t *testing.T) {
 	}
 	ct.run()
 }
+
+// tests Transport.StrictMaxConcurrentStreams
 func TestTransportRequestsStallAtServerLimit(t *testing.T) {
 	const maxConcurrent = 2
 
@@ -3582,6 +3584,7 @@ func TestTransportRequestsStallAtServerLimit(t *testing.T) {
 	}()
 
 	ct := newClientTester(t)
+	ct.tr.StrictMaxConcurrentStreams = true
 	ct.client = func() error {
 		var wg sync.WaitGroup
 		defer func() {