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

Enable buffering both reads and writes #1558

Merged
merged 7 commits into from
Sep 19, 2019
Merged

Enable buffering both reads and writes #1558

merged 7 commits into from
Sep 19, 2019

Conversation

jonhoo
Copy link
Sponsor Contributor

@jonhoo jonhoo commented Sep 13, 2019

BufWriter and BufReader did not previously forward the "opposite" trait (AsyncRead for BufWriter and AsyncWrite for BufReader). This meant that there was no way to have both directions buffered at once. This patch fixes that, and introduces a convenience type + constructor for this double-wrapped construct.

tokio-io/src/io/buf_stream.rs Outdated Show resolved Hide resolved
tokio-io/src/io/buf_stream.rs Outdated Show resolved Hide resolved
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Besides @hawkw 's comments this LGTM 👍

@jonhoo
Copy link
Sponsor Contributor Author

jonhoo commented Sep 17, 2019

The clippy error seems to be in some unrelated code?

@carllerche
Copy link
Member

Merging master should fix the clippy failure (#1569).

/// small reads and writes are batched to the underlying stream.
///
/// See [`BufStream`] for details.
fn buffered_duplex(self) -> BufStream<Self>
Copy link
Member

Choose a reason for hiding this comment

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

In the past, fns that require both AsyncRead + AsyncWrite have been defined on the read half.

@carllerche
Copy link
Member

From an API point of view, I think it is common to have types that implement both AsyncRead + AsyncWrite. In this case, buffered would conflict, so we would need to differentiate those two functions.

Also, what are the pros / cons to adding buffered as fns on the trait? I believe std::io requires calling BufReader::new(my_reader).

@jonhoo
Copy link
Sponsor Contributor Author

jonhoo commented Sep 18, 2019

@carllerche Originally there was no method on the extension traits. @hawkw and @LucioFranco requested it be added in #1558 (comment). I'd be happy to remove them again. Perhaps it makes sense to only expose the duplex version in the ext trait, and not the one-way versions?

@carllerche
Copy link
Member

Personally, my vote is to leave all the constructor fns as new... in part because this is what std does and there isn't a clear consensus on how to expose them on the ext trait.

This question comes up repeatedly, so we probably should figure out some rules of thumb so we can be consistent across the misc types.

@jonhoo
Copy link
Sponsor Contributor Author

jonhoo commented Sep 18, 2019

Done.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

LGTM

@jonhoo jonhoo merged commit 9d5af20 into master Sep 19, 2019
@jonhoo jonhoo deleted the jonhoo/asyncbufstream branch September 19, 2019 18:17
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.

None yet

4 participants