Fix HBPVIS#224: Handle exceptions in server handlers

doc/Changelog.md

@@ -2,6 +2,8 @@
 
 # git master
 
+* [225](https://github.com/HBPVIS/ZeroEQ/pull/225):
+  Fix #224: Handle exceptions in server handlers
 * [219](https://github.com/HBPVIS/ZeroEQ/pull/219):
   Implement Client-Server req-rep support
   * The environment variables ZEROEQ_PUB_SESSION and ZEROEQ_SERVER_SESSION

tests/common.h

@@ -67,7 +67,7 @@ class Echo : public servus::Serializable
         return true;
     }
 
-    Data _toBinary() const final
+    Data _toBinary() const override
     {
         Data data;
         data.ptr =

tests/http/server.cpp

@@ -79,6 +79,10 @@ const Response response204{ServerReponse::no_content, ""};
 const Response error400{ServerReponse::bad_request, ""};
 const Response error404{ServerReponse::not_found, ""};
 const Response error405{ServerReponse::method_not_allowed, ""};
+Response error500(const std::string& body)
+{
+    return Response(ServerReponse::internal_server_error, body);
+}
 
 const std::string jsonGet("Not JSON, just want to see that the is data a-ok");
 const std::string jsonPut("See what my stepbrother jsonGet says");
@@ -977,6 +981,30 @@ BOOST_AUTO_TEST_CASE(issue157)
     thread.join();
 }
 
+BOOST_AUTO_TEST_CASE(issue224) // robustness when handlers throw
+{
+    bool running = true;
+    zeroeq::http::Server server;
+
+    server.handleGET("test/foo", [&]() {
+        throw std::runtime_error("I've had enough!");
+        return jsonGet;
+    });
+
+    std::thread thread([&]() {
+        while (running)
+            server.receive(TIMEOUT);
+    });
+
+    Client client(server.getURI());
+    client.checkGET("/test/foo",
+                    error500("Request handler exception: I've had enough!"),
+                    __LINE__);
+
+    running = false;
+    thread.join();
+}
+
 BOOST_AUTO_TEST_CASE(urlcasesensitivity)
 {
     bool running = true;

tests/reqRep.cpp

@@ -63,6 +63,22 @@ bool runOnce(zeroeq::Server& server, const test::Echo& request, const R& reply)
 
     return handled;
 }
+
+class EchoThrows : public test::Echo
+{
+public:
+    explicit EchoThrows(const std::string& message)
+        : Echo(message)
+    {
+    }
+
+private:
+    Data _toBinary() const final
+    {
+        throw std::runtime_error("I've had enough!");
+        return Data();
+    }
+};
 }
 
 BOOST_AUTO_TEST_CASE(serializable)
@@ -141,6 +157,36 @@ BOOST_AUTO_TEST_CASE(empty_request_raw)
     BOOST_CHECK(serverHandled);
 }
 
+BOOST_AUTO_TEST_CASE(issue224)
+{
+    test::Echo echo("The quick brown fox");
+    const EchoThrows reply("Jumped over the lazy dog");
+
+    zeroeq::Server server(zeroeq::NULL_SESSION);
+    zeroeq::Client client({server.getURI()});
+
+    bool serverHandled = false;
+    std::thread thread([&] { serverHandled = runOnce(server, echo, reply); });
+
+    bool handled = false;
+    client.request(echo, [&](const zeroeq::uint128_t& type, const void* data,
+                             const size_t size) {
+        BOOST_CHECK_EQUAL(type, 0);
+        BOOST_CHECK(!data);
+        BOOST_CHECK_EQUAL(size, 0);
+        BOOST_CHECK(!handled);
+
+        handled = true;
+    });
+
+    BOOST_CHECK(!handled);
+    BOOST_CHECK(client.receive(TIMEOUT));
+    BOOST_CHECK(handled);
+
+    thread.join();
+    BOOST_CHECK(serverHandled);
+}
+
 BOOST_AUTO_TEST_CASE(empty_request_object)
 {
     zeroeq::Server server(zeroeq::URI("inproc://zeroeq.test.empty_request_raw"),

zeroeq/client.h

@@ -106,6 +106,10 @@ class Client : public Receiver
      * The reply function will be executed during receive(). May block when
      * all servers are overloaded or no server is connected.
      *
+     * The reply function will get called with (0, nullptr, 0) if the server
+     * does not have a handler for the request or if the handler had an
+     * exception.
+     *
      * @param request the request identifier and payload
      * @param func the function to execute for the reply
      * @return true if the request was sent, false on error
@@ -116,8 +120,7 @@ class Client : public Receiver
     /**
      * Request the execution of the given data on a connected Server.
      *
-     * The reply function will be executed during receive(). May block of when
-     * all servers are overloaded or no server is connected.
+     * See request() overload above for details.
      *
      * @param request the request identifier
      * @param data the payload data of the request, may be nullptr

zeroeq/http/server.cpp

@@ -659,7 +659,23 @@ bool Server::process(detail::Socket&)
     }
     else
     {
-        message->response = respondTo(message->request);
+        try
+        {
+            message->response = respondTo(message->request);
+        }
+        catch (const std::exception& e)
+        {
+            message->response =
+                make_ready_response(Code::INTERNAL_SERVER_ERROR,
+                                    std::string("Request handler exception: ") +
+                                        e.what());
+        }
+        catch (...)
+        {
+            message->response =
+                make_ready_response(Code::INTERNAL_SERVER_ERROR,
+                                    "An unknown exception occured");
+        }
 
         // When a client makes a CORS request (by setting an 'Origin' header) it
         // expects an 'Access-Control-Allow-Origin' response header. See:

zeroeq/server.cpp

@@ -77,22 +77,27 @@ class Server::Impl : public detail::Sender
         }
         else
         {
-            auto reply = payload
-                             ? i->second(zmq_msg_data(&msg), zmq_msg_size(&msg))
-                             : i->second(nullptr, 0);
-            const bool hasReplyData = reply.second.ptr && reply.second.size;
+            try
+            {
+                auto reply =
+                    payload ? i->second(zmq_msg_data(&msg), zmq_msg_size(&msg))
+                            : i->second(nullptr, 0);
+                const bool hasReplyData = reply.second.ptr && reply.second.size;
 #ifdef ZEROEQ_BIGENDIAN
-            detail::byteswap(reply.first); // convert to little endian
+                detail::byteswap(reply.first); // convert to little endian
 #endif
-            if (!_send(&reply.first, sizeof(reply.first),
-                       hasReplyData ? ZMQ_SNDMORE : 0))
+                if (_send(&reply.first, sizeof(reply.first),
+                          hasReplyData ? ZMQ_SNDMORE : 0) &&
+                    hasReplyData)
+                {
+                    _send(reply.second.ptr.get(), reply.second.size, 0);
+                }
+            }
+            catch (...) // handler had exception
             {
-                if (payload)
-                    zmq_msg_close(&msg);
-                return true;
+                const uint128_t zero;
+                _send(&zero, sizeof(zero), 0); // request and reply, no playload
             }
-            if (hasReplyData)
-                _send(reply.second.ptr.get(), reply.second.size, 0);
         }
 
         if (payload)

zeroeq/server.h

@@ -151,6 +151,9 @@ class Server : public Receiver, public Sender
     /**
      * Register a request handler.
      *
+     * Exceptions in a request handler are considered an error (0 is returned to
+     * client).
+     *
      * @param request the request to handle
      * @param func the function to call on receive() of a Client::request()
      * @return true if subscription was successful, false otherwise