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

Move copy_into from AsyncRead to AsyncBufRead #1659

Closed
Nemo157 opened this issue Jun 6, 2019 · 3 comments · Fixed by #1674
Closed

Move copy_into from AsyncRead to AsyncBufRead #1659

Nemo157 opened this issue Jun 6, 2019 · 3 comments · Fixed by #1674

Comments

@Nemo157
Copy link
Member

Nemo157 commented Jun 6, 2019

Currently if you want to copy a buffered reader into a writer the easiest way is reader.copy_into(writer), this won't use the buffered readers buffer and will instead use an internal buffer in the CopyInto future.

This can be ok for some buffered readers like the provided BufReader as they will bypass their internal buffer when doing large reads (although this assumes that CopyInto will use at least the same buffersize as the BufReader), but for other buffered readers like an IntoAsyncRead wrapping a Stream<Item = Vec<u8>> there will be definite performance loss from extra copying required.

We could provide a similar function on AsyncBufRead and keep the existing one, but I think it would be better to just move it to AsyncBufRead and require users to wrap their reader in a BufReader if they want to copy from a non-buffered reader (which will also allow them to choose the buffer size for their application if they want).

@cramertj
Copy link
Member

IMO we shouldn't diverge from the std::io::copy interface, which only requires Read and Write.

@Nemo157
Copy link
Member Author

Nemo157 commented Jun 14, 2019

@cramertj how do you feel about adding an additional AsyncBufReadExt::copy_buf_into method to allow users to avoid the additional copying?

Eventually AsyncReadExt::copy_into might be able to specialize based on Self: AsyncBufRead, but I think it makes sense to be able to directly control the buffer size/guarantee you're using the buffered method when needed.

@cramertj
Copy link
Member

@Nemo157 seems fine to me!

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 a pull request may close this issue.

2 participants