feat(tonic): Use BoxBody from http-body crate
#622
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidpdrsn
added
the
C-cleanup
|
Not quite sure why the interop tests are failing. They work for me locally. I'm on macOS. |
|
@davidpdrsn are you sure you are running the right tests locally? There is a custom body impl to merge the trailers in the interop tests. |
LucioFranco
reviewed
May 7, 2021
|
I can reproduce the interop failure locally as well now. I'll look into it. |
As of `http-body` 0.4.1 its has had a `BoxBody` type similar to
`tonic::body::BoxBody`. It also has `Empty` and `Body::map_{data,err}`.
That means all the custom body things we had in tonic can basically be
replaced with that.
Note that this is a breaking change so we should merge this next time we
decide to ship a breaking release.
The breaking changes are:
- `tonic::body::Body` has been removed. I think its fine for users to
depend directly on `http-body` if they need this trait.
- `tonic::body::BoxBody` is now just a type alias for
`http_body::combinators::BoxBody<Bytes, Status>`. So the methods it
previously had are gone. The replacements are
- `tonic::body::Body::new` -> `http_body::Body::boxed`
- `tonic::body::Body::map_from` -> `http_body::Body::map_data` and
`http_body::Body::map_err` depending on which part you want to map.
- `tonic::body::Body::empty` -> `http_body::Empty`
Additionally a `Sync` bound has been added to a few methods. I actually
don't think this is a breaking change because the old
`tonic::body::Body` trait had `Sync` as a supertrait meaning the `Sync`
requirement was already there.
Fixes #557
davidpdrsn
added a commit
to hyperium/http-body
that referenced
this pull request
May 8, 2021
I noticed this while working on hyperium/tonic#622 as it broke some interop tests.
|
The bug caught by the interop tests was in |
davidpdrsn
force-pushed
the
david/new-http-body-stuff
branch
from
074f083
to
1f14b9e
Compare
davidpdrsn
changed the title
BoxBody from http-body crateBoxBody from http-body crate
davidpdrsn
added a commit
to hyperium/http-body
that referenced
this pull request
May 8, 2021
I noticed this while working on hyperium/tonic#622 as it broke some interop tests.
LucioFranco
reviewed
May 12, 2021
| @@ -1,337 +0,0 @@ | |||
| /// The message sent by the client when calling ServerReflectionInfo method. | |||
There was a problem hiding this comment.
Thought thats what you mean by #622 (comment).
Not sure what those files were for anyway. Removing them didn't break anything 🤔
LucioFranco
reviewed
May 12, 2021
| @@ -71,8 +71,8 @@ impl<T> Grpc<T> { | |||
| ) -> Result<Response<M2>, Status> | |||
| where | |||
| T: GrpcService<BoxBody>, | |||
| T::ResponseBody: Body + HttpBody + Send + 'static, | |||
| <T::ResponseBody as HttpBody>::Error: Into<crate::Error>, | |||
| T::ResponseBody: Body + Send + Sync + 'static, | |||
There was a problem hiding this comment.
Yes and no. The old tonic::body::Body had Sync as a super trait so previously Sync was implicitly required.
LucioFranco
approved these changes
May 17, 2021
BenxiangGe
pushed a commit
to BenxiangGe/http-body
that referenced
this pull request
Jul 26, 2021
…ium#42) I noticed this while working on hyperium/tonic#622 as it broke some interop tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As of
http-body0.4.1 its has had aBoxBodytype similar totonic::body::BoxBody. It also hasEmptyandBody::map_{data,err}.That means all the custom body things we had in tonic can basically be
replaced with that.
Note that this is a breaking change so we should merge this next time we
decide to ship a breaking release.
The breaking changes are:
tonic::body::Bodyhas been removed. I think its fine for users todepend directly on
http-bodyif they need this trait.tonic::body::BoxBodyis now just a type alias forhttp_body::combinators::BoxBody<Bytes, Status>. So the methods itpreviously had are gone. The replacements are
tonic::body::Body::new->http_body::Body::boxedtonic::body::Body::map_from->http_body::Body::map_dataandhttp_body::Body::map_errdepending on which part you want to map.tonic::body::Body::empty->http_body::EmptyAdditionally a
Syncbound has been added to a few methods. I actuallydon't think this is a breaking change because the old
tonic::body::Bodytrait hadSyncas a supertrait meaning theSyncrequirement was already there.
Fixes #557