Unexpected middleware error behaviour - needs explanation / documentation?

[jacob-pro]

When implementing middleware I discovered various things which seemed confusing:

When we call the underlying service self.service.call(req) it returns a result:

As far as I can tell this service call will be Ok even if the route being called returned an error:

#[get("/broken_route")]
async fn broken_route() -> Result<String, actix_web::Error> {
    Err(std::io::Error::new(ErrorKind::Other, "500").into())
}
...
    async fn test() {
        let app = test::init_service(App::new().service(broken_route).wrap(Assert {})).await;
        let req = test::TestRequest::get().uri("/broken_route").to_request();
        let resp = test::call_service(&app, req).await;
        assert_eq!(resp.status(), http::StatusCode::INTERNAL_SERVER_ERROR);
    }
...
    fn call(&self, req: ServiceRequest) -> Self::Future {
        let service = self.service.call(req);
        Box::pin(async move {
            let result = service.await;
            println!("unwrapping service call");
            Ok(result.unwrap())
        })
    }
...
running 1 test
unwrapping service call:
test tests::test ... ok

But the service.call() can fail if another middleware in the chain fails:

E.g.

        let app = test::init_service(App::new().service(broken_route).wrap(Broken {}).wrap(Assert {})).await;
...
running 1 test
unwrapping service call:
thread 'tests::test2' panicked at 'called `Result::unwrap()` on an `Err` value: Custom { kind: Other, error: "oh no!" }', src\main.rs:146:23

Now this leads to a bug when using the ErrorHandlers middleware:

fn add_error_header<B>(
    mut res: ServiceResponse<B>,
) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
    res.response_mut().headers_mut().insert(
        header::CONTENT_TYPE,
        header::HeaderValue::from_static("Error"),
    );
    Ok(ErrorHandlerResponse::Response(res.map_into_left_body()))
}
...
    async fn test() {
        let srv = actix_test::start(|| App::new()
            .wrap(Broken {})
            .wrap(
                ErrorHandlers::new()
                    .handler(StatusCode::INTERNAL_SERVER_ERROR, add_error_header),
            )
            .service(broken_route));
        let resp = reqwest::get(srv.url("/broken_route")).await.unwrap();
        assert_eq!(resp.status(), http::StatusCode::INTERNAL_SERVER_ERROR);
        // Fails here, ErrorHandlers did not apply
        assert_eq!(resp.headers().get(header::CONTENT_TYPE).unwrap(), "Error");
    }

The Broken middleware is returning a ResponseError which will cause a 500 error, but the ErrorHandler middleware returns early because it got an error when calling the underlying middleware, so it doesn't run.

Possible Solution

Please can we have better documentation for how to handle errors when writing middleware:

1. Document when a service call can fail
2. Document that presumably a middleware should always return a ServiceResponse or risk breaking other middlewares up the chain (e.g. ErrorHandlers, Loggers, Cors etc.)?

I do wonder if a middleware should even be allowed to return an error / if the current pattern just encourages mistakes?

See Also

https://github.com/actix/actix-extras/pull/128 https://github.com/actix/actix-web/issues/1051

Your Environment

• Rust Version: 1.59
• Actix Web Version: 4.0.1

[robjtede]

I need to find time to fill in this skeleton middleware authors guide.

Marking this as a docs issue since this is just how it works until at least the next breaking change. We know it's weird and does deserve and explanation as you rightly say.