Skip to content

feat(http): implement http_body::Body trait for Vec<u8> #130

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

akneni
Copy link

@akneni akneni commented Sep 6, 2024

Implement the http_body::Body trait for the Vec<u8> type to improve ergonomics when working with byte vectors as HTTP bodies. This change allows Vec<u8> to be used directly as a body type in hyper, reducing the need for additional wrappers.

Implement the http_body::Body trait for the Vec<u8> type to improve
ergonomics when working with byte vectors as HTTP bodies. This change
allows Vec<u8> to be used directly as a body type in hyper, reducing
the need for additional wrappers.
@akneni
Copy link
Author

akneni commented Sep 15, 2024

@seanmonstar Any thoughts on this PR?

@de-vri-es
Copy link

Seem logical to have this, considering it is implemented essentially the same already for String.

@cratelyn
Copy link
Member

i haven't had time to consider this fully, but my initial reaction to this is that i'd be concerned about this making it easier to accidentally emit bodies that do not contain valid UTF-8.

something i appreciate about hyper is that it broadly makes it difficult to violate the HTTP specification. while i understand that there are cases where UTF-8 isn't the norm, it seems vaguely good to me that one should defer to Full<D> in that case.

i haven't personally found that additional wrapper to be particularly intrusive, in my experience.

besides encoding, i believe there is a similar benefit when looking at this through a performance lens. refraining from providing a Vec<u8> implementation steers users towards types like Bytes. as the documentation of that type notes, it is intended for use in networking code, and facilitates zero-copy memory usage by allowing multiple Bytes to share the same contiguous memory.

while i am sympathetic to the idea that making Vec<u8> a body would make call-sites simpler, i would personally hesitate to introduce this change, for fear of this making it easier accidentally write slow or incorrect code.

i'll defer to @seanmonstar, but thought i would offer a response since this has been waiting for review for some time.

@de-vri-es
Copy link

I don't think http-body should be concerned about a body having UTF-8 or not. Binary bodies are very common in HTTP (images, movies, PDF, ...).

And if the user wants to emit text, they probably have a String already. It's not a common thing in Rust to accidentally have a Vec<u8> when you intended to have valid UTF-8.

I also don't see why http-body should promote Bytes over Vec<u8>. It can be more efficient then Vec<u8>, but in simple cases (like: read a file from disk and serve it, or any other case where you only use the body once) there is no benefit to using Bytes.

It seems weird to me that there is direct support for String but not for Vec<u8>. They're both the default datatype in Rust for owned data: one for text, one for binary data.

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.

3 participants