axum icon indicating copy to clipboard operation
axum copied to clipboard

use afit and remove async-trait

Open lz1998 opened this issue 2 years ago • 14 comments

Motivation

https://github.com/rust-lang/rust/pull/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

lz1998 avatar Nov 10 '23 07:11 lz1998

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.

davidpdrsn avatar Nov 10 '23 08:11 davidpdrsn

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 avatar Nov 10 '23 08:11 jplatte

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.

jplatte avatar Nov 10 '23 08:11 jplatte

Thats great news! 🎉

davidpdrsn avatar Nov 10 '23 10:11 davidpdrsn

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 avatar Nov 10 '23 18:11 lz1998

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.

lz1998 avatar Nov 13 '23 11:11 lz1998

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.

jplatte avatar Nov 13 '23 12:11 jplatte

I have rebased it to axum 0.7

lz1998 avatar Dec 16 '23 08:12 lz1998

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 avatar Dec 29 '23 09:12 lz1998

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

lz1998 avatar Dec 29 '23 17:12 lz1998

Sure I'll try and take a look!

davidpdrsn avatar Dec 29 '23 17:12 davidpdrsn

Sure I'll try and take a look!

I have rebased it to the main branch.

lz1998 avatar Dec 29 '23 17:12 lz1998

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

davidpdrsn avatar Dec 29 '23 19:12 davidpdrsn

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

gabibbo97 avatar Jan 07 '24 21:01 gabibbo97

@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.

rbruenig avatar Jul 14 '24 08:07 rbruenig

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?

jplatte avatar Sep 20 '24 21:09 jplatte

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

lz1998 avatar Sep 24 '24 03:09 lz1998

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

jplatte avatar Sep 24 '24 07:09 jplatte

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.

jplatte avatar Sep 27 '24 21:09 jplatte

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 avatar Sep 28 '24 10:09 jplatte

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

HigherOrderLogic avatar Oct 01 '24 07:10 HigherOrderLogic

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

jplatte avatar Oct 01 '24 07:10 jplatte

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

jplatte avatar Oct 05 '24 14:10 jplatte