[net] http2: limit client initial MAX_CONCURRENT_STREAMS
360 views
Skip to first unread message
Ian Lance Taylor (Gerrit)
Sep 7, 2021, 5:54:15 PM9/7/21
to Gerrit Bot, Steven Hartland, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Joe Tsai, Go Bot, Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Ian Lance Taylor submitted this change.
Approvals:
Brad Fitzpatrick: Looks good to me, approved; Trusted; Run TryBots
Joe Tsai: Trusted
Objections:
Go Bot: TryBots failed
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 <brad...@golang.org>
Trust: Brad Fitzpatrick <brad...@golang.org>
Trust: Joe Tsai <joe...@digital-static.net>
Run-TryBot: Brad Fitzpatrick <brad...@golang.org>
---
M http2/transport.go
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/http2/transport.go b/http2/transport.go
index dc31cfd..7d5c876 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -51,6 +51,15 @@
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 @@
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 @@
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 @@
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 @@
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()
To view, visit change 236497. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages