Stiffstream / restinio

Cross-platform, efficient, customizable, and robust asynchronous HTTP(S)/WebSocket server C++ library with the right balance between performance and ease of use
Other
1.15k stars 93 forks source link

Dump gathering for unhandled exception. #121

Open SRyabinin opened 4 years ago

SRyabinin commented 4 years ago

Hello Eugeny, thx for your lib.

We have to save dumps for unhandled exception, which can be throwing in request handlers. But you catch all std::exceptions in connection_t::on_request_message_complete() method. Unfortunately log from trigger_error_and_close not enough to understand the reason and location of exception. Rethrowing exceptions from catch is useless in Windows x64. The stack of exception is lost in case of throw; call in x64 platform (see https://social.msdn.microsoft.com/Forums/vstudio/en-us/f4ccad43-4fb4-4760-9d48-b4c883c2e550/call-stack-throw-and-64-bit). So I have a plan to delete catch(std::exception&) from connection_t::on_request_message_complete() but don't know what to do with close in catch section of connection_t::on_request_message_complete(). May be the better way is to catch only asio network exceptions in connection_t::on_request_message_complete()

Could you advise some solution to deliver unhandled exceptions from request handler to dump collector?

eao197 commented 4 years ago

I think there are several possible choices:

1. You can wrap your request handlers in some wrapper. Something like:

restinio::default_request_handler_t wrap_handler(restinio::default_request_handler_t handler) {
   return [handler](restinio::request_handle_t req) -> restinio::request_handling_status_t {
      try {
         return handler(std::move(req));
      }
      catch(...) {
         ... // Do any things you want.
         throw;
      }
   }
}
...
restinio::run( restinio::on_this_thread()
  .address().port(...)
  .request_handler( wrap_handler(your_actual_handler) ) );

In that case, you have no need to patch RESTinio's code.

Note that you can use any type of request-handler you want. RESTinio's default_request_handler_t is shown here just for an example.

2. You can patch RESTinio's code and add your own actions here. But you have to call connection_t::close() in your implementation of catch block.

3. I think that version 0.6.9 is almost complete and an additional feature can be added to 0.6.9 before the release. That feature could be a custom handler for exceptions from request handlers. Something like:

class my_unhandled_exception_processor {
...
public:
   void handle() noexcept {
      ... // Do what you want.
   }
};
...
struct my_traits : public restinio::default_traits_t {
   using unhandled_exception_processor_t = my_unhandled_exception_processor;
};

restinio::run( restinio::on_this_thread<my_traits>()
  .address().port(...)
  .unhandled_exception_processor(
     std::make_shared<my_unhandled_exception_processor>(...))
  ...);

In that case, an instance of my_unhandled_exception_processor will be called inside connection_t if a request-handler throws an exception.

deadem commented 4 years ago

There is a problem:

  1. We can't use throw; due to MS-specific behaviour on 64-bit systems: https://social.msdn.microsoft.com/Forums/vstudio/en-us/f4ccad43-4fb4-4760-9d48-b4c883c2e550/call-stack-throw-and-64-bit There are stack loss on throw;
  2. Patch will be OK, but we want catch there only restinio-related exceptions (asio, etc), passthrough any others. How can we do it?
  3. We're stick on 0.4.2 unfortunately, due to VS2015 ;(
eao197 commented 4 years ago

We can't use throw; due to MS-specific behaviour on 64-bit systems: https://social.msdn.microsoft.com/Forums/vstudio/en-us/f4ccad43-4fb4-4760-9d48-b4c883c2e550/call-stack-throw-and-64-bit There are stack loss on throw;

I think you speak about losing the stack on rethrowing an exception. The way number 1 is about wrapping actual request handler into your wrapper where you can catch any exception and do dumping you want. After that, you just rethrow the exception (it is necessary for RESTinio to close the connection).

Can you provide some info about the way of dumping exceptions you use? Maybe it makes the picture more clear for me.

but we want catch there only restinio-related exceptions (asio, etc), passthrough any others.

We catch std::exception in on_request_message_complete because there can be any exceptions, most of them do not relate to RESTinio nor Asio, but are specific to application logic in request-handler. For example, bad::alloc can be throw from request-handler if a user tries to allocate to many memory.

But if you have to patch RESTinio you can write any catch sections you want:

catch( const restinio::exception_t & ) {...}
catch( const asio::system_error & ) {...}
catch(...) {...}
SRyabinin commented 4 years ago

We can't use throw; due to MS-specific behaviour on 64-bit systems: https://social.msdn.microsoft.com/Forums/vstudio/en-us/f4ccad43-4fb4-4760-9d48-b4c883c2e550/call-stack-throw-and-64-bit There are stack loss on throw;

I think you speak about losing the stack on rethrowing an exception. The way number 1 is about wrapping actual request handler into your wrapper where you can catch any exception and do dumping you want. After that, you just rethrow the exception (it is necessary for RESTinio to close the connection).

I thought about this technic, but it looks like hack and requires modification of our dump collection infrastructure. MS api (dbghelp.dll) writes c++ exception by it EXCEPTION_POINTERS. EXCEPTION_POINTERS avalables only in unhandled exception filters (see SetUnhandledExceptionFilter). I don't know a way to convert std::exception to EXCEPTION_POINTERS in catch section;

Can you provide some info about the way of dumping exceptions you use? Maybe it makes the picture more clear for me.

We use SetUnhandledExceptionFilter _set_se_translator for catch unhandled exceptions and dbghelp.dll for saving dumps.

But if you have to patch RESTinio you can write any catch sections you want:

catch( const restinio::exception_t & ) {...}
catch( const asio::system_error & ) {...}
catch(...) {...}

Well I suppose the patch will be OK. Thank you. Memory allocating exceptions can set data to an undefined state. In this case, we prefer to terminate program with dump .

prince-chrismc commented 4 years ago

From my_unhandled_exception_processor would it be possible to return a response? Currently the connection is closed after a 500 response is returned with no body. Being able to return API specific response would help with system integration to debug the problem.

Regardless the third option would be great to help catch developer mistakes! :+1: