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

Jump to bottom

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
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.10

Comments

@bradfitz
Copy link
Contributor

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
Copy link
Contributor Author

Relevant code:

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

@quentinmit quentinmit added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 19, 2016
@quentinmit
Copy link
Contributor

@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
Copy link
Contributor Author

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 mentioned this issue Aug 3, 2016
Closed
@bradfitz bradfitz added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 8, 2016
@bradfitz bradfitz self-assigned this Aug 8, 2016
gopherbot pushed a commit that referenced this issue Aug 8, 2016
@bradfitz
net/http: make Transport use new connection if over HTTP/2 concurrenc…
7a62274 
…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 pushed a commit to shadowsocks/go that referenced this issue Aug 10, 2016
@bradfitz @madeye
net/http: make Transport use new connection if over HTTP/2 concurrenc…
1f5d8d6 
…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 nathany mentioned this issue Aug 12, 2016
Open
@nathany
Copy link
Contributor

nathany commented Sep 21, 2016 •
edited

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.

This was referenced Sep 28, 2016
Open
Open
@bradfitz
Copy link
Contributor Author

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 bradfitz modified the milestones: Go1.9Maybe, Go1.8 Oct 22, 2016
@benubois benubois mentioned this issue Nov 18, 2016
Closed
@sideshow sideshow mentioned this issue Dec 17, 2016
Merged
1 task
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jun 7, 2017
@bradfitz bradfitz removed their assignment Jun 7, 2017
@bradfitz
Copy link
Contributor Author

bradfitz commented Jun 7, 2017

/cc @tombergan

@bradfitz bradfitz added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Jun 7, 2017
@tombergan tombergan mentioned this issue Jul 20, 2017
Closed
@tombergan tombergan self-assigned this Jul 31, 2017
@gopherbot
Copy link

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
@tombergan
http2: block RoundTrip when the Transport hits MaxConcurrentStreams
1c05540 
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
Copy link

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

@rs rs mentioned this issue Aug 16, 2017
Open
@c3mb0 c3mb0 mentioned this issue Aug 29, 2017
Closed
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
@tombergan
http2: block RoundTrip when the Transport hits MaxConcurrentStreams
1a22714 
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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
5 participants
@bradfitz @nathany @quentinmit @gopherbot @tombergan