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

Remove Chunk in favor of using Bytes directly #1931

Closed
abonander opened this issue Sep 6, 2019 · 7 comments · Fixed by #2048
Closed

Remove Chunk in favor of using Bytes directly #1931

abonander opened this issue Sep 6, 2019 · 7 comments · Fixed by #2048
Labels
A-body Area: body streaming. B-rfc Blocked: More comments would be useful in determine next steps.
Milestone

Comments

@abonander
Copy link
Contributor

abonander commented Sep 6, 2019

Compared to Bytes, Chunk is pointless and kinda frustrating:

  • It's a trivial wrapper around Bytes but Bytes is exposed via the From/Into impl anyways so it's not like it's keeping it out of the public API
  • Chunk adds 0 new functionality over Bytes, all of its methods forwards to Bytes anyway
  • There's no way to split a Chunk without round-tripping through Bytes
  • Any third-party crate that wants to build on Stream<Item = Bytes> needs special adapters to work with hyper::Body

That last one is a real problem for me because I'm finally updating multipart-async and trying to maximize interop by generifying the API as much as possible. Because hyper::Body doesn't implement AsyncRead I've instead standardized around a Stream<Item = impl BodyChunk> API where BodyChunk covers two operations: splitting and dereffing to &[u8]. My final constraint is that I want to be able to return the same impl BodyChunk type, mostly to make it easy to drop multipart-async into existing request handlers.

Without implementing BodyChunk specifically for hyper::Chunk while still having an applicable impl for it, I would need an impl that's generic over T: From<Bytes>, Bytes: From<T>, T: AsRef<[u8]>. My hangup on this is that I now cannot implement BodyChunk for &[u8] directly, which complicates testing as I have to convert all bytestring literals to Bytes. (I also want to avoid a generic impl based on Into<Bytes> as that operation may perform implicit copies and I'm trying to be near-zero-copy.)

I realize Chunk exists because it used to be a custom implementation of Bytes but at this point it can be changed to just be a type alias and basically nothing will break (except usages of Chunk::into_bytes(), ironically).

@davidbarsky
Copy link
Contributor

I'm a firm +1 to this suggestion. I'd also be okay with maybe replacing Bytes usage with just Vec<u8>, but I feel less strongly about the latter point.

@abonander
Copy link
Contributor Author

I haven't checked if BytesMut is reused internally once all strong refs are gone. I assumed that was the major reason for using Bytes in the first place.

@davidbarsky
Copy link
Contributor

I think it might be, but I’m not qualified to give an answer on that at the moment. Disregard my comment about Bytes!

@seanmonstar seanmonstar added A-body Area: body streaming. B-rfc Blocked: More comments would be useful in determine next steps. labels Sep 7, 2019
@seanmonstar seanmonstar added this to the 0.13 milestone Sep 7, 2019
@seanmonstar
Copy link
Member

This could work out. I'd just hope all the same convenient constructors match up (I think in bytes 0.4, Bytes::from(static_str) will make a copy, but that's fixed in 0.5.x).

@davidbarsky
Copy link
Contributor

When you say "same convenient constructors match up", are you referring to the constructors on Bytes in 0.5.x, or something else?

@seanmonstar
Copy link
Member

Yea, I think my concerns are fixed in 0.5.x, so I'd probably wait until that is available before making the change in hyper.

@bstrie
Copy link

bstrie commented Dec 5, 2019

@seanmonstar , I interpreted this issue as implying that you wanted to make Chunk into a type alias, but it's also possible that you intended that Chunk should be removed entirely. I've just submitted a PR for the former.

seanmonstar added a commit that referenced this issue Dec 6, 2019
Closes #1931

BREAKING CHANGE: All usage of `hyper::Chunk` should be replaced with
  `bytes::Bytes` (or `hyper::body::Bytes`).
seanmonstar added a commit that referenced this issue Dec 6, 2019
Closes #1931

BREAKING CHANGE: All usage of `hyper::Chunk` should be replaced with
  `bytes::Bytes` (or `hyper::body::Bytes`).
seanmonstar added a commit that referenced this issue Dec 6, 2019
Closes #1931

BREAKING CHANGE: All usage of `hyper::Chunk` should be replaced with
  `bytes::Bytes` (or `hyper::body::Bytes`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-body Area: body streaming. B-rfc Blocked: More comments would be useful in determine next steps.
Projects
None yet
4 participants