Middleware::response is not invoked if error result was returned by another Middleware::start #255

src/middleware/errhandlers.rs

@@ -84,8 +84,12 @@ impl<S: 'static> Middleware<S> for ErrorHandlers<S> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use error::{Error, ErrorInternalServerError};
     use http::header::CONTENT_TYPE;
     use http::StatusCode;
+    use httpmessage::HttpMessage;
+    use middleware::Started;
+    use test;
 
     fn render_500<S>(_: &mut HttpRequest<S>, resp: HttpResponse) -> Result<Response> {
         let mut builder = resp.into_builder();
@@ -113,4 +117,27 @@ mod tests {
         };
         assert!(!resp.headers().contains_key(CONTENT_TYPE));
     }
+
+    struct MiddlewareOne;
+
+    impl<S> Middleware<S> for MiddlewareOne {
+        fn start(&mut self, _req: &mut HttpRequest<S>) -> Result<Started, Error> {
+            Err(ErrorInternalServerError("middleware error"))
+        }
+    }
+
+    #[test]
+    fn test_middleware_start_error() {
+        let mut srv = test::TestServer::new(move |app| {
+            app.middleware(
+                ErrorHandlers::new()
+                    .handler(StatusCode::INTERNAL_SERVER_ERROR, render_500),
+            ).middleware(MiddlewareOne)
+                .handler(|_| HttpResponse::Ok())
+        });
+
+        let request = srv.get().finish().unwrap();
+        let response = srv.execute(request.send()).unwrap();
+        assert_eq!(response.headers().get(CONTENT_TYPE).unwrap(), "0001");
+    }
 }

src/pipeline.rs

@@ -249,7 +249,8 @@ impl<S: 'static, H: PipelineHandler<S>> StartMiddlewares<S, H> {
                 let reply = unsafe { &mut *hnd.get() }.handle(info.req().clone(), htype);
                 return WaitingResponse::init(info, reply);
             } else {
-                let state = info.mws.borrow_mut()[info.count as usize].start(info.req_mut());
+                let state =
+                    info.mws.borrow_mut()[info.count as usize].start(info.req_mut());
                 match state {
                     Ok(Started::Done) => info.count += 1,
                     Ok(Started::Response(resp)) => {
@@ -263,7 +264,7 @@ impl<S: 'static, H: PipelineHandler<S>> StartMiddlewares<S, H> {
                             _s: PhantomData,
                         })
                     }
-                    Err(err) => return ProcessResponse::init(err.into()),
+                    Err(err) => return RunMiddlewares::init(info, err.into()),
                 }
             }
         }
@@ -297,13 +298,13 @@ impl<S: 'static, H: PipelineHandler<S>> StartMiddlewares<S, H> {
                                     continue 'outer;
                                 }
                                 Err(err) => {
-                                    return Some(ProcessResponse::init(err.into()))
+                                    return Some(RunMiddlewares::init(info, err.into()))
                                 }
                             }
                         }
                     }
                 }
-                Err(err) => return Some(ProcessResponse::init(err.into())),
+                Err(err) => return Some(RunMiddlewares::init(info, err.into())),
             }
         }
     }
@@ -403,7 +404,8 @@ impl<S: 'static, H> RunMiddlewares<S, H> {
                 if self.curr == len {
                     return Some(ProcessResponse::init(resp));
                 } else {
-                    let state = info.mws.borrow_mut()[self.curr].response(info.req_mut(), resp);
+                    let state =
+                        info.mws.borrow_mut()[self.curr].response(info.req_mut(), resp);
                     match state {
                         Err(err) => return Some(ProcessResponse::init(err.into())),
                         Ok(Response::Done(r)) => {

src/route.rs

@@ -289,7 +289,8 @@ impl<S: 'static> ComposeState<S> {
 
 impl<S: 'static> Compose<S> {
     fn new(
-        req: HttpRequest<S>, mws: Rc<RefCell<Vec<Box<Middleware<S>>>>>, handler: InnerHandler<S>,
+        req: HttpRequest<S>, mws: Rc<RefCell<Vec<Box<Middleware<S>>>>>,
+        handler: InnerHandler<S>,
     ) -> Self {
         let mut info = ComposeInfo {
             count: 0,
@@ -350,7 +351,7 @@ impl<S: 'static> StartMiddlewares<S> {
                             _s: PhantomData,
                         })
                     }
-                    Err(err) => return FinishingMiddlewares::init(info, err.into()),
+                    Err(err) => return RunMiddlewares::init(info, err.into()),
                 }
             }
         }
