Catch panics that occur before future is returned.

In the course of resolving this issue, double-boxing of handlers was
discovered and removed.

core/lib/src/catcher.rs

@@ -250,13 +250,21 @@ pub trait ErrorHandler: Cloneable + Send + Sync + 'static {
     async fn handle<'r, 's: 'r>(&'s self, status: Status, req: &'r Request<'_>) -> Result<'r>;
 }
 
-#[crate::async_trait]
+// We write this manually to avoid double-boxing.
 impl<F: Clone + Sync + Send + 'static> ErrorHandler for F
-    where for<'x> F: Fn(Status, &'x Request<'_>) -> ErrorHandlerFuture<'x>
+    where for<'x> F: Fn(Status, &'x Request<'_>) -> ErrorHandlerFuture<'x>,
 {
-    #[inline(always)]
-    async fn handle<'r, 's: 'r>(&'s self, status: Status, req: &'r Request<'_>) -> Result<'r> {
-        self(status, req).await
+    fn handle<'r, 's: 'r, 'life0, 'async_trait>(
+        &'s self,
+        status: Status,
+        req: &'r Request<'life0>,
+    ) -> ErrorHandlerFuture<'r>
+        where 'r: 'async_trait,
+              's: 'async_trait,
+              'life0: 'async_trait,
+              Self: 'async_trait,
+    {
+        self(status, req)
     }
 }
 

core/lib/src/handler.rs

@@ -151,13 +151,22 @@ pub trait Handler: Cloneable + Send + Sync + 'static {
     async fn handle<'r, 's: 'r>(&'s self, request: &'r Request<'_>, data: Data) -> Outcome<'r>;
 }
 
-#[crate::async_trait]
+// We write this manually to avoid double-boxing.
 impl<F: Clone + Sync + Send + 'static> Handler for F
-    where for<'x> F: Fn(&'x Request<'_>, Data) -> HandlerFuture<'x>
+    where for<'x> F: Fn(&'x Request<'_>, Data) -> HandlerFuture<'x>,
 {
     #[inline(always)]
-    async fn handle<'r, 's: 'r>(&'s self, req: &'r Request<'_>, data: Data) -> Outcome<'r> {
-        self(req, data).await
+    fn handle<'r, 's: 'r, 'life0, 'async_trait>(
+        &'s self,
+        req: &'r Request<'life0>,
+        data: Data,
+    ) -> HandlerFuture<'r>
+        where 'r: 'async_trait,
+              's: 'async_trait,
+              'life0: 'async_trait,
+              Self: 'async_trait,
+    {
+        self(req, data)
     }
 }
 

core/lib/src/server.rs

@@ -1,9 +1,8 @@
 use std::io;
 use std::sync::Arc;
-use std::panic::AssertUnwindSafe;
 
 use futures::stream::StreamExt;
-use futures::future::FutureExt;
+use futures::future::{FutureExt, Future};
 use tokio::sync::oneshot;
 use yansi::Paint;
 
@@ -22,18 +21,40 @@ use crate::http::uri::Origin;
 // A token returned to force the execution of one method before another.
 pub(crate) struct Token;
 
