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(client): add conn::Builder::max_buf_size() #1751

Merged
merged 1 commit into from
Jan 24, 2019

Conversation

almielczarek
Copy link
Contributor

This allows users to configure a limit to client connections' read and
write buffers.

Closes #1748

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.

Great start! Would you mind also added it to hyper::client::Builder?

@@ -75,6 +75,7 @@ pub struct Builder {
h1_writev: bool,
h1_title_case_headers: bool,
h1_read_buf_exact_size: Option<usize>,
max_buf_size: Option<usize>,
Copy link
Member

Choose a reason for hiding this comment

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

Since it's specific to HTTP1, what if the name were prefixed with h1_, like the others?

/// # Panics
///
/// The minimum value allowed is 8192. This method panics if the passed `max` is less than the minimum.
pub fn max_buf_size(&mut self, max: usize) -> &mut Self {
Copy link
Member

Choose a reason for hiding this comment

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

Since it's HTTP1 only, perhaps the method should be prefixed with http1_, like the other builder methods in client/mod.rs.


/// Set the maximum buffer size for the connection.
///
/// Default is ~400kb.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably document that setting this option conflicts with setting the http1_read_buf_exact_size option.

@almielczarek
Copy link
Contributor Author

Sorry. I think I merged things into my PR incorrectly.

If need be I can close this PR and reopen only with my changes applied to a fresh fork.

Anyway, your requested changes all make a lot of sense and I have applied them.

There's one thing you brought to light that we may need to think a little bit more about. With this set of changes if a user sets both of the options regarding buffer sizes, the max buffer size will override the exact buffer size regardless of the order in which they are set.

I added notes to the documentation for both options, but we may want to think about making it impossible to set both of these options at the same time by using an enum or an assertion.

@seanmonstar
Copy link
Member

seanmonstar commented Jan 24, 2019

If need be I can close this PR and reopen only with my changes applied to a fresh fork.

You don't need to close the PR, you can simply run the command git rebase upstream/master (assuming upstream is https://github.com/hyperium/hyper).

we may want to think about making it impossible to set both of these options at the same time by using an enum or an assertion.

What about just modifying the exact_read_buf... method to unset the max option if it is set, so that setting either effectively cancels the other?

This allows users to configure a limit to client connections' read and
write buffers.

Closes hyperium#1748
@almielczarek
Copy link
Contributor Author

Didn't think of that!

That seems to be the best option.

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.

Excellent work, thanks!

@seanmonstar seanmonstar merged commit 078ed82 into hyperium:master Jan 24, 2019
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