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

async-multipart: impl Stream for Multipart<Bytes> #51

Merged
merged 1 commit into from
Aug 30, 2021

Conversation

shikhar
Copy link
Contributor

@shikhar shikhar commented Jul 19, 2021

This allows for zero-copy when using hyper with https://docs.rs/hyper/0.14.10/hyper/struct.Body.html#method.wrap_stream

@shikhar shikhar requested a review from Jake-Shadle as a code owner July 19, 2021 16:53
@shikhar shikhar force-pushed the multipart-stream-bytes branch from de81e14 to 60db208 Compare July 19, 2021 19:15
@shikhar shikhar force-pushed the multipart-stream-bytes branch from 33dff9b to 83f0150 Compare August 30, 2021 14:32
@Jake-Shadle
Copy link
Member

Apologies, forgot about this PR, will take a look now!

@shikhar
Copy link
Contributor Author

shikhar commented Aug 30, 2021

Looks like the set of lints needs an upgrade from v0.3 to v0.4 EmbarkStudios/rust-ecosystem#59

@Jake-Shadle
Copy link
Member

Yes, I've just pushed that, just need a rebase and this PR should be good.

Copy link
Member

@Jake-Shadle Jake-Shadle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, sorry it took so long to look at it!

@shikhar shikhar force-pushed the multipart-stream-bytes branch from 83f0150 to c49d27c Compare August 30, 2021 14:51
@mergify mergify bot merged commit 9e66b31 into EmbarkStudios:main Aug 30, 2021
@shikhar shikhar deleted the multipart-stream-bytes branch August 30, 2021 14:54
@Jake-Shadle
Copy link
Member

So I realized that after I merged this that this change doesn't actually affect usage of the Multipart stream, as they are only ever sent, not received, which is what the Stream impl is for. I don't think it has a negative impact on the library so I don't think there is any harm in keeping it, and I'll add it to the CHANGELOG, but don't think this change alone warrants a release. Unless I have missed something?

@shikhar
Copy link
Contributor Author

shikhar commented Aug 30, 2021

Hey @Jake-Shadle I'm fine with no imminent release for this, since I'm able to use a git dependency.

So I realized that after I merged this that this change doesn't actually affect usage of the Multipart stream, as they are only ever sent, not received, which is what the Stream impl is for.

Hmm, I am indeed using this new impl for sending. I have in-memory data to be uploaded and when I call Object::insert_multipart() with content: Bytes, I get Multipart<Bytes>. In order to create a hyper Body, without this change the only way to get the multipart data out is to rely on the Read / AsyncRead impls which involve copying, but the underlying data is all represented as Bytes and having this Stream impl avoids copying. With hyper it is especially convenient since I am able to use https://docs.rs/hyper/0.14.12/hyper/struct.Body.html#method.wrap_stream

@Jake-Shadle
Copy link
Member

Ahh ok, thanks for clarifying, I just glanced at the docs which seem to say the inverse.

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.

2 participants