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

Example in MethodRouter::layer documentation doesn't work #2582

Closed
jorendorff opened this issue Feb 9, 2024 · 2 comments · Fixed by #2586
Closed

Example in MethodRouter::layer documentation doesn't work #2582

jorendorff opened this issue Feb 9, 2024 · 2 comments · Fixed by #2586
Labels
A-axum C-bug Category: This is a bug.

Comments

@jorendorff
Copy link

jorendorff commented Feb 9, 2024

The docs for MethodRouter::layer contain this example:

use axum::{routing::get, Router};
use tower::limit::ConcurrencyLimitLayer;

async fn handler() {}

let app = Router::new().route(
    "/",
    // All requests to `GET /` will be sent through `ConcurrencyLimitLayer`
    get(handler).layer(ConcurrencyLimitLayer::new(64)),
);

This compiles, but no concurrency limit is enforced. This is because each request to GET / is sent through a different ConcurrencyLimit instance, each with its own semaphore.

Full runnable example (test with hey -n 50 http://localhost:3000/api/test):

use axum::{routing::get, Router};
use std::time::Duration;
use tower::limit::ConcurrencyLimitLayer;

async fn handler() {
    eprintln!("/api/test in");
    tokio::time::sleep(Duration::from_secs(1)).await;
    eprintln!("/api/test out");
}

#[tokio::main]
async fn main() {
    let app = Router::new().route(
        "/api/test",
        get(handler).layer(ConcurrencyLimitLayer::new(5)),
    );

    // 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();
}
@xuorig
Copy link

xuorig commented Feb 9, 2024

Maybe more context: I believe this happens because the layer gets cloned and layer is called on every request

let layer_fn = move |route: Route<E>| route.layer(layer.clone());

More precisely it happens here when into_route is called on the handler

let route = handler.clone().into_route(state);

The fix here is to use GlobalConcurrencyLimitLayer which, hands out services that share the same Arc<Semaphore>

 let app = Router::new()
        .route("/api/test", get(|| async move { sleep(Duration::from_millis(1000)).await; println!("API called"); format!("API /api/test called") }))
        .layer(GlobalConcurrencyLimitLayer::new(5));

Interestingly this seems to work with axum 0.6, unclear on what changed, but most likely the layer happened once vs on every request?

@davidpdrsn
Copy link
Member

davidpdrsn commented Feb 11, 2024

Oh thanks for noticing! I'm actually surprised its not working. I would have expected #2483 to fix it since we no longer clone the router for each request, which we used to in 0.7 until 07.4.

I did some testing and adding .with_state(()); at the end of the router fixes it:

#[tokio::main]
async fn main() {
    let app = Router::new()
        .route(
            "/api/test",
            get(handler).layer(ConcurrencyLimitLayer::new(2)),
        )
        .with_state(()); // <-- ADD THIS

    // 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();
}

I'm not totally sure why that is but I'll do some more digging.

Update: This fixes it #2586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants