-
Notifications
You must be signed in to change notification settings - Fork 924
fix: send MAX_STREAMS when available <= 50% of initial for long lived connections #2251
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,10 +135,16 @@ pub struct StreamMap<F: BufFactory = DefaultBufFactory> { | |
| local_max_streams_bidi: u64, | ||
| local_max_streams_bidi_next: u64, | ||
|
|
||
| /// Initial maximum bidirectional stream count | ||
| initial_max_streams_bidi: u64, | ||
|
|
||
| /// Local maximum unidirectional stream count limit. | ||
| local_max_streams_uni: u64, | ||
| local_max_streams_uni_next: u64, | ||
|
|
||
| /// Initial maximum unidirectional stream count | ||
ghedo marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| initial_max_streams_uni: u64, | ||
|
|
||
| /// The total number of bidirectional streams opened by the local endpoint. | ||
| local_opened_streams_bidi: u64, | ||
|
|
||
|
|
@@ -193,9 +199,11 @@ impl<F: BufFactory> StreamMap<F> { | |
| StreamMap { | ||
| local_max_streams_bidi: max_streams_bidi, | ||
| local_max_streams_bidi_next: max_streams_bidi, | ||
| initial_max_streams_bidi: max_streams_bidi, | ||
|
|
||
| local_max_streams_uni: max_streams_uni, | ||
| local_max_streams_uni_next: max_streams_uni, | ||
| initial_max_streams_uni: max_streams_uni, | ||
|
|
||
| max_stream_window, | ||
|
|
||
|
|
@@ -654,18 +662,28 @@ impl<F: BufFactory> StreamMap<F> { | |
|
|
||
| /// Returns true if the max bidirectional streams count needs to be updated | ||
| /// by sending a MAX_STREAMS frame to the peer. | ||
| /// | ||
| /// This only sends MAX_STREAMS when available capacity is at or below 50% | ||
| /// of the initial maximum streams target. | ||
| pub fn should_update_max_streams_bidi(&self) -> bool { | ||
| let available = self | ||
| .local_max_streams_bidi | ||
| .saturating_sub(self.peer_opened_streams_bidi); | ||
| self.local_max_streams_bidi_next != self.local_max_streams_bidi && | ||
| self.local_max_streams_bidi_next / 2 > | ||
| self.local_max_streams_bidi - self.peer_opened_streams_bidi | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the logic before was to send an increase following each stream completion? Consider an application that requires 9 active requests at all times and the stream limit is 10. If 2 of the streams close and the client attempts to re-create them, it will only be able to create 1 of the two and remain in this state until 3 other streams close and trigger a max stream update. So the application would need 2 QUIC connections in order to work reliably in this case even though the number of streams it wants to keep open is less than the per connection stream limit.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, I would expect "available" to be 1 which is less than 10/2, so we should be sending MAX_STREAMS in that case. I added another test case to try and capture this scenario. |
||
| available <= self.initial_max_streams_bidi / 2 | ||
| } | ||
|
|
||
| /// Returns true if the max unidirectional streams count needs to be updated | ||
| /// by sending a MAX_STREAMS frame to the peer. | ||
| /// | ||
| /// This only send MAX_STREAMS when available capacity is at or below 50% of | ||
| /// the initial maximum streams target. | ||
| pub fn should_update_max_streams_uni(&self) -> bool { | ||
| let available = self | ||
| .local_max_streams_uni | ||
| .saturating_sub(self.peer_opened_streams_uni); | ||
| self.local_max_streams_uni_next != self.local_max_streams_uni && | ||
| self.local_max_streams_uni_next / 2 > | ||
| self.local_max_streams_uni - self.peer_opened_streams_uni | ||
| available <= self.initial_max_streams_uni / 2 | ||
| } | ||
|
|
||
| /// Returns the number of active streams in the map. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.