General Exception Handling in C#

Question

I'm running some code through FxCop and am currently looking at clearing all of the non-breaking violations.

The code itself has a few instances of try/catch blocks which just catch general exceptions;

try
{
    // Some code in here that could throw an exception
}
catch(Exception ex)
{
    // Exception Thrown ... sort it out!
}

Now we all know this is is bad practice but I thought I knew how to do it properly - but FxCop has other ideas!

Suppose that the code in the try block could throw an IO Exception - and only an IO exception. There should be nothing wrong with doing this:

try
{
    // Code in here that can only throw an IOException
}
catch (System.IO.IOException ioExp)
{
    // Handle the IO exception or throw it
}
catch (System.Exception ex)
{
    // Catch otherwise unhandled exception
}

But FxCop disagrees with me ... it still flags this as a violation because I'm catching System.Exception.

Is this really bad practice or should/can I safely ignore this violation?

Answer 1

If you writing library(framework) then FxCop is 100% right.

You catch exceptions - what it's mean? that you know why they throwed. Are you sure you know all possible reasons?

If you writing just application then variations possible. For example if you catch all unhandled exceptions for logging.

Answer 2

What does FxCopy say the specific violation is? If you want to just log information about an exception then rethrow it then this is perfectly valid, though there are other ways this might be done. If you are re-raising it though, make sure you just use:

 throw;

and not

 throw ex;

Maybe that's the issue?

Answer 3

If the code in the block should only throw an IOException, then why do you want to catch System.Exception as well?

Then, the degree of conformancy to FxCop guidelines you want to achieve is a choice of yours. I would see nothing bad in catching every exception at the highest possible level and log all the information you can, for example.

Answer 4

You should only catch exceptions that you can do something about. Otherwise let the exception propagate up to the caller or if, for instance, in a Gui app the exceptions should be handled by a generic catch all handler and then logged. See: UnhandledException So don't catch unless you intended to do something with it.. admittedly that could simply be logging the error.

Edit Also look at Application.ThreadException

Answer 5

You are supposed to swallow exceptions you know how to handle. Catching the exception and not throwing it (either directly or wrapped in a specific exception) means you know the specifics of the exception in the context of the application. Since Exception can be anything, it's unlikely you know how to deal with it.

If you're just logging the exception it's no big deal, just log it and throw it for a outer layer to handle or catch.

Answer 6

Grey area...

In an ideal world you'd alway catch an explicit exception because, in theory, those are the only kind you can sensibly deal with - anything else should percolate through to some top level handler.

The trouble is we don't necessarily live in an ideal world and may well want to use a generic handler to accumulate additional information about the generic exception (parameters etc) into the exception and/or to perform additional logging before passing the exception back up the tree and in any case - at the appropriate level - to so arrange things that our end users don't see raw exceptions in the UI. The counter to this would be to suggest that when new errors occur at a low level (rather than arriving at your app level handler) you then add exception specific handling that can do your capture for you - but there can be deployment issues with this as a solution where including a generic case in bits of code that are, for whatever reason, a bit more prone to unhandled exceptions that one might like.

If it were me I'd probably be considering the flag on an assembly by assembly basis...

p.s. I'm assuming that at no point to you have a handler that just swallows the exception and allows the app to carry on.

Answer 7

I agree with FxCop and most of the other answer here: don't catch System.Exception ever, unless it is at the highest level in your application to log unexpected (and thus fatal) exceptions. Also check out this question that's basically about the same thing.

Answer 8

I agree with the other answers that catch (Exception ex) is fine if you either

1. just log and rethrow the exception or
2. do it at the highest level (UI) of your application.

There is one more situation that comes to my mind where it could make sense: When writing unattended code (e.g. some system service), it can make perfect sense to keep the program running if some (well-defined) sub-task fails for whatever reason. Of course, care must be taken to ensure that the reason for the problem gets logged and (maybe) the task is tried again after some time.