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: Support thread-local functionality #1505

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

inq
Copy link

@inq inq commented Sep 7, 2023

This adds a feature "current-thread" which generates a trait with #[async_trait(?Send], like this:

#[tonic::async_trait(?Send)]
 impl test_server::Test for Svc {
     async fn test_request(&self, req: Request<SomeData>) -> Result<Response<SomeData>, Status> {
         Ok(Response::new(req.into_inner()))
     }
 }

I've tried make the PR as small as possible, but I apologize for the size. But the changes are really simple!

Summary

  1. Add local_executor option for tonic-build. So If we activate it, we generate altered code using async_trait(?Send). We can see an example in ‘tests/current_thread`
  2. Remove axum dependency: Current-thread executor support #1501
  3. We can distinguish the environments by two structs implementing hyper::Executor. They are existing TokioExec and newly added LocalExec.
  4. Now some structs and traits receive new generic argument, Ex. Its default variable is TokioExec, so we can preserve compatibility. Plus, to avoid direct access with LocalExec, we need to use type aliases. For example, Routes became Routes<Ex = TokioExec>, and LocalRoutes is added as a type alias for Routes<LocalExec>.
  5. The only one place with pain is transport::server::executor::define_serve_with_shutdown macro. I could not find a better way, but I think we need to wait until Tracking issue for RFC 2515, "Permit impl Trait in type aliases" rust-lang/rust#63063 is finished.

@LucioFranco
Copy link
Member

So I think we can reduce the diff here by a lot if we don't go with the feature flag method and just add a config option to tonic-build. If its more complicated that we need to rework send bounds everywhere then that might be more complicated and become out of scope for now.

@inq
Copy link
Author

inq commented Sep 11, 2023

Thank you for a good advice. Let me try another way related with trait bound.

@inq inq changed the title feat: Implement "current-thread" feature feat: Support thread-local functionality Sep 14, 2023
@inq
Copy link
Author

inq commented Sep 14, 2023

I am sorry for late submission. It's almost done except adding test cases. Would it be okay if we use trait-bound way? I can ensure that:

  1. Compatible except axum::Router
  2. zero-cost since it is decided on the compile-time
  3. Even though there were many changes, it would be easier to manage type bounds since I moved them into tonic::util::body

@inq
Copy link
Author

inq commented Sep 14, 2023

Done! I've added some tests (copied from integration tests)

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