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

fix: set MAX_CONCURRENT_STREAMS to usize::MAX if no value is advertised initially #736

Conversation

magurotuna
Copy link
Contributor

@magurotuna magurotuna commented Jan 13, 2024

Currently, if the server provides no value as MAX_CONCURRENT_STREAMS in its initial SETTINGS frame, the client keeps using the value set by h2::client::Builder::initial_max_send_streams. However, given that the absence of MAX_CONCURRENT_STREAMS in the initial SETTINGS frame implies that the server is willing to accept unlimited number of concurrent streams, it is more reasonable to overwrite the value set by h2::client::Builder::initial_max_send_streams with usize::MAX in this case.

Note that this overwrite only happens when the server does not advertise MAX_CONCURRENT_STREAMS in its initial SETTINGS frame. For second and later SETTINGS frames that do not include MAX_CONCURRENT_STREAMS, the client will not update the value and keep using the previously advertised value.

This PR extracts some part from #732.
Related to #731.


Consideration

What value is set in this particular case varies among HTTP/2 implementations. For instance, Go uses 1,000 and nghttp2 uses infinity. I chose infinity tentatively, though it should work fine.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Thanks for separating this out, it's a great improvement and I appreciate that it's more compact :)

@seanmonstar seanmonstar enabled auto-merge (rebase) January 13, 2024 13:35
@seanmonstar seanmonstar merged commit d2f09fb into hyperium:master Jan 13, 2024
7 checks passed
@magurotuna magurotuna deleted the set_max_concurrent_streams_to_inf_in_case_it_is_missing branch January 14, 2024 05:30
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.

2 participants