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

Allow adjustment of per-connection concurrent stream limits #1315

Merged
merged 1 commit into from Mar 18, 2022

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Feb 25, 2022

Fixes #1313.

This is useful when multiple application protocols are hosted on the same endpoint, or when application logic such as authentication might want to adjust the limits of pre-existing connections.

djc
djc previously approved these changes Feb 25, 2022
quinn-proto/src/connection/streams/state.rs Outdated Show resolved Hide resolved
djc
djc previously approved these changes Feb 27, 2022
@Ralith Ralith force-pushed the mutable-stream-flow-control branch 3 times, most recently from a0ee295 to 95c0039 Compare February 27, 2022 19:41
@Ralith
Copy link
Collaborator Author

Ralith commented Feb 27, 2022

On review, the previous implementation had significant bugs:

  • ensure_remote_streams was not idempotent
  • MAX_STREAMS frames sent in 0-RTT would not be retransmitted if 0-RTT was rejected

I've adjusted the implementation to resolve those by:

  • Counting allocated remote streams rather than concurrently opened ones
  • Testing that we allocate the expected number of streams on set_max_concurrent calls
  • Explicitly tracking whether flow control was adjusted during 0-RTT

Please re-review.

@sakridge
Copy link

Any updates on this?

@Ralith Ralith requested a review from djc March 17, 2022 18:18
@djc
Copy link
Collaborator

djc commented Mar 17, 2022

Sorry, I should review this. Will do that today (I'm on Pacific time right now).

@djc djc merged commit 88f65bd into main Mar 18, 2022
@djc djc deleted the mutable-stream-flow-control branch March 18, 2022 18:12
@djc djc mentioned this pull request May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Per-connection stream limits
3 participants