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

Comments

Projects
None yet
2 participants
@Nemo157
Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

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

@Nemo157

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

@Nemo157 seems fine to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.