From 6ac2fe9ee91b8597ee2e0f582aa86331d12b8024 Mon Sep 17 00:00:00 2001
From: Ian Leue <ian@braze.com>
Date: Tue, 8 Jan 2019 22:20:12 -0500
Subject: [PATCH] http2: revert Transport's strict interpretation of
 MAX_CONCURRENT_STREAMS (#1)

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 <andybons@golang.org>
---
 http2/transport.go      | 25 +++++++++++++++++++++++--
 http2/transport_test.go |  3 +++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/http2/transport.go b/http2/transport.go
index ef356d6d9..8c8d78a16 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -95,6 +95,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).
@@ -670,8 +680,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
 }
diff --git a/http2/transport_test.go b/http2/transport_test.go
index 2c0f53e5c..1400c56b1 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -3532,6 +3532,8 @@ func TestTransportResponseDataBeforeHeaders(t *testing.T) {
 	}
 	ct.run()
 }
+
+// tests Transport.StrictMaxConcurrentStreams
 func TestTransportRequestsStallAtServerLimit(t *testing.T) {
 	const maxConcurrent = 2
 
@@ -3585,6 +3587,7 @@ func TestTransportRequestsStallAtServerLimit(t *testing.T) {
 	}()
 
 	ct := newClientTester(t)
+	ct.tr.StrictMaxConcurrentStreams = true
 	ct.client = func() error {
 		var wg sync.WaitGroup
 		defer func() {