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
Configure receive window per connection #1386
Configure receive window per connection #1386
Conversation
Hi @Ralith, can you take a look at this? |
The relevant review from #1384 still applies. Copying here for convenience: To be effective, set_receive_window must increase StreamsState::local_max_data by the difference between the new connection-level receive window and the old one, and set pending.max_data = true. If the window shrinks, then instead local_max_data must not be changed, but the difference in credit passed to add_read_credits in future calls should be silently absorbed. To limit the risk of drift, let's cite the methods on TransportConfig (with intra-doc links) rather than duplicating their docs. |
Thanks @Ralith on the local_max_data when window is shrank, just to confirm when you say "absorbed", it means when we add_read_credits, we do not do the following: self.local_max_data = self.local_max_data.saturating_add(credits) Instead we do self.local_max_data = self.local_max_data.saturating_add(credits - (shrink_diff)) where shrink_diff = (new_receive_window - old_receive_window)? |
Close, but some state is required, because it could take many calls to |
Hi @Ralith, on the following
Which |
|
@Ralith -- I have addressed your feedback. Could you take another look? Thanks |
@Ralith -- a gentle reminder, can you review this again? |
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.
Thanks for the ping, this had slipped through the cracks.
Thank you for writing up those very thorough tests! Overall this looks great, just a few minor tweaks.
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.
LGTM, thanks!
Thanks @Ralith for your detailed reviews. Can you help merge this PR? I do not have permission to merge the PR against this repo. Thanks |
Usually I prefer to give @djc an opportunity to weigh in on nontrivial changes. Do you specifically need it merged quickly? |
Yes -- we have a project which we plan to use this feature tweak different data limits for different connections for the same endpoint from the server side -- related to issue #1342. |
I'll review this today. |
Sure, please submit a PR to the 0.8.x branch (squashing all the changes). |
Problem:
Currently the receive_window on the server side is obtained from the ServerConfig stored in the Endpoint, which will make all connections having the same receive_window and making per connection data limits difficult.
Solution:
Create interfaces to set the receive_window per connection.
This is to address #1342 to make receive_window configurable per connection. It only handles receive_window not the stream_receive_window which can be addressed in a separate PR.