Split apart the Body type

seanmonstar commented 3 years ago

This has been suggested before: the current trait hyper::body::HttpBody should be renamed to hyper::Body, and the existing struct hyper::Body should be split up into more descriptive implementations. I'm coming around to that idea, so here's the full proposal.

Proposal

Change the trait hyper::body::HttpBody to hyper::Body
Split up the hyper::Body type:
- hyper::body::Empty: an empty body, yielding no data or trailers. Since it never yields data, its Buf type could even be some enum NeverBuf {}.
- hyper::body::Full: the full body, able to yield 1 data buffer (what hyper::Body::from(buf) is in 0.13).
- hyper::body::Streaming: the streaming bodies received from a remote over HTTP/1 or 2.
- (Optional) hyper::body::BoxBody

A client response would then be Response<Streaming> (as would a server Request<Streaming>), since they are streamed from the connection. Hopefully, this should make the intent clearer when you have a Request<Empty> or Response<Full>, instead of just Request<Body>.

Status: Accepted

Progress

[x] Remove stream variant
[x] Remove Body's Once variant
[x] Make body::Sender type and Body::channel() constructor private.
[x] Rename Body to Recv temporarily
[x] Rename HttpBody re-export to Body
[x] #2971
[ ] #3103
[ ] #3104

seanmonstar commented 3 years ago

cc @LucioFranco @hawkw

davidbarsky commented 3 years ago

As a consumer, I like this proposal.

hyper::body::Empty: an empty body, yielding no data or trailers. Since it never yields data, its Buf type could even be some enum NeverBuf {}.

I don't want to jinx this, but it's possible that ! might get stabilized with https://github.com/rust-lang/rust/pull/79366. I don't think it'll be stable in time for Hyper 0.14, but there's a chance it might be stable in time for Hyper 0.15 (if planned).

sfackler commented 3 years ago

Does this imply that the Client type can drop its body type parameter? That'd be quite nice for reasons even beyond this issue, like allowing non 'static bodies.

seanmonstar commented 3 years ago

I don't see how the client would drop its body type parameter... it still needs to know what kind of bodies you expect to send.

LucioFranco commented 3 years ago

I like this proposal, I'd also consider adding a GenericBody which is an enum of all of the bodies + the boxed one? We may also want to provide utils like EitherBody to compose them. Overall, I am a fan of this, no major nits.

peku33 commented 3 years ago

Would be nice to still be able to use all types at once, as @LucioFranco said. I belive most of apps mix those, eg. Full for data, Streaming for SSE etc. If body type goes into server template parameter, it would be a bit harder to return multi-variant version. Boxing is an option, but still requires some allocations. Having enum of three types doesn't sound bad.

davidbarsky commented 3 years ago

Something I didn't consider when responding to this initially: what implications, if any, does this proposed change have for h2-style patterns where the initial handshake request has a body of () but the client subsequently expected to write to a stream handle?

seanmonstar commented 3 years ago

If you expect different requests to have different body behaviors, you can use a body type that captures that.

seanmonstar commented 3 years ago

So, it does seem like this is desirable, but I'm going to punt to the next milestone, since proposing this right when 0.14 was almost ready is kinda late.

aeryz commented 3 years ago

I'm interested in implementing this if it is ok. I agree with @LucioFranco that we need to have a GenericBody type. BoxBody would also work but it requires allocation. I also don't see a Channel type on @seanmonstar's proposal, so I guess we need to add that too.

tesaguri commented 3 years ago

Those body types (maybe except for Streaming and GenericBody?) seem to be unspecific to hyper and to be able to be implemented in http-body or somewhere. They can be generic over the data and error types so that hyper can re-export them as, for example, pub type Full<T = Bytes, E = crate::Error> = http_body::Full<T, E>.

Having concrete Body implementations outside of hyper might be useful for abstractions (like tower-http) that do not necessarily depend on a specific version of hyper.

davidpdrsn commented 3 years ago

Having concrete Body implementations outside of hyper might be useful for abstractions (like tower-http) that do not necessarily depend on a specific version of hyper.

I'd like to +1 this. Body things that aren't hyper specific would be nice to have in http-body if thats appropriate.

Earlier today I opened a PR for adding Empty to http-body https://github.com/hyperium/http-body/pull/37. I use it in ServeDir.

seanmonstar commented 2 years ago

The http-body-util crate now has many of these variants. We likely won't re-export directly from hyper, since that util crate is less stable. Next steps for hyper 1.0 are to remove the internal variants of hyper::Body, and replace their usage in tests and examples with those from http-body-util.

Xuanwo commented 2 years ago

Hi, @seanmonstar. I'm trying to work on the Remove Body's Once variant.

However, I met a problem on the ffi part:

ffi_fn! {
    /// Construct a new HTTP request.
    fn hyper_request_new() -> *mut hyper_request {
        Box::into_raw(Box::new(hyper_request(Request::new(Body::empty()))))
    } ?= std::ptr::null_mut()
}

I can't remove Body::empty() (which depends on Once) because hyper_request uses Body directly:

pub struct hyper_request(pub(super) Request<Body>);

Any ideas?

seanmonstar commented 2 years ago

Oh, good catch! Here, I broke it out into a sub-issue, so we can chat there about this specific part: https://github.com/hyperium/hyper/issues/2922

hyperium / hyper

Split apart the Body type #2345

Proposal

Progress