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

Add StreamReader to io module #2052

Merged
merged 4 commits into from
Apr 2, 2020
Merged

Add StreamReader to io module #2052

merged 4 commits into from
Apr 2, 2020

Conversation

Darksonn
Copy link
Contributor

@Darksonn Darksonn commented Jan 5, 2020

Allow conversion from a stream of chunks of bytes to an AsyncRead.

Motivation

Http crates such as hyper and reqwest provide their response as a stream of chunks of bytes, which is not easily convertible into a type that implements AsyncRead.

This type was originally motivated when helping someone on the official Rust discord.

Solution

This PR provides a new type called IntoAsyncRead that buffers the last chunk received from the stream, and implements both AsyncRead and AsyncBufRead on this new type.

The futures crate has a very similar type, however that type operates on the AsyncRead trait from the futures crate as opposed to the tokio version of the trait.

Allow conversion from a stream of chunks of bytes to an `AsyncRead`.
@carllerche
Copy link
Member

At this point, I’m a bit hesitant to add more traits right now related to async-io given the current uncertainty re: future.

How would this trait be used in practice?

@Darksonn
Copy link
Contributor Author

That's fair enough. The main use case (and what originally prompted this) is when reading a response provided by e.g. hyper.

@carllerche
Copy link
Member

For some reason, I thought this was a trait... i should have paid more attention.

I think an adapter is good. As I mentioned on discord, some thoughts:

  • place it in io as stream is more abstract than "io".
  • The constructor should be a free fn to match stream::iter.
  • Generic over T: AsRef<[u8]>
  • I would avoid AsyncRead in the name.

Maybe something like:

fn byte_stream<T: ...>(stream: T) -> ByteStream<T> { ... }

I don't think ByteStream is great so I would love to hear other naming suggestions.

@Darksonn
Copy link
Contributor Author

Regarding being generic over T: AsRef<[u8]>, notice that this actually somewhat complicates the implementation, as you need both an option around the chunk, and an index to track how much of the chunk you've read.

A few naming suggestions: ByteStreamReader, ChunkStream, ChunkStreamReader, ChunkedStream, ChunkReader.

@carllerche
Copy link
Member

tokio::io::StreamReader is probably best. Assuming it is generic over T: Buf, i think that will be the only adapter in tokio::io from T: Stream.

@Darksonn Darksonn changed the title Add IntoAsyncRead type to stream module Add StreamReader to io module Mar 3, 2020
@LucioFranco
Copy link
Member

@carllerche ping

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great

@carllerche carllerche merged commit e10471d into tokio-rs:master Apr 2, 2020
hawkw added a commit that referenced this pull request Apr 4, 2020
# 0.2.16 (April 3, 2020)

### Fixes

- sync: fix a regression where `Mutex`, `Semaphore`, and `RwLock` futures no
  longer implement `Sync` (#2375)
- fs: fix `fs::copy` not copying file permissions (#2354)

### Added

- time: added `deadline` method to `delay_queue::Expired` (#2300)
- io: added `StreamReader` (#2052) 

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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.

None yet

3 participants