Add support for catching handlers that panic

README.md

@@ -10,9 +10,14 @@ stack.
 If the application handler returns an `Err(_)` the server will log the
 description via the `log` crate and then return a generic 500 status response.
 
-If the handler panics, the default panic handler prints a message to stderr and the
-connnection is closed without sending a response.  In the future, these panics
-will likely be turned into a generic 500 status response.
+If the handler panics, the default panic handler prints a message to stderr,
+the handler is unwound, an error is logged via `log`, and a generic 500 status
+response is returned.
+
+Handlers that rely on interior mutability (such as with a `Mutex`) must be
+prepared to deal with possibly inconsistent state (such as a poisoned `Mutex`)
+if a previous call has paniced.  It is unlikely that many handlers have such
+shared state between requests.
 
 ## Request Processing
 

src/lib.rs

@@ -14,6 +14,7 @@ mod tests;
 
 use std::io::{Cursor, Read};
 use std::net::SocketAddr;
+use std::panic::{catch_unwind, AssertUnwindSafe};
 use std::sync::Arc;
 
 use futures::{future, Future, Stream};
@@ -208,17 +209,24 @@ impl<H: conduit::Handler> hyper::service::Service for Service<H> {
     /// Returns a future which buffers the response body and then calls the conduit handler from a thread pool
     fn call(&mut self, request: Request<Self::ReqBody>) -> Self::Future {
         let pool = self.pool.clone();
-        let handler = self.handler.clone();
+        // Handlers that have interior mutability between requests are unlikely.  Any hanlders that
+        // do must be prepared to handle poisoned mutexes.
+        let handler = AssertUnwindSafe(self.handler.clone());
 
         let (parts, body) = request.into_parts();
+
         let response = body.concat2().and_then(move |full_body| {
             pool.spawn_fn(move || {
-                let mut request = ConduitRequest::new(Parts(parts), full_body);
-                let response = handler
-                    .call(&mut request)
-                    .map(good_response)
-                    .unwrap_or_else(|e| error_response(e.description()));
-
+                // If the handler panics, the request is dropped and potentially inconsistent data
+                // is not observable.
+                let mut request = AssertUnwindSafe(ConduitRequest::new(Parts(parts), full_body));
+                let response =
+                    catch_unwind(move || {
+                        handler
+                            .call(&mut *request)
+                            .map(good_response)
+                            .unwrap_or_else(|e| error_response(e.description()))
+                    }).unwrap_or_else(|_| error_response("Application handler paniced"));
                 Ok(response)
             })
         });

src/tests.rs

@@ -1,6 +1,7 @@
 use std::collections::HashMap;
 use std::error::Error;
 use std::io::Cursor;
+use std::sync::Mutex;
 
 use conduit::{Handler, Request, Response};
 use futures::{Future, Stream};
@@ -25,10 +26,17 @@ impl Handler for ErrorResult {
     }
 }
 
-struct Panic;
-impl Handler for Panic {
-    fn call(&self, _req: &mut Request) -> Result<Response, Box<Error + Send>> {
-        panic!()
+struct PanicOnce(Mutex<()>);
+impl Handler for PanicOnce {
+    fn call(&self, req: &mut Request) -> Result<Response, Box<Error + Send>> {
+        // A previous panic may have caused inconsistent state.
+        // Return a normal result as a flag to the test code.
+        if self.0.is_poisoned() {
+            return OkResult.call(req);
+        };
+
+        let _dont_drop = self.0.lock();
+        panic!();
     }
 }
 
@@ -101,8 +109,19 @@ fn err_responses() {
     assert_generic_err(simulate_request(ErrorResult));
 }
 
-#[ignore] // catch_unwind not yet implemented
 #[test]
 fn recover_from_panic() {
-    assert_generic_err(simulate_request(Panic));
+    use hyper::service::{NewService, Service};
+
+    let handler = PanicOnce(Mutex::default());
+    let new_service = super::Service::new(handler, 1);
+    let mut service = new_service.new_service().wait().unwrap();
+
+    // First call panics and is turned into a generic error
+    let response = service.call(hyper::Request::default()).wait().unwrap();
+    assert_generic_err(response);
+
+    // Second call returns status 200 to indicate a poisoned mutex
+    let response = service.call(hyper::Request::default()).wait().unwrap();
+    assert_eq!(response.status(), 200);
 }