From f8bed21638790b6d748cd04178fea36ccce79ec2 Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Thu, 4 Jun 2020 09:58:11 +0000 Subject: [PATCH] http2: limit client initial MAX_CONCURRENT_STREAMS Prevent the client trying to establish more streams than the server is willing to accept during the initial life time of a connection by limiting `maxConcurrentStreams` to `100`, the http2 specifications recommended minimum, until we've received the initial `SETTINGS` frame from the server. After a `SETTINGS` frame has been received use the servers `MAX_CONCURRENT_STREAMS`, if present, otherwise use `1000` as a reasonable value. For normal consumers this will have very little impact, allowing a decent level of concurrency from the start, and for highly concurrent consumers or large bursts it will prevent significant number of rejected streams being attempted hence actually increasing performance. Fixes golang/go#39389 Change-Id: I35fecd501ca39cd059c7afd1d44090b023f16e1e GitHub-Last-Rev: 0d1114d3a558cefed17008aba3e4a4d7b2ad3866 GitHub-Pull-Request: golang/net#73 Reviewed-on: https://go-review.googlesource.com/c/net/+/236497 Reviewed-by: Brad Fitzpatrick Trust: Brad Fitzpatrick Trust: Joe Tsai Run-TryBot: Brad Fitzpatrick --- http2/transport.go | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/http2/transport.go b/http2/transport.go index dc31cfd75..7d5c876ad 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -51,6 +51,15 @@ const ( transportDefaultStreamMinRefresh = 4 << 10 defaultUserAgent = "Go-http-client/2.0" + + // initialMaxConcurrentStreams is a connections maxConcurrentStreams until + // it's received servers initial SETTINGS frame, which corresponds with the + // spec's minimum recommended value. + initialMaxConcurrentStreams = 100 + + // defaultMaxConcurrentStreams is a connections default maxConcurrentStreams + // if the server doesn't include one in its initial SETTINGS frame. + defaultMaxConcurrentStreams = 1000 ) // Transport is an HTTP/2 Transport. @@ -247,6 +256,7 @@ type ClientConn struct { doNotReuse bool // whether conn is marked to not be reused for any future requests closing bool closed bool + seenSettings bool // true if we've seen a settings frame, false otherwise wantSettingsAck bool // we sent a SETTINGS frame and haven't heard back goAway *GoAwayFrame // if non-nil, the GoAwayFrame we received goAwayDebug string // goAway frame's debug data, retained as a string @@ -642,10 +652,10 @@ func (t *Transport) newClientConn(c net.Conn, singleUse bool) (*ClientConn, erro tconn: c, readerDone: make(chan struct{}), nextStreamID: 1, - maxFrameSize: 16 << 10, // spec default - initialWindowSize: 65535, // spec default - maxConcurrentStreams: 1000, // "infinite", per spec. 1000 seems good enough. - peerMaxHeaderListSize: 0xffffffffffffffff, // "infinite", per spec. Use 2^64-1 instead. + maxFrameSize: 16 << 10, // spec default + initialWindowSize: 65535, // spec default + maxConcurrentStreams: initialMaxConcurrentStreams, // "infinite", per spec. Use a smaller value until we have received server settings. + peerMaxHeaderListSize: 0xffffffffffffffff, // "infinite", per spec. Use 2^64-1 instead. streams: make(map[uint32]*clientStream), singleUse: singleUse, wantSettingsAck: true, @@ -2353,12 +2363,14 @@ func (rl *clientConnReadLoop) processSettings(f *SettingsFrame) error { return ConnectionError(ErrCodeProtocol) } + var seenMaxConcurrentStreams bool err := f.ForeachSetting(func(s Setting) error { switch s.ID { case SettingMaxFrameSize: cc.maxFrameSize = s.Val case SettingMaxConcurrentStreams: cc.maxConcurrentStreams = s.Val + seenMaxConcurrentStreams = true case SettingMaxHeaderListSize: cc.peerMaxHeaderListSize = uint64(s.Val) case SettingInitialWindowSize: @@ -2390,6 +2402,17 @@ func (rl *clientConnReadLoop) processSettings(f *SettingsFrame) error { return err } + if !cc.seenSettings { + if !seenMaxConcurrentStreams { + // This was the servers initial SETTINGS frame and it + // didn't contain a MAX_CONCURRENT_STREAMS field so + // increase the number of concurrent streams this + // connection can establish to our default. + cc.maxConcurrentStreams = defaultMaxConcurrentStreams + } + cc.seenSettings = true + } + cc.wmu.Lock() defer cc.wmu.Unlock()