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

Support setting flow control limits for individual stream types #3948

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Oct 30, 2023

Closes #3897.

Description

This PR adds 3 new QUIC_SETTINGS members:

  • StreamRecvWindowBidiLocalDefault
  • StreamRecvWindowBidiRemoteDefault
  • StreamRecvWindowUnidiDefault

With the semantic that if those are set, then they overwrite the StreamRecvWindowDefault setting.

This should preserve backwards compatibility since StreamRecvWindowDefault setting still has effect, but new apps can use the new settings to specify more granular limits.

Testing

Tests in SettingsTests.cpp were updated. I will also verify the change once I managed to write a test app against it.

Documentation

Documentation files were updated.

@rzikm rzikm requested a review from a team as a code owner October 30, 2023 14:54
src/core/settings.c Dismissed Show dismissed Hide dismissed
src/core/settings.c Dismissed Show dismissed Hide dismissed
src/core/settings.c Dismissed Show dismissed Hide dismissed
Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add a new test case on top of MsQuic API to make sure things work as expected.

src/core/settings.c Outdated Show resolved Hide resolved
src/core/settings.c Outdated Show resolved Hide resolved
src/core/settings.c Outdated Show resolved Hide resolved
src/core/stream.c Outdated Show resolved Hide resolved
@nibanks nibanks added Area: API Area: Core Related to the shared, core protocol logic labels Oct 30, 2023
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #3948 (1c4dce8) into main (96f730a) will increase coverage by 0.36%.
Report is 3 commits behind head on main.
The diff coverage is 90.16%.

@@            Coverage Diff             @@
##             main    #3948      +/-   ##
==========================================
+ Coverage   86.88%   87.25%   +0.36%     
==========================================
  Files          56       56              
  Lines       16901    16958      +57     
==========================================
+ Hits        14684    14796     +112     
+ Misses       2217     2162      -55     
Files Coverage Δ
src/core/connection.c 82.48% <100.00%> (+0.77%) ⬆️
src/core/stream.c 90.86% <100.00%> (+0.02%) ⬆️
src/core/settings.c 90.97% <89.47%> (+0.20%) ⬆️

... and 16 files with indirect coverage changes

@rzikm
Copy link
Member Author

rzikm commented Oct 31, 2023

@nibanks The PR should be ready for another round of review

@nibanks nibanks merged commit d33bc56 into microsoft:main Oct 31, 2023
411 of 412 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: API Area: Core Related to the shared, core protocol logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting initial flow-control limits for individual stream types
2 participants