Skip to content

Add IntoAsyncRead::into_inner() method and test #1997

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

acfoltzer
Copy link

@acfoltzer acfoltzer commented Dec 6, 2019

This is useful when you don't necessarily want to consume the entire stream as a reader, but still want to retain the stream for other uses.

The nested tuple/Option/tuple is a bit awkward, but minimal. Do you think it'd be worth making a single-purpose enum to return here instead? For readability of code that calls this method, I added an enum.

This is useful when you don't necessarily want to consume the entire stream as a reader, but still
want to retain the stream for other uses.
@acfoltzer
Copy link
Author

acfoltzer commented Dec 6, 2019

This build failure is pretty puzzling to me, as I cannot duplicate it even with the exact same nightly compiler on Ubuntu 18.04: https://travis-ci.com/rust-lang/futures-rs/jobs/264248283

This makes code that calls this method look far less mysterious, even if it is more verbose.
@acfoltzer acfoltzer force-pushed the acf/into-async-read-into-inner branch from 8600adf to 100594a Compare December 7, 2019 00:12
@cramertj
Copy link
Member

Hey, sorry for the delay! I don't quite have the time to properly review this right now, but I'll be sure to take a look in the next few days.

@taiki-e
Copy link
Member

taiki-e commented Aug 29, 2020

All existing into_inner methods discard the combinator state and return only the underlying stream. Any reason why this returns an own enum containing the state instead of returning the Option<St>? (Methods that return combinator state can cause problems when the internal implementation changes.)

@taiki-e taiki-e added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Nov 3, 2020
@taiki-e taiki-e added A-io Area: futures::io and removed futures-util labels Dec 17, 2020
@acfoltzer
Copy link
Author

@taiki-e sorry for the delay on this reply. The reason it returns the combinator state is to not lose the bytes that have been buffered up to that point. If we just return the stream, those buffered bytes are lost.

@taiki-e
Copy link
Member

taiki-e commented Feb 23, 2021

@acfoltzer Thanks for your response. I think the motive makes sense, but I would prefer not to expose the internal representation of these combinators.

So, I think it might be preferable to add a method like BufWriter::buffer separate from into_inner.
And it would probably have the following implementation:

    pub fn into_inner(self) -> St {
        self.stream
        // or:
        // if let ReadState::Eof = self.state { None } else { Some(self.stream) }
    }

    /// Returns a reference to the internally buffered data.
    pub fn buffer(&self) -> &[u8] {
        match self.state {
            ReadState::Ready { chunk, chunk_start } => &chunk.as_ref()[chunk_start..],
            ReadState::PendingChunk | ReadState::Eof => &[],
        }
    }

@acfoltzer What do you think?

@taiki-e taiki-e added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author labels Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: futures::io S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants