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

feat(http2): add initial_max_send_streams method to HTTP/2 client builder #3524

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

magurotuna
Copy link
Contributor

@magurotuna magurotuna commented Jan 13, 2024

This adds initial_max_send_streams method to the HTTP/2 client builder.

The value set via this method will then be forwarded to h2::client::Builder::initial_max_send_streams. If not set, the default value defined in h2 crate will be used in case some users might rely on this behavior. However, in the following version we plan to explicitly set 100 as a reasonable default value (see hyperium/h2#731 for rationale on why this can be considered a reasonable value).

@seanmonstar
Copy link
Member

I had this thought, and I'd love to hear other opinions. I can be convinced either way. But:

Should the feature be exposed in 1.2, and the default changed in 1.3? I wonder if there's value in giving people some time to specify their preferred option, before perhaps limiting them more than expected.

At the same time, getting the default to people faster may be more helpful to more people.

@magurotuna
Copy link
Contributor Author

Given that hyper is forming the foundation of HTTP in the Rust ecosystem, being conservative makes more sense to me. Your first idea, exposing the builder method in the next minor version and then setting the default value in one more next minor version, sounds a better one. I'd be happy to make a change to this PR to remove the default value part.

From our (developers of Deno) perspective, our eventual goal is to fix the fetch implementation that's powered by reqwest create. So it would be ideal if we could ship hyper's next version including this new builder method, and incorporate the new version into reqwest finally. (Then we can configure the initial_max_send_streams value on our side)

@seanmonstar
Copy link
Member

Alright, yea, sounds good to me too. If you want to remove the smaller default here, we can include this in 1.2 with a note that the default will change.

@magurotuna magurotuna force-pushed the initial_max_send_streams branch 2 times, most recently from fcf1ba0 to e7303e8 Compare January 19, 2024 12:08
@magurotuna magurotuna force-pushed the initial_max_send_streams branch from e7303e8 to 1e00345 Compare January 19, 2024 12:09
@magurotuna
Copy link
Contributor Author

OK, so now the part of setting the default value has been removed with a note stating the default value may change.

@seanmonstar seanmonstar merged commit fdfa60d into hyperium:master Jan 19, 2024
21 checks passed
@magurotuna magurotuna deleted the initial_max_send_streams branch January 20, 2024 02:45
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