Issue #1060 maximum number of streams on the client should be capped … #1071
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ped at 100 by default
2. Defer adding back to streamsQuota pool in CloseStream
menghanl
reviewed
Mar 1, 2017
Contributor
menghanl
left a comment
menghanl
left a comment
There was a problem hiding this comment.
Looks good. Some minor comments.
Thanks.
| } | ||
| var rstStream bool | ||
| defer func() { | ||
| if !rstStream { |
Contributor
There was a problem hiding this comment.
Add comments here to explain what rstStream means and our special handling for streams quota pool.
| t.framer.writeSettings(true, i.ss...) | ||
| } | ||
| case *resetStream: | ||
| t.streamsQuota.add(1) |
Contributor
There was a problem hiding this comment.
Explain a little bit about the race.
transport/transport.go
Outdated
| statusCode codes.Code | ||
| statusDesc string | ||
| // rstStream is a flag that is true when a RST stream frame | ||
| // is sent to the server signifying that this stream is closing. |
Contributor
There was a problem hiding this comment.
rstStream indicates whether a RSTStream needs to be sent to the server to signify that this stream is closing.
"is sent" is a bit confusing.
test/end2end_test.go
Outdated
|
|
||
| const defaultMaxStreamsClient = 100 | ||
|
|
||
| func TestClientExceedMaxStreamsLimit(t *testing.T) { |
Contributor
There was a problem hiding this comment.
How about TestExceedDefaultMaxStreamsLimit, as opposed to the previous test TestExceedMaxStreamsLimit.
test/end2end_test.go
Outdated
| "Conn.resetTransport failed to create client transport", | ||
| "grpc: the connection is closing", | ||
| ) | ||
| te.maxStream = 0 // Server allows infinite streams. The cap should be on client side. |
Contributor
There was a problem hiding this comment.
Add something saying that "the server won't send the settings frame for MaxConcurrentStreams"?
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…at 100 by default
fixes #1060