Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/http2: block when Transport hits max concurrent streams #13774

Closed
bradfitz opened this issue Dec 29, 2015 · 8 comments
Closed

x/net/http2: block when Transport hits max concurrent streams #13774

bradfitz opened this issue Dec 29, 2015 · 8 comments
Assignees
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 29, 2015

Currently if the http2.Transport hits MAX_CONCURRENT_STREAMS for a host, it just makes a new TCP connection and creates the stream on the new connection instead.

We should probably just block the application and chill out for a bit, waiting for the host to be happy again.

We're probably respecting the letter of the spec more than the spirit of the spec.

/cc @bmizerany

@bradfitz bradfitz added this to the Go1.7 milestone Dec 29, 2015
@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Dec 29, 2015

Relevant code:

func (cc *ClientConn) canTakeNewRequestLocked() bool {
        return cc.goAway == nil &&
                int64(len(cc.streams)+1) < int64(cc.maxConcurrentStreams) &&
                cc.nextStreamID < 2147483647
}
@quentinmit

This comment has been minimized.

Copy link
Contributor

@quentinmit quentinmit commented May 19, 2016

@bradfitz @bmizerany Whose role is it to decide what the right behavior is here?

(My 2¢ is that we shouldn't build in any hidden rate limiting to the client; it's up to the user's code or the remote server to rate limit or block new connections.)

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented May 19, 2016

Usually I end up making decisions about HTTP, but often after various people express their opinions. Mine is that we should be polite by default and respect specs, unless the user configures otherwise.

Related bug: #13957

@bradfitz bradfitz modified the milestones: Go1.8, Go1.7 May 19, 2016
@bradfitz bradfitz added NeedsFix and removed NeedsInvestigation labels Aug 8, 2016
@bradfitz bradfitz self-assigned this Aug 8, 2016
gopherbot pushed a commit that referenced this issue Aug 8, 2016
…y limit

The Go HTTP/1 client will make as many new TCP connections as the user requests.

The HTTP/2 client tried to have that behavior, but the policy of
whether a connection is re-usable didn't take into account the extra 1
stream counting against SETTINGS_MAX_CONCURRENT_STREAMS so in practice
users were getting errors.

For example, if the server's advertised max concurrent streams is 100
and 200 concurrrent Go HTTP requests ask for a connection at once, all
200 will think they can reuse that TCP connection, but then 100 will
fail later when the number of concurrent streams exceeds 100.

Instead, recognize the "no cached connections" error value in the
shouldRetryRequest method, so those 100 will retry a new connection.

This is the conservative fix for Go 1.7 so users don't get errors, and
to match the HTTP/1 behavior. Issues #13957 and #13774 are the more
involved bugs for Go 1.8.

Updates #16582
Updates #13957

Change-Id: I1f15a7ce60c07a4baebca87675836d6fe03993e8
Reviewed-on: https://go-review.googlesource.com/25580
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
madeye added a commit to shadowsocks/go that referenced this issue Aug 10, 2016
…y limit

The Go HTTP/1 client will make as many new TCP connections as the user requests.

The HTTP/2 client tried to have that behavior, but the policy of
whether a connection is re-usable didn't take into account the extra 1
stream counting against SETTINGS_MAX_CONCURRENT_STREAMS so in practice
users were getting errors.

For example, if the server's advertised max concurrent streams is 100
and 200 concurrrent Go HTTP requests ask for a connection at once, all
200 will think they can reuse that TCP connection, but then 100 will
fail later when the number of concurrent streams exceeds 100.

Instead, recognize the "no cached connections" error value in the
shouldRetryRequest method, so those 100 will retry a new connection.

This is the conservative fix for Go 1.7 so users don't get errors, and
to match the HTTP/1 behavior. Issues golang#13957 and golang#13774 are the more
involved bugs for Go 1.8.

Updates golang#16582
Updates golang#13957

Change-Id: I1f15a7ce60c07a4baebca87675836d6fe03993e8
Reviewed-on: https://go-review.googlesource.com/25580
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@nathany

This comment has been minimized.

Copy link
Contributor

@nathany nathany commented Sep 21, 2016

It sounds like this might help for a scenario I'm working on. My client library receives an initial Settings frame with SettingMaxConcurrentStreams = 1 and once the connection is authenticated, receives SettingMaxConcurrentStreams = 500.

logged via: https://github.com/golang/net/blob/master/http2/transport.go#L1735

My initial thought was to gain access to the settings data via some API (possibly like httptrace?). Then I could manage the number of goroutines sending requests.

Blocking all the requests currently in flight could actually work better though. I'd be curious to try it out.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Oct 22, 2016

I'm going to punt this to Go 1.9. It's relatively low priority compared to other open stuff, and not really a problem in practice. Also, http1 already has the as-many-conn-as-requested issue, and I'd like to make their two behaviors match, which makes this more work.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Jun 7, 2017

/cc @tombergan

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 4, 2017

Change https://golang.org/cl/53250 mentions this issue: http2: block RoundTrip when the Transport hits MaxConcurrentStreams

gopherbot pushed a commit to golang/net that referenced this issue Aug 9, 2017
Currently if the http2.Transport hits SettingsMaxConcurrentStreams for a
server, it just makes a new TCP connection and creates the stream on the
new connection. This CL updates that behavior to instead block RoundTrip
until a new stream is available.

I also fixed a second bug, which was necessary to make some tests pass:
Previously, a stream was removed from cc.streams only if either (a) we
received END_STREAM from the server, or (b) we received RST_STREAM from
the server. This CL removes a stream from cc.streams if the request was
cancelled (via ctx.Close, req.Cancel, or resp.Body.Close) before
receiving END_STREAM or RST_STREAM from the server.

Updates golang/go#13774
Updates golang/go#20985
Updates golang/go#21229

Change-Id: I660ffd724c4c513e0f1cc587b404bedb8aff80be
Reviewed-on: https://go-review.googlesource.com/53250
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 9, 2017

Change https://golang.org/cl/54052 mentions this issue: net/http: update bundled http2

@gopherbot gopherbot closed this in 6b6b9f6 Aug 9, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
Currently if the http2.Transport hits SettingsMaxConcurrentStreams for a
server, it just makes a new TCP connection and creates the stream on the
new connection. This CL updates that behavior to instead block RoundTrip
until a new stream is available.

I also fixed a second bug, which was necessary to make some tests pass:
Previously, a stream was removed from cc.streams only if either (a) we
received END_STREAM from the server, or (b) we received RST_STREAM from
the server. This CL removes a stream from cc.streams if the request was
cancelled (via ctx.Close, req.Cancel, or resp.Body.Close) before
receiving END_STREAM or RST_STREAM from the server.

Updates golang/go#13774
Updates golang/go#20985
Updates golang/go#21229

Change-Id: I660ffd724c4c513e0f1cc587b404bedb8aff80be
Reviewed-on: https://go-review.googlesource.com/53250
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Aug 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.