How should one log when an exception is triggered?

Question

In a program I recently wrote, I wanted to log when my "business logic" code triggered an exception in third-party or project APIs. ( To clarify, I want to log when use of an an API causes an exception. This can be many frames above the actual throw, and may be many frames below the actual catch ( where logging of the exception payload can occur. ) ) I did the following:

void former_function()
{
    /* some code here */
    try
    {
       /* some specific code that I know may throw, and want to log about */
    }
    catch( ... )
    {
       log( "an exception occurred when doing something with some other data" );
       throw;
    }
    /* some code here */
}

In short, if an exception occurs, create a catch-all clause, log the error, and re-throw. In my mind this is safe. I know in general catch-all is considered bad, since one doesn't have a reference to the exception at all to get any useful information. However, I'm just going to re-throw it, so nothing is lost.

Now, on its own it was fine, but some other programmers modified this program, and ended up violating the above. Specifically, they put a huge amount of code into the try-block in one case, and in another removed the 'throw' and placed a 'return'.

I see now my solution was brittle; it wasn't future-modification-proof.

I want a better solution that does not have these problems.

I have another potential solution that doesn't have the above issue, but I wonder what others think of it. It uses RAII, specifically a "Scoped Exit" object that implicitly triggers if std::uncaught_exception is not true on construction, yet is true on destruction:

#include <ciso646> // not, and
#include <exception> // uncaught_exception

class ExceptionTriggeredLog
{
private:
    std::string const m_log_message;
    bool const m_was_uncaught_exception;
public:
    ExceptionTriggeredLog( std::string const& r_log_message )
      : m_log_message( r_log_message ),
        m_was_uncaught_exception( std::uncaught_exception() )
    {
    }
    ~ExceptionTriggeredLog()
    {
        if( not m_was_uncaught_exception
            and std::uncaught_exception() )
        {
            try
            {
                log( m_log_message );
            }
            catch( ... )
            {
                // no exceptions can leave an destructor.
                // especially when std::uncaught_exception is true.
            }
        }
    }
};

void potential_function()
{
    /* some code here */
    {
       ExceptionTriggeredLog exception_triggered_log( "an exception occurred when doing something with some other data" );
       /* some specific code that I know may throw, and want to log about */
    }
    /* some code here */
}

I want to know:

• technically, would this work robustly? Initially it seems to work, but I know there are some caveats about using std::uncaught_exception.
• is there another way to accomplish what I want?

Note: I've updated this question. Specifically, I've:

• added the try/catch that was initially missing, around the log function-call.
• added tracking the std::uncaught_exception state on construction. This guards against the case where this object is created inside a 'try' block of another destructor which is triggered as part of exception stack-unwinding.
• fixed the new 'potential_function' to create a named object, not a temporary object as before.

Tinsel

You probably know this, but you will need to either declare log as noexcept or throw() or wrap it inside try { and } catch(...). – Pillage

@Pillage Thanks for the reminder; in my simple example here it's throw()/noexcept, but in general it won't be, and needs to have the try/catch. – Tinsel

What stops your fellow programmers from putting the "huge amount of code" outside the braces? Or remove them because they seem to be unnecessary? I would try adding a comment Please don't replace this throw with a return. That doesn't work!. – Sonstrom

@BoPersson If I thought I were in a position to change others' behaviour, I would have pursued that. But I'm not in this circumstance, and in general, one cannot rely on this. Thus, I want to change my approach. Using an RAII seems cleaner, and it's much more difficult to pervert. – Tinsel

@BoPersson I'd wrap the statement you want to protect inside a macro so they can't muck with the braces, the braces being part of the code emitted by the macro. – Wherefore

I'm a bit curious about this subject myself. I would be very interested to see what are the pros and cons of the approach I've outlined below (using a lambda.) Any ideas? – Terriss

@YaserZhian The only immediate benefit of my approach is that it's usage could be for a large stanza of code, whereas the lambda approach would get uglier as you add more statements. However, my original premise was to log about triggering exceptions on specific API calls, so this point may be moot. – Tinsel

@YaserZhian Still, my RAII solution may be a better general solution, as it can be applied to my original situation, as well as other needs. – Tinsel

@CharlesLWilcox: On the other hand, this particular use of RAII depends on a library call (std::uncaught_exception()) which is a dependency (I come from game development, so even dependency on the STL is still a dependency on foreign code!) but I'm the first one to admit that it's not a big deal. – Terriss

