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

transport: impl Service for Channel instead of GrpcService #482

Merged
merged 3 commits into from
Jan 5, 2021
Merged

transport: impl Service for Channel instead of GrpcService #482

merged 3 commits into from
Jan 5, 2021

Conversation

ajalab
Copy link
Contributor

@ajalab ajalab commented Oct 24, 2020

Motivation

Fixes #481. Adds Service impl for Channel.

Solution

This PR replaces GrpcService impl for tonic::transport::Channel with Service impl. The two trait implementations cannot co-exist due to name conflict, but the former impl will be still alive thanks to the general trait implementation for Service implementors.

Notes:

After applying this change, existing codes that both imports GrpcService and Service traits can be broken due to multiple method definitions. This can be easily fixed by removing an import whichever or calling the preferable method explicitly. In this crate, only examples/src/tower/client.rs will be affected, so I fixed that in the second commit.

Finally, I added timeout example, which makes use of the unlocked feature by this change: wraps a channel with tower service.

@ajalab
transport: impl Service for Channel instead of GrpcService
3ae50d7 
This adds tower::Service impl for tonic::transport::Channel
and removes GrpcService impl declaration.

Channel still implements GrpcService, thanks to the general
impl declaration of GrpcService for types who implement Service.

Fixes hyperium#481.
@ajalab
examples: stop using GrpcService to avoid ambiguity
454341e
@ajalab ajalab mentioned this pull request Oct 24, 2020
Closed
@ajalab ajalab changed the title Channel impls service transport: impl Service for Channel instead of GrpcService Oct 24, 2020
@LucioFranco
Copy link
Member

This looks good 👍 Its just a breaking change, so I need to think about how we will do all our breaking changes for the next release.

@ajalab
Copy link
Contributor Author

ajalab commented Oct 26, 2020

Definitely. Thank you for taking the time to review!

@danburkert
Copy link
Contributor

Just found this issue after being stymied when trying to wrap Channel in a custom Service wrapper. I found the GrpcService machinery to be very difficult to work with, would it be possible to just remove it and inline the necessary bounds everywhere it's currently used? Trait aliases would be a better solution, but AFAIK they are not stable.

@LucioFranco
Copy link
Member

@danburkert Yeah, I think there is benefit to the type but I think I made a few mistakes in actually using it correctly. That said, I am planning a large refactor of the transport system and want to simplify tonic a bit. So I am seriously taking this all into consideration but just a bit busy right now with other things. I am planning on breaking out a chunk of time in the next month or two to work on this. Sorry for the delay all.

@LucioFranco LucioFranco merged commit e7be352 into hyperium:master Jan 5, 2021
@ajalab ajalab deleted the channel-impls-service branch January 7, 2021 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucioFranco LucioFranco LucioFranco approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Impl tower::Service for tonic::transport::Channel
3 participants
@ajalab @LucioFranco @danburkert