@@ -371,7 +372,8 @@ impl<S: 'static> StartMiddlewares<S> {
                             let reply = info.handler.handle(info.req.clone());
                             return Some(WaitingResponse::init(info, reply));
                         } else {
-                            let state = info.mws.borrow_mut()[info.count].start(&mut info.req);
+                            let state =
+                                info.mws.borrow_mut()[info.count].start(&mut info.req);
                             match state {
                                 Ok(MiddlewareStarted::Done) => info.count += 1,
                                 Ok(MiddlewareStarted::Response(resp)) => {
@@ -382,16 +384,13 @@ impl<S: 'static> StartMiddlewares<S> {
                                     continue 'outer;
                                 }
                                 Err(err) => {
-                                    return Some(FinishingMiddlewares::init(
-                                        info,
-                                        err.into(),
-                                    ))
+                                    return Some(RunMiddlewares::init(info, err.into()))
                                 }
                             }
                         }
                     }
                 }
-                Err(err) => return Some(FinishingMiddlewares::init(info, err.into())),
+                Err(err) => return Some(RunMiddlewares::init(info, err.into())),
             }
         }
     }
@@ -483,7 +482,8 @@ impl<S: 'static> RunMiddlewares<S> {
                 if self.curr == len {
                     return Some(FinishingMiddlewares::init(info, resp));
                 } else {
-                    let state = info.mws.borrow_mut()[self.curr].response(&mut info.req, resp);
+                    let state =
+                        info.mws.borrow_mut()[self.curr].response(&mut info.req, resp);
                     match state {
                         Err(err) => {
                             return Some(FinishingMiddlewares::init(info, err.into()))

src/scope.rs

@@ -519,7 +519,7 @@ impl<S: 'static> StartMiddlewares<S> {
                             _s: PhantomData,
                         })
                     }
-                    Err(err) => return Response::init(err.into()),
+                    Err(err) => return RunMiddlewares::init(info, err.into()),
                 }
             }
         }
@@ -546,7 +546,8 @@ impl<S: 'static> StartMiddlewares<S> {
                             };
                             return Some(WaitingResponse::init(info, reply));
                         } else {
-                            let state = info.mws.borrow_mut()[info.count].start(&mut info.req);
+                            let state =
+                                info.mws.borrow_mut()[info.count].start(&mut info.req);
                             match state {
                                 Ok(MiddlewareStarted::Done) => info.count += 1,
                                 Ok(MiddlewareStarted::Response(resp)) => {
@@ -556,12 +557,14 @@ impl<S: 'static> StartMiddlewares<S> {
                                     self.fut = Some(fut);
                                     continue 'outer;
                                 }
-                                Err(err) => return Some(Response::init(err.into())),
+                                Err(err) => {
+                                    return Some(RunMiddlewares::init(info, err.into()))
+                                }
                             }
                         }
                     }
                 }
-                Err(err) => return Some(Response::init(err.into())),
+                Err(err) => return Some(RunMiddlewares::init(info, err.into())),
             }
         }
     }
