Is `catch(...) { throw; }` a bad practice?

Question

While I agree that catching ... without rethrowing is indeed wrong, I however believe that using constructs like this:

try
{
  // Stuff
}
catch (...)
{
  // Some cleanup
  throw;
}

Is acceptable in cases where RAII is not applicable. (Please, don't ask... not everybody in my company likes object-oriented programming and RAII is often seen as "useless school stuff"...)

My coworkers says that you should always know what exceptions are to be thrown and that you can always use constructs like:

try
{
  // Stuff
}
catch (exception_type1&)
{
  // Some cleanup
  throw;
}
catch (exception_type2&)
{
  // Some cleanup
  throw;
}
catch (exception_type3&)
{
  // Some cleanup
  throw;
}

Is there a well admited good practice regarding these situations?

Answer 1

My coworkers says that you should always know what exceptions are to be thrown [...]

Your coworker, I'd hate to say it, has obviously never worked on general-purpose libraries.

How in the world can a class like std::vector even pretend to know what the copy constructors will throw, while still guaranteeing exception safety?

If you always knew what the callee would do at compile-time, then polymorphism would be useless! Sometimes the entire goal is to abstract away what happens at a lower level, so you specifically don't want to know what's going on!

Answer 2

What you seem to be caught in is the specific hell of someone trying to have their cake and eat it too.

RAII and exceptions are designed to go hand in hand. RAII is the means by which you do not have to write a lot of catch(...) statements to do cleanup. It will happen automatically, as a matter of course. And exceptions are the only way to work with RAII objects, because constructors can only succeed or throw (or put the object in an error state, but who wants that?).

A catch statement can do one of two things: handle an error or exceptional circumstance, or do cleanup work. Sometimes it does both, but every catch statement exists to do at least one of these.

catch(...) is incapable of doing proper exception handling. You don't know what the exception is; you can't get information about the exception. You have absolutely no information other than the fact that an exception was thrown by something within a certain code block. The only legitimate thing you can do in such a block is to do cleanup. And that means re-throwing the exception at the end of the cleanup.

What RAII gives you with regard to exception handling is free cleanup. If everything is RAII encapsulated properly, then everything will be properly cleaned up. You no longer need to have catch statements do cleanup. In which case, there is no reason to write a catch(...) statement.

So I would agree that catch(...) is mostly evil... provisionally.

That provision being proper use of RAII. Because without it, you need to be able to do certain cleanup. There's no getting around it; you have to be able to do cleanup work. You need to be able to ensure that throwing an exception will leave the code in a reasonable state. And catch(...) is a vital tool in doing so.

You cannot have one without the other. You cannot say that both RAII and catch(...) are bad. You need at least one of these; otherwise, you're not exception safe.

Of course, there's one valid-though-rare use of catch(...) that not even RAII can banish: getting an exception_ptr to forward to someone else. Typically through a promise/future or similar interface.

My coworkers says that you should always know what exceptions are to be thrown and that you can always use constructs like:

Your coworker is an idiot (or just terribly ignorant). This should be immediately obvious due to how much copy-and-paste code he's suggesting that you write. The cleanup for each of those catch statements will be exactly the same. That's a maintenance nightmare, not to mention readability.

In short: this is the problem that RAII was created to solve (not that it doesn't solve other problems).

What confuses me about this notion is that it's generally backwards to how most people argue that RAII is bad. Generally, the argument goes "RAII is bad because you have to use exceptions to signal constructor failure. But you can't throw exceptions, because it's not safe and you'll have to have lots of catch statements to clean everything up." Which is a broken argument because RAII solves the problem that the lack of RAII creates.

More than likely, he's against RAII because it hides details. Destructor calls aren't immediately visible on automatic variables. So you get code that gets called implicitly. Some programmers really hate that. Apparently, to the point where they think having 3 catch statements, all of which do the same thing with copy-and-paste code is a better idea.

Answer 3

Two comments, really. The first is that while in an ideal world, you should always know what exceptions might be thrown, in practice, if you're dealing with third party libraries, or compiling with a Microsoft compiler, you don't. More to the point, however; even if you do know exactly all of the possible exceptions, is that relevant here? catch (...) expresses the intent far better than catch ( std::exception const& ), even supposing that all possible exceptions derive from std::exception (which would be the case in an ideal world). As for using several catch blocks, if there is no common base for all exceptions: that is downright obfuscation, and a maintenance nightmare. How do you recognize that all of the behaviors are identical? And that that was the intent? And what happens if you have to change the behavior (bug fix, for example)? It's all too easy to miss one.

Answer 4

I think your co-worker has mixed up some good advice - you should only handle known exceptions in a catch block when you don't re-throw them.

This means:

try
{
  // Stuff
}
catch (...)
{
  // General stuff
}

Is bad because it will silently hide any error.

However:

try
{
  // Stuff
}
catch (exception_type_we_can_handle&)
{
  // Deal with the known exception
}

Is fine - we know what we're dealing with and don't need to expose it to the calling code.

Likewise:

try
{
  // Stuff
}
catch (...)
{
  // Rollback transactions, log errors, etc
  throw;
}

Is fine, best practice even, the code to deal with general errors should be with the code that causes them. It's better than relying on the callee to know that a transaction needs rolling back or whatever.

Answer 5

Any answer of yes or no should be accompanied by a rationale of why this is so.

Saying that it is wrong simply because I've been taught that way is just blind fanaticism.

Writing the same //Some cleanup; throw several times, as in your example is wrong because is code duplication and that is a maintenance burden. Writing it just once is better.

Writing a catch(...) to silence all the exceptions is wrong because you should only handle exceptions you know how to handle, and with that wildcard you can cath more than you expect, and doing so can silence important errors.

But if you rethrow after a catch(...), then the latter rationale no longer applies, as you are not actually handling the exception, so there is no reason why this should be discouraged.

Actually I've been done this for logging in sensitive functions without any problem whatsoever:

void DoSomethingImportant()
{
    try
    {
        Log("Going to do something important");
        DoIt();
    }
    catch (std::exception &e)
    {
        Log("Error doing something important: %s", e.what());
        throw;
    }
    catch (...)
    {
        Log("Unexpected error doing something important");
        throw;
    }
    Log("Success doing something important");
}

Answer 6

I generally agree with the mood of the posts here, I really dislike the pattern of catching specific exceptions--I think that the syntax of this is still in it's infancy and not able to cope with the redundant code yet.

But since everyone is saying that, I'll pipe in with the fact that even though I use them sparingly I've often looked at one of my "catch(Exception e)" statements and said "Damn, I wish I'd called out the specific exceptions that time" because when you come in later it's often nice to know what the intention was and what the client is likely to throw at a glance.

I'm not justifying the attitude of "Always use x", just saying that occasionally it actually is nice to see them listed out and I'm sure that's why some people think it's the "Right" way to go.