tower-http::trace on_response() callback handling is broken due to incompatible types

danwilliams commented 10 months ago

Bug Report

Version

tower-http v0.4.4

Platform

Not relevant, but Linux (Ubuntu 23.10 64-bit).

Crates

tower-http v0.4.4

Description

Using the example provided in the latest documentation, using an on_response() closure does not compile.

https://docs.rs/tower-http/latest/tower_http/trace/index.html#customization

Here is a minimal example to reproduce it:

use axum::{Router, Server, body::Body, routing::get};
use axum::http::{HeaderMap, Request, Response};
use bytes::Bytes;
use std::{net::SocketAddr, time::Duration};
use tower_http::{classify::ServerErrorsFailureClass, trace::TraceLayer};
use tracing::Span;

#[tokio::main]
async fn main() {
    let addr = SocketAddr::from(([0, 0, 0, 0], 3000));
    let app  = Router::new()
        .route("/", get(|| async { "Hello, world!" }))
        .layer(
            TraceLayer::new_for_http()
                .make_span_with(|request: &Request<Body>| {
                    tracing::debug_span!("http-request")
                })
                .on_request(|request: &Request<Body>, _span: &Span| {
                    tracing::debug!("started {} {}", request.method(), request.uri().path())
                })
                .on_response(|response: &Response<Body>, latency: Duration, _span: &Span| {
                    tracing::debug!("response generated in {:?}", latency)
                })
                .on_body_chunk(|chunk: &Bytes, latency: Duration, _span: &Span| {
                    tracing::debug!("sending {} bytes", chunk.len())
                })
                .on_eos(|trailers: Option<&HeaderMap>, stream_duration: Duration, _span: &Span| {
                    tracing::debug!("stream closed after {:?}", stream_duration)
                })
                .on_failure(|error: ServerErrorsFailureClass, latency: Duration, _span: &Span| {
                    tracing::debug!("something went wrong")
                })
        )
    ;
    tracing::info!("Listening on {}", addr);
    Server::bind(&addr)
        .serve(app.into_make_service())
        .await
        .unwrap()
    ;
}

If you delete the layer(), i.e. lines 13-33, it compiles and runs just fine. However, using these lines fails with the following error:

error[E0631]: type mismatch in closure arguments
  --> src/main.rs:13:4
   |
13 |         .layer(
   |          ^^^^^ expected due to this
...
21 |                 .on_response(|response: &Response<Body>, latency: Duration, _span: &Span| {
   |                              ------------------------------------------------------------ found signature defined here
   |
   = note: expected closure signature `for<'a, 'b> fn(&'a Response<http_body::combinators::box_body::UnsyncBoxBody<bytes::Bytes, axum::Error>>, Duration, &'b Span) -> _`
              found closure signature `for<'a, 'b> fn(&'a Response<Body>, Duration, &'b Span) -> _`
   = note: required for `[closure@src/main.rs:21:18: 21:78]` to implement `OnResponse<http_body::combinators::box_body::UnsyncBoxBody<bytes::Bytes, axum::Error>>`
   = note: required for `Trace<Route, SharedClassifier<ServerErrorsAsFailures>, ..., ..., ..., ..., ..., ...>` to implement `tower_service::Service<axum::http::Request<Body>>`

Notes

Note that the lines have been taken from the documentation unmodified.
Note also that they are not all required for the error to occur - this is enough:
```
    .layer(
        TraceLayer::new_for_http()
            .on_response(|response: &Response<Body>, latency: Duration, _span: &Span| {
                tracing::debug!("response generated in {:?}", latency)
            })
```
However, I wanted to make it clear in this report that not only is this the focal point but also that using the entire example makes no difference (not that it should, but, good to be thorough).
Finally, note that although I am using Axum, I did also try specifically using the hyper::Body and got exactly the same problem, so it's not an Axum issue.

As a side note, I had spent several hours picking through some code that I've been writing, which was not working, before tracking down the root cause of the problem and realising that it is illustrated by this example in the documentation. Hence I've used this example for the bug report, as it's very clear and shows that the documented approach causes the error, but in reality what I'm doing that made me stumble upon this is a little different (yet suffers the same cause). I was semi-relieved to find that it was a bug in tower and not my code, but also somewhat surprised 🙂

julien-leclercq commented 10 months ago

Hello, I am having the exact same problem here.

danwilliams commented 10 months ago

julien-leclercq commented 5 hours ago @danwilliams It works for me if I use the same pattern as here https://github.com/tower-rs/tower-http/blob/master/examples/axum-key-value-store/src/main.rs#L62-L92

@julien-leclercq agreed, but that's not the subject of this bug report. That example uses DefaultOnResponse, whereas the problem I have reported is when using a closure.

psibi commented 8 months ago

The above example seems to be working fine with v0.5.0

edezhic commented 8 months ago

Maybe this particular example is working now but the underlying problem is still there, I get a similar error adding

.on_response(|response: &Response, latency: Duration, span: &Span| {
    tracing::debug!("response generated in {:?}", latency)
})

which is copypasted from docs

edezhic commented 8 months ago

Looks like 0.5.1 fixed it, at least in my case

jplatte commented 7 months ago

I'll close this for now given the two reports of things working with 0.5.x. However if there are still people who need it fixed on 0.4.x, please let me know. If you could provide a repository / tarball with a crate that reproduces the problem, that would be appreciated!

tower-rs / tower-http