@CharlesLWilcox: Also, the above method uses a bit more memory (for the bool!) Which again, is absolutely no big deal in most (all?) situations. And obviously it needs more code. But I fail to see how using a lambda will "get uglier as you add more statements." It seems to me like the usage of both methods are written almost exactly the same: a block of arbitrary code, indented one tab. But the lambda approach does depend on the compiler to inline the try_and_log function, which again is not at an extraordinary thing to expect. – Terriss

@CharlesLWilcox: By the way, I think in your sample usage code, you cannot use an anonymous object for the ExceptionTriggeredLog instance. I might be wrong but doesn't your object gets destroyed at the same line it was created? By the way, I think wrapping the above version in macros needs more care (because of the braces involved) and still more code for the macros. – Terriss

@YaserZhian Regarding the 'potential_function' example, you're correct, I had a temporary object; I changed it to a named object. – Tinsel

@YaserZhian Yes, this depends both upon more memory, ( the bool, ) and upon #include <exception> and the symbol std::uncaught_exception. Given that ExceptionTriggeredLog is intended to be used by-value in a small scope, and given that the methods would be implicitly inline, I assume any compiler would optimizate the bool away. However, std::uncaught_exception is called once for the "successful" path, and twice for the "error" path; I'm unsure if this function is inline-able, if this interacts with the ( somewhat slower ) exception-logic, etc. So, that is a good critique. – Tinsel

@YaserZhian By "get uglier as you add more statements", comparing the two approaches, the proposed 'potential_function' just has the extra object ExceptionTriggeredLog, then the normal logic. The proposed 'try_and_log' solution adds the normal code inside as an argument; I find this.... a bit awkward. I'm familiar with functors, but don't use them regularly. Perhaps as lambda become more wide-spread, due to their convenience, this would not be an issue for me. – Tinsel

@CharlesLWilcox: (Thanks for the correct vernacular, by the way!) All in all, I guess the difference and preference between these two approaches come down to coding style, case-by-case requirements and personal taste. There aren't any substantial advantages or disadvantages to either approach as far as I can see. And thanks for the discussion. – Terriss

Answer 1

I have no comment on your method, but it seems interesting! I have another way that might also work for what you want, and might be a little more general-purpose. It requires lambdas from C++11 though, which might or might not be an issue in your case.

It's a simple function template that accepts a lambda, runs it and catches, logs and rethrows all exceptions:

template <typename F>
void try_and_log (char const * log_message, F code_block)
{
    try {
        code_block ();
    } catch (...) {
        log (log_message);
        throw;
    }
}

The way you use it (in the simplest case) is like this:

try_and_log ("An exception was thrown here...", [&] {
    this_is_the_code ();
    you_want_executed ();
    and_its_exceptions_logged ();
});

As I said before, I don't know how it stacks against your own solution. Note that the lambda is capturing everything from its enclosing scope, which is quite convenient. Also note that I haven't actually tried this, so compile errors, logical problems and/or nuclear wars may result from this.

The problem I see here is that it is not easy to wrap this into a macro, and expecting your colleagues to write the [=] { and } parts correctly and all the time might be too much!

For wrapping and idiot-proofing purposes, you'll probably need two macros: a TRY_AND_LOG_BEGIN to emit the first line till the opening brace for the lambda and an TRY_AND_LOG_END to emit the closing brace and parenthesis. Like so:

#define TRY_AND_LOG_BEGIN(message)  try_and_log (message, [&] {
#define TRY_AND_LOG_END()           })

And you use them like this:

TRY_AND_LOG_BEGIN ("Exception happened!")  // No semicolons here!
    whatever_code_you_want ();
TRY_AND_LOG_END ();

Which is - depending on your perspective - is either a net gain or a net loss! (I personally prefer the straightforward function call and lambda syntax, which gives me more control and transparency.

Also, it is possible to write the log message at the end of the code block; just switch the two parameters of the try_and_log function.

Terriss

I'm unsure the MACROS add any value compared to the raw try_and_log; in both cases another coder could easily add new statements. Compared to my original 'former_function' example, this hides the underlying try/catch(...){log("bad stuff");throw;} so that the re-throw is unaltered, and the catch(...) is not copy-pasted into other locations in the code, code that shouldn't be catching all exceptions. – Tinsel