-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(tonic): rpit future for generated server traits #2283
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
base: master
Are you sure you want to change the base?
Conversation
Supported at 1.75, which is tonic's current MSRV, return position impl trait (RPIT) makes user code cleaner, friendlier in a debugger, and possibly a couple nanoseconds faster due to fewer boxed Future indirections. There is still a Box Future at the tower_service interface due to the switch from RPIT style "simple" traits and the fancier static associated type Future style from tower. It's similar to hyperium#1697 but instead I went for the full change, to evaluate the expected impact more fully. In the expected use cases where people generate their code, they would remove `#[tonic::async_trait]` from their implementations of Server stubs. You can see in all the tests and examples, that's the only user-facing delta. It is a breaking change, but it's a simplification that seemed worthwhile to me with the burden of 1-line deletion per service. The vs code auto-generated todo!() implementations are a little nicer this way too, which is a quality of life boost for me :-) Relates to: hyperium#1651 ________________ **UX before:** ```rust #[tonic::async_trait] impl Greeter for MyGreeter { ``` **UX after:** ```rust impl Greeter for MyGreeter { ``` ________________ **Testing** Due to the nature of the change, a wide range of tests exercise it. It is a change to the existing infrastructure rather than a new feature, so there's no duplication or parameterization of tests that would increase coverage.
Good luck, I tried to land it a while ago but it was blocked on raising MSRV. |
MSRV is currently 1.75, which is sufficient for this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick look and overall it looks pretty good. I think if we are going to land this though I would want this to stick as an opt-in only and let users migrate to this codegen version when they want.
Hi, Any updates on this? I think this feature's UX improvement is underrated (along with removing a redundant dependency) |
Err(tonic::Status::unimplemented("Not yet implemented")) | ||
fn #name(#self_param, request: tonic::Request<#req_message>) | ||
-> impl std::future::Future<Output = std::result::Result<tonic::Response<#res_message>, tonic::Status>> + Send { | ||
async { Err(tonic::Status::unimplemented("Not yet implemented")) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just return std::future::ready()
instead of a async block, less work for the optimizer to do.
Motivation
Probably the main motivation here is that when I use vs code to auto-generate fn's for a tonic stub, it spews out the guts of async_trait. It's educational, but it's quite heavy and unnecessary since Rust 1.75. I'm pretty confident that
Future + Send
is about as strict as tonic intends to be toward generated service stubs, but of course let's chat about it.Secondarily, I count microseconds at both my day job and my home hobby work. I also spend a lot of time stepping in a debugger. Box types are pretty annoying to deal with in the vs code debugger, and they do tend to cost a little time. I use
fn f() -> impl Future<Output = _> + Send
in all my async traits nowadays. It works great at home and at work.Solution
Supported at 1.75, which is tonic's current MSRV, return position impl trait (RPIT) makes user code cleaner, friendlier in a debugger, and possibly a couple nanoseconds faster due to fewer boxed Future indirections. There is still one Box Future at the tower_service interface due to the switch from RPIT style "simple" traits and the fancier static associated type Future style from tower.
This unwraps 2 layers of Box indirection into static types: 1 in the framework's generated GRPC code below tower, and 1 in user code specified by async_trait.
It's similar to #1697 but instead I went for the full change, to evaluate the expected impact more fully.
Impact
In the expected use cases where people generate their code, they would remove
#[tonic::async_trait]
from their implementations of Server stubs. You can see in all the tests and examples, that's the only typical user-facing delta.In the use cases where people write their own non-generated Server implementations, they would need to move the trait impl's Future type into the call function's return type. If they're reimplementing UnaryService, they would possibly need to explicitly narrow any wrapped tower_service::Future to Send as the UnaryService (and friends) now explicitly does.
It is a breaking change, but it's a simplification that seemed worthwhile to me with the burden of 1-line deletion per service. The vs code auto-generated todo!() implementations are a little nicer this way too, which is a quality of life boost for me :-)
Relates to: #1651
UX before:
UX after:
Testing
Due to the nature of the change, a wide range of tests exercise it. It is a change to the existing infrastructure rather than a new feature, so there's no duplication or parameterization of tests that would increase coverage. The tests were quite reassuring, in that they only required the removal of
#[tonic::async_trait]
, which was by design.