http2: limit client initial MAX_CONCURRENT_STREAMS #73
Conversation
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
|
This PR (HEAD: 0d1114d) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/236497 to see it. Tip: You can toggle comments from me using the |
| 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. |
There was a problem hiding this comment.
Are there any restrictions that preventing setting maxConcurrentStreams in Transport configurations?
There was a problem hiding this comment.
That could be done but it only has meaning for http2
There was a problem hiding this comment.
it only has meaning for http2
What do you mean?
There was a problem hiding this comment.
Transport configurations come from: https://pkg.go.dev/net/http#Transport which is used for http1 and http2, just flagging that adding an option for initial max concurrent streams would be http2 specific.
There was a problem hiding this comment.
By using https://pkg.go.dev/net/http#Transport with function https://pkg.go.dev/golang.org/x/net/http2#ConfigureTransports limited the connection reuse cases:
Lines 173 to 176 in ad29c8a
This may result in an http2: no cached connection was available error from the lines below:
Lines 476 to 480 in ad29c8a
Aren't you modifying the http2 package source code? Why shouldn't it have a MaxConcurrentStreams specific configuration anyway?
|
Message from Brad Fitzpatrick: Patch Set 2: Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/236497. |
|
Message from Brad Fitzpatrick: Patch Set 2: Trust+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/236497. |
|
Message from Brad Fitzpatrick: Patch Set 2: Run-TryBot+1 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/236497. |
|
Message from Go Bot: Patch Set 2: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/236497. |
|
Message from Go Bot: Patch Set 2: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/236497. |
|
Message from Go Bot: Patch Set 2: TryBot-Result-1 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/236497. |
|
Message from Joe Tsai: Patch Set 2: Trust+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/236497. |
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: 0d1114d GitHub-Pull-Request: #73 Reviewed-on: https://go-review.googlesource.com/c/net/+/236497 Reviewed-by: Brad Fitzpatrick <[email protected]> Trust: Brad Fitzpatrick <[email protected]> Trust: Joe Tsai <[email protected]> Run-TryBot: Brad Fitzpatrick <[email protected]>
|
This PR is being closed because golang.org/cl/236497 has been merged. |
…RRENT_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. Updates golang/go#49076 Change-Id: I35fecd501ca39cd059c7afd1d44090b023f16e1e GitHub-Last-Rev: 0d1114d GitHub-Pull-Request: #73 Reviewed-on: https://go-review.googlesource.com/c/net/+/236497 Reviewed-by: Brad Fitzpatrick <[email protected]> Trust: Brad Fitzpatrick <[email protected]> Trust: Joe Tsai <[email protected]> Run-TryBot: Brad Fitzpatrick <[email protected]> Reviewed-on: https://go-review.googlesource.com/c/net/+/356979 Trust: Damien Neil <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
…RRENT_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. Updates golang/go#49077 Change-Id: I35fecd501ca39cd059c7afd1d44090b023f16e1e GitHub-Last-Rev: 0d1114d GitHub-Pull-Request: #73 Reviewed-on: https://go-review.googlesource.com/c/net/+/236497 Reviewed-by: Brad Fitzpatrick <[email protected]> Trust: Brad Fitzpatrick <[email protected]> Trust: Joe Tsai <[email protected]> Run-TryBot: Brad Fitzpatrick <[email protected]> Reviewed-on: https://go-review.googlesource.com/c/net/+/357674 Trust: Damien Neil <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
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 <[email protected]> Trust: Brad Fitzpatrick <[email protected]> Trust: Joe Tsai <[email protected]> Run-TryBot: Brad Fitzpatrick <[email protected]>
…RRENT_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. Updates golang/go#49077 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 <[email protected]> Trust: Brad Fitzpatrick <[email protected]> Trust: Joe Tsai <[email protected]> Run-TryBot: Brad Fitzpatrick <[email protected]> Reviewed-on: https://go-review.googlesource.com/c/net/+/357674 Trust: Damien Neil <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
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
maxConcurrentStreamsto100, the http2 specificationsrecommended minimum, until we've received the initial
SETTINGSframe from the server.After a
SETTINGSframe has been received use the serversMAX_CONCURRENT_STREAMS, if present, otherwise use1000as 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