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

Split apart the Body type #2345

Closed
6 of 8 tasks
seanmonstar opened this issue Nov 26, 2020 · 15 comments
Closed
6 of 8 tasks

Split apart the Body type #2345

seanmonstar opened this issue Nov 26, 2020 · 15 comments
Labels
A-body Area: body streaming. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful.
Milestone
1.0

Comments

@seanmonstar
Copy link
Member

seanmonstar commented Nov 26, 2020 •
edited
Loading

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
@seanmonstar seanmonstar added B-rfc Blocked: More comments would be useful in determine next steps. A-body Area: body streaming. B-breaking-change Blocked: this is an "API breaking change". labels Nov 26, 2020
@seanmonstar seanmonstar added this to the 0.14 milestone Nov 26, 2020
@seanmonstar
Copy link
Member Author

cc @LucioFranco @hawkw

@davidbarsky
Copy link
Contributor

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 rust-lang/rust#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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member

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
Copy link

peku33 commented Nov 29, 2020

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
Copy link
Contributor

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
Copy link
Member Author

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

@seanmonstar
Copy link
Member Author

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.

@seanmonstar seanmonstar modified the milestones: 0.14, 0.15 Dec 3, 2020
@aeryz
Copy link
Contributor

aeryz commented Jan 8, 2021

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
Copy link
Contributor

tesaguri commented Jan 8, 2021 •
edited
Loading

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.

@tesaguri tesaguri mentioned this issue Feb 6, 2021
Merged
@davidpdrsn
Copy link
Member

davidpdrsn commented Feb 6, 2021 •
edited
Loading

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 hyperium/http-body#37. I use it in ServeDir.

@aeryz aeryz mentioned this issue Feb 7, 2021
Closed
@tesaguri tesaguri mentioned this issue Mar 25, 2021
Merged
@tesaguri tesaguri mentioned this issue Aug 21, 2021
Open
@davidpdrsn davidpdrsn mentioned this issue Aug 22, 2021
Merged
davidpdrsn added a commit to tokio-rs/axum that referenced this issue Aug 22, 2021
@davidpdrsn
Add StreamBody (#237)
2322d39 
This adds `StreamBody` which converts a `Stream` of `Bytes` into a `http_body::Body`.

---

As suggested by Kestrer on Discord it would make sense for axum to provide different kinds of body types other than `Empty`, `Full`, and `hyper::Body`. There is also some talk about [splitting up `hyper::Body`](hyperium/hyper#2345) so this can be seen as getting started on that effort. axum's body types could be moved to hyper or http-body if thats the direction we decide on.

The types I'm thinking about adding are:

- `StreamBody`-  added in this PR
- `AsyncReadBody` - similar to [http-body#41](https://github.com/hyperium/http-body/pull/41/files)
- `ChannelBody` - similar to `hyper::Body::channel`
@davidpdrsn davidpdrsn mentioned this issue Nov 25, 2021
Merged
1 task
@davidpdrsn davidpdrsn mentioned this issue Dec 5, 2021
Merged
@seanmonstar seanmonstar modified the milestones: 0.15, 1.0 May 20, 2022
@seanmonstar seanmonstar mentioned this issue May 22, 2022
Closed
@seanmonstar seanmonstar removed the B-rfc Blocked: More comments would be useful in determine next steps. label Jun 8, 2022
@seanmonstar
Copy link
Member Author

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.

@seanmonstar seanmonstar added E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. C-feature Category: feature. This is adding a new feature. and removed B-breaking-change Blocked: this is an "API breaking change". labels Jun 15, 2022
@Xuanwo

This comment was marked as off-topic.

Sign in to view
@seanmonstar

This comment was marked as outdated.

Sign in to view
@seanmonstar seanmonstar modified the milestones: 1.0 RC1, 1.0 Final Sep 15, 2022
This was referenced Nov 25, 2023
Closed
Closed
@tesaguri tesaguri mentioned this issue Jan 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-body Area: body streaming. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful.
Projects
No open projects
hyper 1.0
Status: Done
Milestone
1.0
Development

No branches or pull requests
9 participants
@seanmonstar @davidpdrsn @sfackler @davidbarsky @Xuanwo @LucioFranco @peku33 @tesaguri @aeryz