-macro_rules! info_panic {
-    ($e:expr) => {{
-        error_!("A handler has panicked. This is an application bug.");
-        info_!("A panic in Rust must be treated as an exceptional event.");
-        info_!("Panicking is not a suitable error handling mechanism.");
-        info_!("Unwinding, the result of a panic, is an expensive operation.");
-        info_!("Panics will severely degrade application performance.");
-        info_!("Instead of panicking, return `Option` and/or `Result`.");
-        info_!("Values of either type can be returned directly from handlers.");
-        warn_!("Forwarding to the {} error catcher.", Paint::blue(500).bold());
-        $e
-    }}
+async fn handle<Fut, T, F>(name: Option<&str>, run: F) -> Option<T>
+    where F: FnOnce() -> Fut, Fut: Future<Output = T>,
+{
+    use std::panic::AssertUnwindSafe;
+
+    macro_rules! panic_info {
+        ($name:expr, $e:expr) => {{
+            match $name {
+                Some(name) => error_!("Handler {} panicked.", Paint::white(name)),
+                None => error_!("A handler panicked.")
+            };
+
+            info_!("This is an application bug.");
+            info_!("A panic in Rust must be treated as an exceptional event.");
+            info_!("Panicking is not a suitable error handling mechanism.");
+            info_!("Unwinding, the result of a panic, is an expensive operation.");
+            info_!("Panics will severely degrade application performance.");
+            info_!("Instead of panicking, return `Option` and/or `Result`.");
+            info_!("Values of either type can be returned directly from handlers.");
+            warn_!("A panic is treated as an internal server error.");
+            $e
+        }}
+    }
+
+    let run = AssertUnwindSafe(run);
+    let fut = std::panic::catch_unwind(move || run())
+        .map_err(|e| panic_info!(name, e))
+        .ok()?;
+
+    AssertUnwindSafe(fut)
+        .catch_unwind()
+        .await
+        .map_err(|e| panic_info!(name, e))
+        .ok()
 }
 
 // This function tries to hide all of the Hyper-ness from Rocket. It essentially
@@ -271,10 +292,9 @@ impl Rocket {
             info_!("Matched: {}", route);
             request.set_route(route);
 
-            // Dispatch the request to the handler.
-            let outcome = AssertUnwindSafe(route.handler.handle(request, data))
-                .catch_unwind().await
-                .unwrap_or_else(|_| info_panic!(Outcome::Failure(Status::InternalServerError)));
+            let name = route.name.as_deref();
+            let outcome = handle(name, || route.handler.handle(request, data)).await
+                .unwrap_or_else(|| Outcome::Failure(Status::InternalServerError));
 
             // Check if the request processing completed (Some) or if the
             // request needs to be forwarded. If it does, continue the loop
@@ -316,10 +336,9 @@ impl Rocket {
 
         if let Some(catcher) = catcher {
             warn_!("Responding with registered {} catcher.", catcher);
-            let handler = AssertUnwindSafe(catcher.handler.handle(status, req));
-            handler.catch_unwind().await
+            handle(None, || catcher.handler.handle(status, req)).await
                 .map(|result| result.map_err(Some))
-                .unwrap_or_else(|_| info_panic!(Err(None)))
+                .unwrap_or_else(|| Err(None))
         } else {
             let code = Paint::blue(status.code).bold();
             warn_!("No {} catcher registered. Using Rocket default.", code);

core/lib/tests/panic-handling.rs

@@ -1,8 +1,11 @@
 #[macro_use] extern crate rocket;
 
-use rocket::Rocket;
-use rocket::http::Status;
+use rocket::{Rocket, Route, Request};
+use rocket::data::Data;
+use rocket::http::{Method, Status};
 use rocket::local::blocking::Client;
+use rocket::catcher::{Catcher, ErrorHandlerFuture};
+use rocket::handler::HandlerFuture;
 
 #[get("/panic")]
 fn panic_route() -> &'static str {
@@ -24,9 +27,19 @@ fn double_panic() {
     panic!("so, so sorry...")
 }
 
+fn pre_future_route<'r>(_: &'r Request<'_>, _: Data) -> HandlerFuture<'r> {
+    panic!("hey now...");
+}
+
+fn pre_future_catcher<'r>(_: Status, _: &'r Request) -> ErrorHandlerFuture<'r> {
+    panic!("a panicking pre-future catcher")
+}
+
 fn rocket() -> Rocket {
+    let pre_future_panic = Route::new(Method::Get, "/pre", pre_future_route);
     rocket::ignite()
         .mount("/", routes![panic_route])
+        .mount("/", vec![pre_future_panic])
         .register(catchers![panic_catcher, ise])
 }
 
@@ -36,8 +49,8 @@ fn catches_route_panic() {
     let response = client.get("/panic").dispatch();
     assert_eq!(response.status(), Status::InternalServerError);
     assert_eq!(response.into_string().unwrap(), "Hey, sorry! :(");
-
 }
+
 #[test]
 fn catches_catcher_panic() {
     let client = Client::debug(rocket()).unwrap();
@@ -52,5 +65,23 @@ fn catches_double_panic() {
     let client = Client::debug(rocket).unwrap();
     let response = client.get("/noroute").dispatch();
     assert_eq!(response.status(), Status::InternalServerError);
-    assert!(!response.into_string().unwrap().contains(":("));
+    assert!(response.into_string().unwrap().contains("Rocket"));
+}
+
+#[test]
+fn catches_early_route_panic() {
+    let client = Client::debug(rocket()).unwrap();
+    let response = client.get("/pre").dispatch();
+    assert_eq!(response.status(), Status::InternalServerError);
+    assert_eq!(response.into_string().unwrap(), "Hey, sorry! :(");
+}
+
+#[test]
+fn catches_early_catcher_panic() {
+    let panic_catcher = Catcher::new(404, pre_future_catcher);
+
+    let client = Client::debug(rocket().register(vec![panic_catcher])).unwrap();
+    let response = client.get("/idontexist").dispatch();
+    assert_eq!(response.status(), Status::InternalServerError);
+    assert_eq!(response.into_string().unwrap(), "Hey, sorry! :(");
 }