@@ -653,7 +656,8 @@ impl<S: 'static> RunMiddlewares<S> {
                 if self.curr == len {
                     return Some(FinishingMiddlewares::init(info, resp));
                 } else {
-                    let state = info.mws.borrow_mut()[self.curr].response(&mut info.req, resp);
+                    let state =
+                        info.mws.borrow_mut()[self.curr].response(&mut info.req, resp);
                     match state {
                         Err(err) => {
                             return Some(FinishingMiddlewares::init(info, err.into()))

tests/test_middleware.rs

@@ -8,6 +8,7 @@ use std::sync::Arc;
 use std::thread;
 use std::time::{Duration, Instant};
 
+use actix_web::error::{Error, ErrorInternalServerError};
 use actix_web::*;
 use futures::{future, Future};
 use tokio_timer::Delay;
@@ -33,7 +34,9 @@ impl<S> middleware::Middleware<S> for MiddlewareTest {
         Ok(middleware::Response::Done(resp))
     }
 
-    fn finish(&mut self, _: &mut HttpRequest<S>, _: &HttpResponse) -> middleware::Finished {
+    fn finish(
+        &mut self, _: &mut HttpRequest<S>, _: &HttpResponse,
+    ) -> middleware::Finished {
         self.finish
             .store(self.finish.load(Ordering::Relaxed) + 1, Ordering::Relaxed);
         middleware::Finished::Done
@@ -457,7 +460,9 @@ impl<S> middleware::Middleware<S> for MiddlewareAsyncTest {
         )))
     }
 
-    fn finish(&mut self, _: &mut HttpRequest<S>, _: &HttpResponse) -> middleware::Finished {
+    fn finish(
+        &mut self, _: &mut HttpRequest<S>, _: &HttpResponse,
+    ) -> middleware::Finished {
         let to = Delay::new(Instant::now() + Duration::from_millis(10));
 
         let finish = Arc::clone(&self.finish);
@@ -788,3 +793,211 @@ fn test_async_sync_resource_middleware_multiple() {
     thread::sleep(Duration::from_millis(40));
     assert_eq!(num3.load(Ordering::Relaxed), 2);
 }
+
+struct MiddlewareWithErr;
+
+impl<S> middleware::Middleware<S> for MiddlewareWithErr {
+    fn start(
+        &mut self, _req: &mut HttpRequest<S>,
+    ) -> Result<middleware::Started, Error> {
+        Err(ErrorInternalServerError("middleware error"))
+    }
+}
+
+struct MiddlewareAsyncWithErr;
+
+impl<S> middleware::Middleware<S> for MiddlewareAsyncWithErr {
+    fn start(
+        &mut self, _req: &mut HttpRequest<S>,
+    ) -> Result<middleware::Started, Error> {
+        Ok(middleware::Started::Future(Box::new(future::err(
+            ErrorInternalServerError("middleware error"),
+        ))))
+    }
+}
+
+#[test]
+fn test_middleware_chain_with_error() {
+    let num1 = Arc::new(AtomicUsize::new(0));
+    let num2 = Arc::new(AtomicUsize::new(0));
+    let num3 = Arc::new(AtomicUsize::new(0));
+
+    let act_num1 = Arc::clone(&num1);
+    let act_num2 = Arc::clone(&num2);
+    let act_num3 = Arc::clone(&num3);
+
+    let mut srv = test::TestServer::with_factory(move || {
+        let mw1 = MiddlewareTest {
+            start: Arc::clone(&act_num1),
+            response: Arc::clone(&act_num2),
+            finish: Arc::clone(&act_num3),
+        };
+        App::new()
+            .middleware(mw1)
+            .middleware(MiddlewareWithErr)
+            .resource("/test", |r| r.h(|_| HttpResponse::Ok()))
+    });
+
+    let request = srv.get().uri(srv.url("/test")).finish().unwrap();
+    let response = srv.execute(request.send()).unwrap();
+
+    assert_eq!(num1.load(Ordering::Relaxed), 1);
+    assert_eq!(num2.load(Ordering::Relaxed), 1);
+    assert_eq!(num3.load(Ordering::Relaxed), 1);
+}
+
+#[test]
+fn test_middleware_async_chain_with_error() {
+    let num1 = Arc::new(AtomicUsize::new(0));
+    let num2 = Arc::new(AtomicUsize::new(0));
+    let num3 = Arc::new(AtomicUsize::new(0));
+
+    let act_num1 = Arc::clone(&num1);
+    let act_num2 = Arc::clone(&num2);
+    let act_num3 = Arc::clone(&num3);
+
+    let mut srv = test::TestServer::with_factory(move || {
+        let mw1 = MiddlewareTest {
+            start: Arc::clone(&act_num1),
+            response: Arc::clone(&act_num2),
+            finish: Arc::clone(&act_num3),
+        };
+        App::new()
+            .middleware(mw1)
+            .middleware(MiddlewareAsyncWithErr)
+            .resource("/test", |r| r.h(|_| HttpResponse::Ok()))
+    });
+
+    let request = srv.get().uri(srv.url("/test")).finish().unwrap();
+    let response = srv.execute(request.send()).unwrap();
+
+    assert_eq!(num1.load(Ordering::Relaxed), 1);
+    assert_eq!(num2.load(Ordering::Relaxed), 1);
+    assert_eq!(num3.load(Ordering::Relaxed), 1);
+}
+
+#[test]
+fn test_scope_middleware_chain_with_error() {
+    let num1 = Arc::new(AtomicUsize::new(0));
+    let num2 = Arc::new(AtomicUsize::new(0));
+    let num3 = Arc::new(AtomicUsize::new(0));
+
+    let act_num1 = Arc::clone(&num1);
+    let act_num2 = Arc::clone(&num2);
+    let act_num3 = Arc::clone(&num3);
+
+    let mut srv = test::TestServer::with_factory(move || {
+        let mw1 = MiddlewareTest {
+            start: Arc::clone(&act_num1),
+            response: Arc::clone(&act_num2),
+            finish: Arc::clone(&act_num3),
+        };
+        App::new().scope("/scope", |scope| {
+            scope
+                .middleware(mw1)
+                .middleware(MiddlewareWithErr)
+                .resource("/test", |r| r.h(|_| HttpResponse::Ok()))
+        })
+    });
+
+    let request = srv.get().uri(srv.url("/scope/test")).finish().unwrap();
+    let response = srv.execute(request.send()).unwrap();
+
+    assert_eq!(num1.load(Ordering::Relaxed), 1);
+    assert_eq!(num2.load(Ordering::Relaxed), 1);
+    assert_eq!(num3.load(Ordering::Relaxed), 1);
+}
+
+#[test]
+fn test_scope_middleware_async_chain_with_error() {
+    let num1 = Arc::new(AtomicUsize::new(0));
+    let num2 = Arc::new(AtomicUsize::new(0));
+    let num3 = Arc::new(AtomicUsize::new(0));
+
+    let act_num1 = Arc::clone(&num1);
+    let act_num2 = Arc::clone(&num2);
+    let act_num3 = Arc::clone(&num3);
+
+    let mut srv = test::TestServer::with_factory(move || {
+        let mw1 = MiddlewareTest {
+            start: Arc::clone(&act_num1),
+            response: Arc::clone(&act_num2),
+            finish: Arc::clone(&act_num3),
+        };
+        App::new().scope("/scope", |scope| {
+            scope
+                .middleware(mw1)
+                .middleware(MiddlewareAsyncWithErr)
+                .resource("/test", |r| r.h(|_| HttpResponse::Ok()))
+        })
+    });
+
+    let request = srv.get().uri(srv.url("/scope/test")).finish().unwrap();
+    let response = srv.execute(request.send()).unwrap();
+
+    assert_eq!(num1.load(Ordering::Relaxed), 1);
+    assert_eq!(num2.load(Ordering::Relaxed), 1);
+    assert_eq!(num3.load(Ordering::Relaxed), 1);
+}
+
+#[test]
+fn test_resource_middleware_chain_with_error() {
+    let num1 = Arc::new(AtomicUsize::new(0));
+    let num2 = Arc::new(AtomicUsize::new(0));
+    let num3 = Arc::new(AtomicUsize::new(0));
+
+    let act_num1 = Arc::clone(&num1);
+    let act_num2 = Arc::clone(&num2);
+    let act_num3 = Arc::clone(&num3);
+
+    let mut srv = test::TestServer::with_factory(move || {
+        let mw1 = MiddlewareTest {
+            start: Arc::clone(&act_num1),
+            response: Arc::clone(&act_num2),
+            finish: Arc::clone(&act_num3),
+        };
+        App::new().resource("/test", move |r| {
+            r.middleware(mw1);
+            r.middleware(MiddlewareWithErr);
+            r.h(|_| HttpResponse::Ok());
+        })
+    });
+
+    let request = srv.get().uri(srv.url("/test")).finish().unwrap();
+    let response = srv.execute(request.send()).unwrap();
+
+    assert_eq!(num1.load(Ordering::Relaxed), 1);
+    assert_eq!(num2.load(Ordering::Relaxed), 1);
+    assert_eq!(num3.load(Ordering::Relaxed), 1);
+}
+
+#[test]
+fn test_resource_middleware_async_chain_with_error() {
+    let num1 = Arc::new(AtomicUsize::new(0));
+    let num2 = Arc::new(AtomicUsize::new(0));
+    let num3 = Arc::new(AtomicUsize::new(0));
+
+    let act_num1 = Arc::clone(&num1);
+    let act_num2 = Arc::clone(&num2);
+    let act_num3 = Arc::clone(&num3);
+
+    let mut srv = test::TestServer::with_factory(move || {
+        let mw1 = MiddlewareTest {
+            start: Arc::clone(&act_num1),
+            response: Arc::clone(&act_num2),
+            finish: Arc::clone(&act_num3),
+        };
+        App::new().resource("/test", move |r| {
+            r.middleware(mw1);
+            r.middleware(MiddlewareAsyncWithErr);
+            r.h(|_| HttpResponse::Ok());
+        })
+    });
+
+    let request = srv.get().uri(srv.url("/test")).finish().unwrap();
+    let response = srv.execute(request.send()).unwrap();
+
+    assert_eq!(num1.load(Ordering::Relaxed), 1);
+    assert_eq!(num2.load(Ordering::Relaxed), 1);
+    assert_eq!(num3.load(Ordering::Relaxed), 1);
+}