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: Implement AsyncBufReadExt::fill_buf #1687

Closed
wants to merge 7 commits into from

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Oct 25, 2019

Adds a future wrapper around AsyncBufRead::poll_fill_buf so that it can be used without needing to poll manually. It unfortunately has to use unsafe to forget a lifetime but I hope the comment explains why this is safe.

@sfackler
Copy link
Contributor

The fact that this simple future requires unsafety to munge lifetimes makes me think that the definition of AsyncBufRead needs to change.

@taiki-e
Copy link
Member

taiki-e commented Oct 26, 2019

The fact that this simple future requires unsafety to munge lifetimes makes me think that the definition of AsyncBufRead needs to change.

Probably it will be API like rust-lang/rfcs#1015?

pub trait AsyncBufRead: AsyncRead {
    /// Attempt to read from the inner reader, and append the data onto the end of
    /// any currently buffered data.
    fn poll_read_into_buf(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<io::Result<usize>>;

    /// Get a read-only view into the buffered data.
    ///
    /// NOTE: This method **will not** do an implicit read if the buffer is empty.
    /// It will just return an empty slice.
    fn get_buf(&self) -> &[u8];

    fn consume(self: Pin<&mut Self>, amt: usize);
}

@sfackler
Copy link
Contributor

Yeah, that's what I was thinking.

Adds a future wrapper around `AsyncBufRead::poll_fill_buf`. It
unfortunately has to use unsafe to forget a lifetime but I hope the
comment explains why this is safe.
Implementing a future based `fill_buf` method proves impossible without
unsafe on the current API. By altering it to separate the reading into a
buffer and retrieving the buffer we avoid this while simultaneously
getting a more expressive API.

`poll_fill_buf` still exists as a default method implemented by calling
these new required methods.

BREAKING CHANGE

Implementors of `AsyncBufRead` must implement `poll_read_into_buf` and
`get_buf` instead of `poll_fill_buf`
@Marwes
Copy link
Contributor Author

Marwes commented Oct 28, 2019

I altered AsyncBufRead to the proposed API and left poll_fill_buf/fill_buf as default impls based on these new methods (which was easy and does not require any unsafe).

The only downside I can see is that std::io::BufRead can no longer be used to implement (a fake)AsyncBufRead

@carllerche
Copy link
Member

Thanks for doing this! Since @seanmonstar submitted the referenced RFC, I have requested his review on this PR.

@Darksonn
Copy link
Contributor

I am closing this PR in favor of refactoring the AsyncBufRead trait as described in #2428.

@Darksonn Darksonn closed this Apr 22, 2020
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.

5 participants