Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dump gathering for unhandled exception. #121

Open
SRyabinin opened this issue Aug 6, 2020 · 5 comments
Open

Dump gathering for unhandled exception. #121

SRyabinin opened this issue Aug 6, 2020 · 5 comments

Comments

@SRyabinin
Copy link

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
Copy link
Member

eao197 commented Aug 6, 2020

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
Copy link

deadem commented Aug 6, 2020

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
Copy link
Member

eao197 commented Aug 6, 2020

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
Copy link
Author

SRyabinin commented Aug 6, 2020

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
Copy link
Contributor

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! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants