Implement AsyncBufReadExt::copy_buf_into

[Nemo157]

closes #1659

[Nemo157]

To be able to re-use CopyBufInto as the implementation of CopyInto I had to make that take the reader and writer by value, it seemed bad to have the two methods be inconsistent so I changed it here as well.

It seems like reusing the reader after copying out of it is unlikely to be a common case, and the only overhead to do so is you will have to (&mut reader).copy_into(&mut writer).await?;, being able to keep the writer accessible after copying doesn't change at all as &mut AsyncWrite + Unpin: AsyncWrite.

[cramertj]

can you clarify why you had to take it by-value in order to do this? It's somewhat surprising to me that the writer would be by-value (the reader makes decent enough sense, though), especially since we don't poll_close.

[Nemo157]

Taking the reader by-ref in both cases would look something like:

impl<R: AsyncBufRead + Unpin, W: AsyncWrite + Unpin> CopyBufInto<'_, R, W> {
    fn new(reader: &mut R, writer: &mut W) -> Self { ... }
}

struct CopyInto<'a, R, W> {
    inner: CopyBufInto<'a, R, W>,
}

impl<R: AsyncRead + Unpin, W: AsyncWrite + Unpin> CopyInto<'_, R, W> {
    fn new(reader: &mut R, writer: &mut W) -> Self {
        Self { inner: CopyBufInto::new(&mut BufReader::new(reader), writer) }
    }
}

we want to wrap the reader in a BufReader to allow the reuse, but then we don't have anywhere with ownership of that BufReader when we pass the reference into CopyBufInto::new (unless we make CopyInto a self-referential struct and only create the inner CopyBufInto when we're first polled, but I'd prefer to not deal with manually implemented self-referential structs).

It's somewhat surprising to me that the writer would be by-value

Yeah, I could change the writer back to being by-ref since we don't need to wrap that, but because of impl AsyncWrite for &mut impl AsyncWrite + Unpin you can just pass it by reference anyway (you can see that the example didn't have to change: https://github.com/rust-lang-nursery/futures-rs/pull/1674/files#diff-58a0189b65158d401601a19cc9f17257R104)

[taiki-e]

It's somewhat surprising to me that the writer would be by-value

Yeah, I could change the writer back to being by-ref since we don't need to wrap that, but because of impl AsyncWrite for &mut impl AsyncWrite + Unpin you can just pass it by reference anyway

It may be nice to have an explanation like "Note that this method consumes writer but does not close it.".

[Nemo157]

Added a note.

[cramertj]

I'm still skeptical that this is the correct route here-- it seems to me like passing in a non-Pin<&mut T> or &mut T AsyncWrite by-value is basically always going to be a bug. I don't think it's worth changing the API for the sake of consistency with another when the usage should always be different.

[Nemo157]

👍 I'll change it back to taking the writer by reference then.

[cramertj]

Thanks! Sorry for the confusing back-and-forth.