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

use afit and remove async-trait #2308

Merged
merged 18 commits into from
Sep 28, 2024
Merged

use afit and remove async-trait #2308

merged 18 commits into from
Sep 28, 2024

Conversation

lz1998
Copy link
Contributor

@lz1998 lz1998 commented Nov 10, 2023

Motivation

rust-lang/rust#115822

1.75.0 will be stable on: 28 December, 2023.

Remove async-trait and use AFIT(Async Fn In Trait) and RPITIT(Return Position Impl Trait In Traits).

Solution

@davidpdrsn
Copy link
Member

Are you able to make it actually compile?

I'm not totally up to date on the specifics of async fn in traits but last I heard there were some shortcomings around declaring/propagating Send bounds. That probably will be a deal breaker for axum.

@jplatte
Copy link
Member

jplatte commented Nov 10, 2023

The situation is this: You still can't write a generic function like fn foo<T: FromRequest>() where T::foo(): Send to say you need the returned future to be Send. However, async_trait was already enforcing Sendness for all trait implementations, and that is still possible by using fn from_request(...) -> impl Future<Output = Self> + Send in the trait declaration, such that callers can rely on all implementations fulfilling that. Implementations can then use async fn sugar.

As such, I think it makes sense to get this in for the next breaking release. It removes a bunch of indirection, and allows auto traits other than Send to be inferred in non-generic contexts. It should also make the documentation a little easier to read, and might even allow a few projects to get rid of syn & quote in their dependency trees (turbo.fish is really close, other than axum's async-trait dependency there is only tower's pin-project dependency which is still waiting on a breaking-change release though, unless the decision to not merge my PR removing it by introducing a few lines of unsafe is revisited).

@jplatte
Copy link
Member

jplatte commented Nov 10, 2023

As for this PR, it introduces a number of trait declarations that don't use + Send after -> impl Future and I think that should be changed, even if there is no code that relies on the Sendness of the returned futures in generic code inside axum.

@davidpdrsn
Copy link
Member

Thats great news! 🎉

@lz1998
Copy link
Contributor Author

lz1998 commented Nov 10, 2023

Are you able to make it actually compile?

I'm not totally up to date on the specifics of async fn in traits but last I heard there were some shortcomings around declaring/propagating Send bounds. That probably will be a deal breaker for axum.

It can be compiled on nightly version.

rustup default nightly

@lz1998
Copy link
Contributor Author

lz1998 commented Nov 13, 2023

As for this PR, it introduces a number of trait declarations that don't use + Send after -> impl Future and I think that should be changed, even if there is no code that relies on the Sendness of the returned futures in generic code inside axum.

Why is + Send nessesary? It can be compiled without + Send.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Thanks! Have now reviewed everything and it looks pretty good, just some minor things could still be improved.

