-
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?
Conversation
… connections The current logic sends MAX_STREAMS too aggressively. As the next MAX_STREAMS value increases, the condition for whether or not to send MAX_STREAMS becomes unconditionally true. This change fixes that so that MAX_STREAMS is only sent when 50% or more streams have completed.
| .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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
The current logic sends MAX_STREAMS too aggressively. As the next MAX_STREAMS value increases, the condition for whether or not to send MAX_STREAMS becomes unconditionally true. This change fixes that so that MAX_STREAMS is only sent when 50% or more streams have completed.