actix / actix/actix-web

Unexpected middleware error behaviour - needs explanation / documentation?

Abierto
#2,736 1 comentario 1 reacción 0 asignados Ver en GitHub
A-web C-docs
Lenguaje dominante
Rust
Estrellas
24.8k
Forks
1.9k
Merge medio
23 h 10 min
PR fusionados (30 d)
26

Descripción

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 {
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(
mut res: ServiceResponse,
) -> Result, 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](https://github.com/actix/actix-web/blob/master/actix-web/src/middleware/err_handlers.rs#L157) 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

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.