actix / actix/actix-web

Unexpected middleware error behaviour - needs explanation / documentation?

Đang mở
#2,736 1 bình luận 1 reaction 0 người được giao Xem trên GitHub
A-web C-docs
Ngôn ngữ chính
Rust
Star
24.8k
Fork
1.9k
Merge trung bình
23 giờ 10 phút
Pull request đã merge (30 ngày)
26

Mô tả

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

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.