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

Drop async_trait #1556

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

Drop async_trait #1556

wants to merge 2 commits into from

Conversation

matze
Copy link
Contributor

@matze matze commented Oct 23, 2023

Motivation

This change was born mostly out of curiosity rather than a serious attempt to get it in any time soon. The main motivation of course is to get rid of the async_trait macro and boxed futures in favor of recently stabilized async fn in traits.

Solution

Remove usage of the async_trait macro and adjust the server codegen parts to return desugared impl Future<…> + Send because of the Send bound problem.

Caveats

This requires a nightly compiler but with that passes all tests as well as some manual tests I performed.

The larger question to answer though is how this would be merged into tonic eventually. tonic-health and tonic-reflection rely on generated code which prohibits a gradual, feature-based integration. Moreover, this would require quite a large bump of MSRV to 1.75.

@LucioFranco
Copy link
Member

@matze Nice! I think we could potentially offer this as an opt-in during codegen? I don't think there is anything stopping us from support both since its mostly codegen and user callsite code that is affected. What do you think?

@matze
Copy link
Contributor Author

matze commented Oct 27, 2023

If I understand correctly and in order to have mid-term compatibility means adding a separate flag async_trait that is automatically enabled with codegen? Even easier, if codegen is on but async_trait not, then this one here could kick in. Should indeed make the PR much smaller as well and be green. Let me try :-)

@matze
Copy link
Contributor Author

matze commented Oct 27, 2023

@LucioFranco I tested but the initial caveat still holds, all crates using pre-generated services require either async_trait or a nightly compiler. And emitting code with the feature flag check in place does not work either.

@matze
Copy link
Contributor Author

matze commented Nov 15, 2023

A similar PR is approved for axum, how do you think just breaking as well?

@LucioFranco
Copy link
Member

@LucioFranco I tested but the initial caveat still holds, all crates using pre-generated services require either async_trait or a nightly compiler. And emitting code with the feature flag check in place does not work either.

I am not following what you mean by this?

A similar tokio-rs/axum#2308 is approved for axum, how do you think just breaking as well?

I think we should if we can get it so it works for both and users can choose.

@matze
Copy link
Contributor Author

matze commented Nov 16, 2023

I am not following what you mean by this?

I mean I don't see how this could ever work for both. tonic-health and tonic-reflection contain pre-generated implementations that either fit a hypothetical feature flag that represents async-trait-less tonic or not. I mean we could go back to the times when tonic-health and tonic-reflection were generated at build time but I guess you had good reasons to not do so.

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.

None yet

2 participants