futures-rs icon indicating copy to clipboard operation
futures-rs copied to clipboard

Add `IntoAsyncRead::into_inner()` method and test

Open acfoltzer opened this issue 5 years ago • 5 comments

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.

acfoltzer avatar Dec 06 '19 22:12 acfoltzer

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

acfoltzer avatar Dec 06 '19 23:12 acfoltzer

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.

cramertj avatar Dec 10 '19 22:12 cramertj

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 avatar Aug 29 '20 07:08 taiki-e

@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.

acfoltzer avatar Feb 05 '21 23:02 acfoltzer

@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 avatar Feb 23 '21 03:02 taiki-e