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

feat: Decouple NamedService from the transport feature #969

Merged
merged 4 commits into from
Jun 21, 2022

Conversation

olix0r
Copy link
Contributor

@olix0r olix0r commented Apr 9, 2022

Motivation

The tonic::transport::NamedService trait is gated by the transport
feature. This means that generated server implementations can only
include this implementation when the transport feature is enabled on
tonic-build; and if the transport feature is not used at
generation-time, then a server cannot be used with
tonic::transport::Server at runtime.

We would like to publish a crate including generated protobuf/gRPC
bindings that does not require a dependency on the transport feature
(and its fairly heavyweight dependency tree). But it would be nice if
its servers could still be used by applications that want to use
tonic::transport::Server.

Solution

By decoupling the NamedService trait from the transport feature,
tonic-build can be changed to always provide an implementation for
this trait without requiring all of the transport dependencies. This
enables generated servers to be used with tonic::transport::Server.

The `tonic::transport::NamedService` trait is gated by the `transport`
feature. This means that generated server implementations can only
include this implementation when the `transport` feature is enabled on
`tonic-build`; and if the `transport` feature is not used at
generation-time, then a server cannot be used with
`tonic::transport::Server` at runtime.

We would like to publish a crate including generated protobuf/gRPC
bindings that does not _require_ a dependency on the `transport` feature
(and its fairly heavyweight dependency tree). But it would be nice if
its servers could still be used by applications that want to use
`tonic::transport::Server`.

By decoupling the `NamedService` trait from the `transport` feature,
`tonic-build` can be changed to always provide an implementation for
this trait without requiring all of the `transport` dependencies. This
enables generated servers to be used with `tonic::transport::Server`.
@LucioFranco
Copy link
Member

I wonder if this counts as a breaking change? I don't think so...so maybe we can get this in a patch release for yall. What do you think?

@olix0r
Copy link
Contributor Author

olix0r commented Apr 11, 2022

@LucioFranco I don't think this is a breaking change, as it's purely additive. We retain the transport::NamedService export. Generated servers get an extra implementation when transport is not enabled.

That said, I worked around this by avoiding the transport::Server type in our code and instead just using hyper directly. This avoids the slew of dependencies that come in for the load balancer, etc.

It still seems worth pursuing, though, I think. I'll fix up the CI errors...

@LucioFranco
Copy link
Member

Yeah, I think its worth it. I suspect this will be a feature that will live beyond the removal of the transport module.

@olix0r olix0r marked this pull request as draft April 13, 2022 16:18
@LucioFranco
Copy link
Member

@olix0r do you still want this feature?

@LucioFranco
Copy link
Member

We could probably get this into the next release

@olix0r
Copy link
Contributor Author

olix0r commented Jun 21, 2022

I think I'd still like this... I'm juggling a few things at the moment but I'll try to get back to this; or, if someone else has a chance to take it over that would be great, too. Cc @hawkw

@hawkw
Copy link
Contributor

hawkw commented Jun 21, 2022

Happy to help get this ready to merge, thanks for the ping!

@LucioFranco
Copy link
Member

@hawkw 👍 I wanna try and get some sort of tonic release this week and we could get this merged.

@hawkw
Copy link
Contributor

hawkw commented Jun 21, 2022

@olix0r olix0r#1 updates your branch from the latest master and fixes the compilation error, if you want to merge that into this PR? otherwise, i'm happy to open a new PR that supplants this one.

Comment on lines -260 to 271
fn generate_transport(
fn generate_named(
server_service: &syn::Ident,
server_trait: &syn::Ident,
service_name: &str,
) -> TokenStream {
let service_name = syn::LitStr::new(service_name, proc_macro2::Span::call_site());

quote! {
impl<T: #server_trait> tonic::transport::NamedService for #server_service<T> {
impl<T: #server_trait> tonic::server::NamedService for #server_service<T> {
const NAME: &'static str = #service_name;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i considered just folding this back into the generate function, because it isn't generating a ton of code, and it no longer needs to be factored out for feature-flagging reasons, but i don't have a strong opinion here...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah maybe but we can do that in another refactor nbd

@hawkw
Copy link
Contributor

hawkw commented Jun 21, 2022

@LucioFranco i think this is ready for a review unless i've missed anything? i can't mark the branch as "ready for review" because i didn't open it...

@LucioFranco LucioFranco marked this pull request as ready for review June 21, 2022 17:47
Comment on lines -260 to 271
fn generate_transport(
fn generate_named(
server_service: &syn::Ident,
server_trait: &syn::Ident,
service_name: &str,
) -> TokenStream {
let service_name = syn::LitStr::new(service_name, proc_macro2::Span::call_site());

quote! {
impl<T: #server_trait> tonic::transport::NamedService for #server_service<T> {
impl<T: #server_trait> tonic::server::NamedService for #server_service<T> {
const NAME: &'static str = #service_name;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Yeah maybe but we can do that in another refactor nbd

@LucioFranco LucioFranco changed the title Decouple NamedService from the transport feature feat: Decouple NamedService from the transport feature Jun 21, 2022
@LucioFranco LucioFranco merged commit feae96c into hyperium:master Jun 21, 2022
@LucioFranco
Copy link
Member

Thanks!

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