axum-core/src/ext_traits/request.rs Outdated Show resolved Hide resolved
axum-core/src/ext_traits/request.rs Show resolved Hide resolved
axum-core/src/ext_traits/request_parts.rs Outdated Show resolved Hide resolved
axum-core/src/ext_traits/request_parts.rs Outdated Show resolved Hide resolved
@@ -247,12 +242,12 @@ macro_rules! impl_traits_for_either {

async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> {
$(
if let Ok(value) = FromRequestParts::from_request_parts(parts, state).await {
if let Ok(value) = $ident::from_request_parts(parts, state).await {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change this? It means that an inherent from_request_parts would be called here if it exists in addition to the one from a FromRequestParts impl, which I don't think is right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because an error occurs if I don't change this.

error[E0282]: type annotations needed
   --> axum-extra/src/either.rs:250:17
    |
250 |                 FromRequestParts::from_request_parts(parts, state).await.map(Self::$last)
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type
...
275 | impl_traits_for_either!(Either8 => [E1, E2, E3, E4, E5, E6, E7], E8);
    | -------------------------------------------------------------------- in this macro invocation
    |
    = note: this error originates in the macro `impl_traits_for_either` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0282`.
error: could not compile `axum-extra` (lib) due to 7 previous errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it to this.

            async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> {
                $(
                    if let Ok(value) = <$ident as FromRequestParts<S>>::from_request_parts(parts, state).await {
                        return Ok(Self::$ident(value));
                    }
                )*

                <$last as FromRequestParts<S>>::from_request_parts(parts, state).await.map(Self::$last)
            }

@jplatte
Copy link
Member

jplatte commented Nov 13, 2023

Why is + Send nessesary? It can be compiled without + Send.

Because it's impossible for generic functions to declare that they accept any implementer of a given trait where one or all of its async methods return a Send future.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Thanks! I think we can merge this once RPITIT & AFIT hit stable! 🎉

@lz1998
Copy link
Contributor Author

lz1998 commented Dec 16, 2023

I have rebased it to axum 0.7

@lz1998
Copy link
Contributor Author

lz1998 commented Dec 29, 2023

There are still a lot of macros need to be fixed.

::axum::extract::FromRequest::from_request

should be changed to

<#field_ty as ::axum::extract::FromRequest<#trait_generics, _>>::from_request // the second generic type is private::ViaParts
---- debug_handler::ui stdout ----
thread 'debug_handler::ui' panicked at /Users/lz1998/.cargo/registry/src/rsproxy.cn-8f6827c7555bfaf8/trybuild-1.0.86/src/run.rs:101:13:
6 of 29 tests failed

---- from_request::ui stdout ----
thread 'from_request::ui' panicked at /Users/lz1998/.cargo/registry/src/rsproxy.cn-8f6827c7555bfaf8/trybuild-1.0.86/src/run.rs:101:13:
15 of 58 tests failed



error[E0282]: type annotations needed
  --> src/main.rs:22:11
   |
22 |     host: Result<headers::Host, TypedHeaderRejection>,
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type

@lz1998
Copy link
Contributor Author

lz1998 commented Dec 29, 2023

@davidpdrsn It's hard to solve the problem of macros because of the second generic type private::ViaParts, can you help to solve this?

@davidpdrsn
Copy link
Member

Sure I'll try and take a look!

@lz1998
Copy link
Contributor Author

lz1998 commented Dec 29, 2023

Sure I'll try and take a look!

I have rebased it to the main branch.

@davidpdrsn
Copy link
Member

Thanks for all your work on this by the way ❤️

@davidpdrsn davidpdrsn added this to the 0.8 milestone Dec 30, 2023
@davidpdrsn davidpdrsn added the breaking change A PR that makes a breaking change. label Dec 30, 2023
@gabibbo97
Copy link

gabibbo97 commented Jan 7, 2024

Just to add on this, if someone was wondering about the performance implications.

(Tested with --release, lto = "thin", and codegen-units = 1)

I am comparing with the following Cargo.toml

[package]
name = "bench"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[profile.release]
lto = "thin"
codegen-units = 1

[dependencies]
tokio = { version = "1", features = [ "full" ] }

# This PR
axum = { git = "https://github.com/lz1998/axum.git", branch = "main" }

# Upstream
#axum = { git = "https://github.com/tokio-rs/axum.git", rev = "c1c917092daeb91456b41510b629c4ba9763b2b6" }

and this main.rs

use axum::{
    routing::get,
    Router,
};

#[tokio::main]
async fn main() {
    // build our application with a single route
    let app = Router::new()
        .route("/", get(|| async { "Hello, World!" }));

    // run our app with hyper, listening globally on port 3000
    let listener = tokio::net::TcpListener::bind("0.0.0.0:3000").await.unwrap();
    axum::serve(listener, app).await.unwrap();
}

Baseline results

# --release lto = "thin" codegen-units = 1
> wrk -t1 -c1 --latency http://127.0.0.1:3000    
Running 10s test @ http://127.0.0.1:3000
  1 threads and 1 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    12.35us    5.98us 330.00us   95.38%
    Req/Sec    80.57k    10.08k   88.10k    88.00%
  Latency Distribution
     50%   11.00us
     75%   12.00us
     90%   14.00us
     99%   33.00us
  800650 requests in 10.00s, 99.26MB read
Requests/sec:  80064.18
Transfer/sec:      9.93MB

> wrk -t16 -c1000 --latency http://127.0.0.1:3000
Running 10s test @ http://127.0.0.1:3000
  16 threads and 1000 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.04ms    1.16ms  39.22ms   88.95%
    Req/Sec    71.86k     8.35k  114.12k    74.25%
  Latency Distribution
     50%  608.00us
     75%    1.18ms
     90%    2.35ms
     99%    5.55ms
  11481372 requests in 10.08s, 1.39GB read
Requests/sec: 1139313.22
Transfer/sec:    141.25MB

> ls -lb target/release
-rwxr-xr-x.  2 giacomo giacomo 8417072 Jan  7 22:40 bench

PR results

# --release lto = "thin" codegen-units = 1
> wrk -t1 -c1 --latency http://127.0.0.1:3000    
Running 10s test @ http://127.0.0.1:3000
  1 threads and 1 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    11.34us    4.51us 588.00us   98.88%
    Req/Sec    86.17k     5.20k   88.44k    98.02%
  Latency Distribution
     50%   11.00us
     75%   11.00us
     90%   12.00us
     99%   16.00us
  865967 requests in 10.10s, 107.36MB read
Requests/sec:  85737.63
Transfer/sec:     10.63MB

> wrk -t16 -c1000 --latency http://127.0.0.1:3000
Running 10s test @ http://127.0.0.1:3000
  16 threads and 1000 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.02ms    1.12ms  18.49ms   88.87%
    Req/Sec    72.55k     7.59k  100.27k    67.81%
  Latency Distribution
     50%  604.00us
     75%    1.16ms
     90%    2.30ms
     99%    5.51ms
  11593246 requests in 10.06s, 1.40GB read
Requests/sec: 1152503.62
Transfer/sec:    142.88MB

> ls -lb target/release
-rwxr-xr-x.  2 giacomo giacomo 8417080 Jan  7 22:38 bench

As you can see, latency and throughput improves with almost no difference in binary size

Also interesting that the P99 latency for -c1 is halved

@rbruenig
Copy link

@lz1998 @jplatte RPITIT & AFIT (Rust 1.75) have been stable for more than 6 months now. Is there a chance that this can be merged soon? If theres any work remaining I'd like to help.

@jplatte
Copy link
Member

jplatte commented Sep 20, 2024

There is a chance, though getting a release out is a lot more involved (due to the matchit upgrade, but also a few other planned changes).

Anyways.. @lz1998 I just fixed merge conflicts for you, would you mind taking another look at the current CI failures?

@lz1998
Copy link
Contributor Author

lz1998 commented Sep 24, 2024

There is a chance, though getting a release out is a lot more involved (due to the matchit upgrade, but also a few other planned changes).

Anyways.. @lz1998 I just fixed merge conflicts for you, would you mind taking another look at the current CI failures?

The commented line will cause an error, because the second generic type private::ViaRequest is private. Can you help to solve this? @jplatte
https://github.com/lz1998/axum/blob/main/axum-macros/src/lib.rs#L162

use axum::extract::{FromRequest, Request};
use axum_extra::{
    TypedHeader,
    headers::{UserAgent},
};

struct MyExtractor {}
impl<S: Send + Sync> FromRequest<S> for MyExtractor {
    type Rejection = ();

    async fn from_request(req: Request, state: &S) -> Result<Self, Self::Rejection> {
        // let _ = <TypedHeader::<UserAgent> as FromRequest<S>>::from_request(req, state).await;
        let _ = TypedHeader::<UserAgent>::from_request(req, state).await;
        Ok(MyExtractor {})
    }
}


#[tokio::main]
async fn main() {}
error[E0277]: the trait bound `TypedHeader<UserAgent>: FromRequest<S>` is not satisfied
  --> src/main.rs:18:18
   |
18 |         let _ = <TypedHeader::<UserAgent> as FromRequest<S>>::from_request(req, state).await;
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `FromRequest<S>` is not implemented for `TypedHeader<UserAgent>`
   |
   = note: Function argument is not a valid axum extractor. 
           See `https://docs.rs/axum/0.7/axum/extract/index.html` for details
   = help: the following other types implement trait `FromRequest<S, M>`:
             (T1, T2)
             (T1, T2, T3)
             (T1, T2, T3, T4)
             (T1, T2, T3, T4, T5)
             (T1, T2, T3, T4, T5, T6)
             (T1, T2, T3, T4, T5, T6, T7)
             (T1, T2, T3, T4, T5, T6, T7, T8)
             (T1, T2, T3, T4, T5, T6, T7, T8, T9)
           and 20 others

@jplatte
Copy link
Member

jplatte commented Sep 24, 2024

Yeah, in that case you wouldn't use T::from_request, you'd use T::from_request_parts or RequestExt::extract_parts.

@jplatte
Copy link
Member

jplatte commented Sep 27, 2024

Oh, that code isn't what we actually have as an example whatever, it's macro-generated? I don't see how this would be a new problem though.. Will look at what changed in the macro code.

axum-macros/src/from_request.rs Outdated Show resolved Hide resolved
axum-macros/src/from_request.rs Outdated Show resolved Hide resolved
axum-macros/src/from_request.rs Outdated Show resolved Hide resolved
axum-macros/src/from_request.rs Outdated Show resolved Hide resolved
axum-macros/src/from_request.rs Outdated Show resolved Hide resolved
axum-macros/src/from_request.rs Outdated Show resolved Hide resolved
axum-macros/src/from_request.rs Outdated Show resolved Hide resolved
axum-macros/src/from_request.rs Outdated Show resolved Hide resolved
lz1998 and others added 6 commits September 28, 2024 18:24
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
@jplatte
Copy link
Member

jplatte commented Sep 28, 2024

Small tip, instead of accepting every suggestion individually, you can go to the files tab of the PR and batch them up into a single commit 😉

Also, I opened #2943 for the MSRV bump that's needed for this.

@jplatte jplatte enabled auto-merge (squash) September 28, 2024 21:25
@jplatte jplatte merged commit 19101f6 into tokio-rs:main Sep 28, 2024
18 checks passed
@HigherOrderLogic
Copy link
Contributor

Is it possible to get a beta release with this patch included?

@jplatte
Copy link
Member

jplatte commented Oct 1, 2024

I was planning to do a pre-release soon, yes. I'll probably wait until we have merged the matchit upgrade though.

@jplatte
Copy link
Member

jplatte commented Oct 5, 2024

This is out as part of the latest set of alpha releases :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A PR that makes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants