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.

Sign up for GitHub

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

Jump to bottom

How should an AsyncRead be sent as a body? #2008

Closed
tmccombs opened this issue Nov 9, 2019 · 7 comments
Closed

How should an AsyncRead be sent as a body? #2008

tmccombs opened this issue Nov 9, 2019 · 7 comments

Comments

@tmccombs
Copy link

tmccombs commented Nov 9, 2019

Is there currently a good way to send an AsyncRead as the body of a response (or a request for that matter)?

The best I could come up with is this:

    let stream = FramedRead::new(file, BytesCodec::new()).map(|r| {
        r.map(|b| b.freeze())
    });
    Ok(Response::new(Body::wrap_stream(stream)))

But there are a couple problems with this:

  1. It requires enabling the "unstable-stream" feature. The name make it sound like it is more unstable than the rest of the crate, but afaict, streams are stable in the upstream tokio and futures crates.
  2. Having to map over the Stream to convert the Result<BytesMut, _> to Result<Bytes, _> is unergonomic, especially since this pattern needs to be done each time an AsyncRead is sent as a Body (unless a custom Decoder is written to decode to Chunks or something).

For 1, I think that either the feature flag should be removed, or there should be better documentation on why it exists.

For 2, the easiest solutions would be to either implement From<BytesMut> for Chunk, or do #1931. Another option is to add direct support for AsyncRead to Body (See #1328).
@seanmonstar
Copy link
Member

  1. See Rename unstable-stream feature to just stream #2034.
  2. We might change Chunk to just Bytes (as in Remove Chunk in favor of using Bytes directly #1931).

As for direct support of AsyncRead, I'm not sure it belongs in hyper. While it might be common in some cases (it sounds like you've needed it several times), in others, the streaming API is more common (what I've found useful in apps myself).

@chrysn
Copy link

chrysn commented Dec 16, 2019

Just to clarify: This is about tokio_io::AsyncRead, right? (Because I'm trying to feed a futures::io::AsyncRead obtained from async-std into a Body).

@jedisct1
Copy link
Contributor

@chrysn did you eventually manage to do it?

@chrysn
Copy link

chrysn commented Dec 16, 2019

Yes, but rather brutally.

(By having a task spawned that reads from the AsyncRead file and feeds into a channel body. I'm not happy with it because of the task (especially as it's spawned using the tokio rt-core feature), the closure that pulls in #![feature(async_closure)] (could be avoided), and because of the copying between the buffer and the Bytes. Code at http://paste.debian.net/1121274/ if it's of interest.)

I think it'd be really convenient to have a way to get a Body right from an AsyncRead – but there was the "not sure it belongs in hyper" comment, and I'd like to understand whether that was only about tokio's AsyncRead or whether a future::AsyncRead would belong (in which case I'd open another issue, and might contribute code as I fix all my unhappiness about the current workaround).

@tmccombs
Copy link
Author

I'm not sure what the best solution is, but the main use case I have for this is streaming a File from disk to the Body without having to keep the entire file in memory at once (files can be very large), which seems like a pretty common use case.

@sfackler
Copy link
Contributor

You don't need to use hyper's Body type at all - you can pretty easily make a http_body::Body implementation wrapping an AsyncRead that just buffers into a BytesMut and then splits of Bytes from that.

@chrysn
Copy link

chrysn commented Dec 18, 2019

Using a different Body type is definitely possible (working on a task-free cleaner implementation right now), but it's a bit unwieldy (all Response<Body> become Response<Box<dyn hyper::body::HttpBody<Data=hyper::body::Bytes, Error=hyper::Error> + Send + Unpin>> (or Pin<Box<...>> without Unpin`) for as long as I respond with mixed regular and from-AsRead bodies.

I do now have an implementation at https://gitlab.com/chrysn/annex-to-web/blob/master/src/asyncread_hyperbody.rs -- I'm still quite uncertain around where I do structural pinning, and have no clue whether the error handling is any good (but the HttpBody documentation is not explicit enough for me to grasp its details).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@seanmonstar @jedisct1 @chrysn @sfackler @tmccombs