actix / actix/actix-web

Unexpected middleware error behaviour - needs explanation / documentation?

オープン
#2,736 コメント 1 件 リアクション 1 件 担当者 0 名 GitHub で見る
A-web C-docs
主要言語
Rust
スター
24.8k
フォーク
1.9k
平均マージ
23時間 10分
マージ済み PR(30日)
26

説明

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

コントリビューションガイド

コントリビューションガイドを開く

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。