FAQ: Why does FxCop warn against catch(Exception)? - Part 1 [Nick Guerrera]
This is the first installment in a three-part series on why FxCop warns against catch(Exception):
FAQ: Why does FxCop warn against catch(Exception)? - Part 1
FAQ: Why does FxCop warn against catch(Exception)? - Part 2
FAQ: Why does FxCop warn against catch(Exception)? - Part 3
This question comes up a lot, and I think there’s a lot of confusion and controversy about the rule. Many people seem to think that it’s noisy, annoying, and not worth their time. I find that very unfortunate because following its guidance can really help you build better software. Here’s my attempt to convince the Nay-Sayers that catch(Exception) is almost always a terrible idea.
I’ve heard a lot of arguments against DoNotCatchGeneralExceptionTypes, but they generally boil down to one or both of the following:
- “If I call an API which doesn’t document which exceptions it can throw, then I have no choice but to catch all possible exceptions!”
- “My application needs to be robust; it can’t crash just because of an exception that I forgot to handle!”
Both arguments are ill-conceived because they presume that you can handle an exception even if you don’t know what failure it represents or how it impacts the code that will execute next. There’s an implicit assumption in both arguments that the mere act of catching an exception is equivalent to handling it appropriately, which simply isn’t true.
For the sake of the discussion to follow, exceptions can be divided in to two general categories:
- Those that signal bugs (i.e. API misuse), or catastrophic system failures
(e.g. ArgumentException, NullReferenceException, ExecutionEngineException.) - Those that signal unpreventable yet recoverable error conditions.
(e.g. FileNotFoundException, SocketException.)
In languages like Java, which have a notion of checked exceptions, only exceptions of the second kind are eligible to be checked while exceptions of the first kind must be left unchecked. On that basis and for lack of better terminology, I will hereafter refer to exceptions in the first bucket as ‘unchecked’ and exceptions in the second bucket as ‘checked’. (Note the quotes.) I am using the terms loosely to mean that if exceptions were checked in .NET, then those in the first bucket would remain unchecked while those in the second bucket could become checked.
Independent of where you stand on the checked exception debate, this distinction is very important to hold in your mind when writing exception handling code. In particular, do you really ever want to catch the ‘unchecked’ exceptions at all? Probably not! Instead, you should prevent them from occurring in the first place. For example, check if a variable is null before dereferencing it, don’t catch (NullReferenceException).
If a bug in your code manifests itself as an exception (and most of the ‘unchecked’ exceptions are just that), then it’s best to let the application terminate right at the point of failure. When the bug is discovered, you’ll get exactly the diagnostic information you need to fix it quickly, add the appropriate regression tests, and move on. On the other hand, debugging a system which aggressively swallows general exceptions feels like you’re one of the police investigators in “CSI: Miami”. The culprit has left the crime scene and you have to work backwards from the subtle clues in your program’s incorrect behavior.
And that’s the principal reason to avoid catch (Exception): it hides bugs in your code. These bugs are far more costly to resolve and since they can go undetected, the quality of your software suffers.
Some readers might be thinking that it would be helpful if the .NET Framework exception class hierarchy reflected the ‘checked’ vs. ‘unchecked’ nature of exceptions. In fact, it’s rather seductive to imagine a base class, say System.CheckedException, for all of the ‘checked’ exceptions. The argument follows that you could then replace catch (Exception) with catch (CheckedException)… and the problem would be solved since you no longer catch those insidious ‘unchecked’ exceptions. Unfortunately, this approach would be just as flawed. FxCop would still warn against catch (CheckedException) because it’s still far too general. By definition, ‘checked’ exceptions are unpreventable and must therefore be caught. Nevertheless, catching them is the easy part; the hard part is deciding what to do next, and that decision is purely a function of the specific exception type and it’s meaning in the given context. For example, to recover from FileNotFoundException for your configuration file, you might use a default configuration, and to recover from a SocketException, you might fall back to a local store until the network becomes available again. However, if you don’t know what to do about it, then let it go.
Returning to the first argument against this rule, please don’t get me wrong, I definitely empathize with the need for accurate exception information in API documentation. However, in the absence of such documentation, the safest approach is to treat all API as
“innocent until proven guilty.” Wait until you have hard evidence that a particular exception can be raised before you attempt to handle it.
Also, be sure to test your exception handling code. For example, delete a file out from underneath your application while it’s running and see how it deals with FileNotFoundException. It’s simply not sufficient to write optimistic and untested code, just because the exception is mentioned in the API documentation. I can’t tell you how many times I’ve seen untested catch handlers fail to work as expected. The classic example is a formatting exception raised in the error message that’s never provoked from a test. Spurious and unnecessary catch (Exception) blocks can completely torpedo your code coverage! (Remember: if it’s not tested, it’s broken.)
One important thing to note is that if you follow our advice and catch fewer exceptions, then exceptions will naturally travel further up the call stack. Sometimes they’ll terminate the application, but other times someone further up the call stack will know what to do and your only job is to clean up after yourself in finally blocks as the stack unwinds.
To summarize, here are some important things to consider when dealing with exceptions in managed code:
- Do not catch preventable (‘unchecked’) exceptions.
Only catch the specific unpreventable (‘checked’) exceptions that you are certain can be raised and that you know how to handle. - Test your exception handling code.
- Use finally blocks liberally for clean-up code to keep your state as consistent as possible in case an exception that you cannot handle needs to be handled further up the stack.
And finally, I’d like to point out that our team has learned this lesson the hard way. There are actually far too many catch (Exception) blocks in FxCop itself and it has caused us a great deal of grief. We’re working on removing them and I’m confident it will help us to deliver better software!
Comments
Anonymous
June 14, 2006
Great article! Thanks.Anonymous
June 14, 2006
do you have a list of the 'unchecked' exceptions?Anonymous
June 14, 2006
This article refers to a common mistake many people make (regarding all exceptions as an advanced error code).
However, there is one issue that troubles me: what if my program handles 10 specific exceptions I defined in the same way (when all these exceptions hint at different problems that are related), in this case I would want some catch blocks to perform the same code. If these exceptions don't share a common base, there is no way to make all of them perform the same code short of using the same lines (or method call) in each catch block.
Furthermore, if all developers worked correctly and used this guideline my code might be facing unexpected exceptions from 3rd party code bugs.
If this 3rd party isn't available to fix the bug, my program can fail at unexpected times.Anonymous
June 15, 2006
Of course, it would help if the Exception class was abstract, and the Framework never threw a general Exception!
For example, try calling:
System.Drawing.ColorTranslator.FromHtml("#aaax")
-> System.Exception: #aaax is not a valid value for Int32.
http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=94064Anonymous
June 15, 2006
If my program has a feature to auto-save unsaved work every 10 minutes, and the auto-save procedure (which may use a third-party library) throws an unchecked exception, are you suggesting that the program should simply crash out, because this exception must not be handled?
As for debugging, my unchecked exception handling is surrounded with #if !DEBUG, whereas the release version logs all available detail.Anonymous
June 15, 2006
>>> If my program has a feature to auto-save[...]are you suggesting that the program should simply crash out[...]
Your program should crash out in this case BUT should have a backstop of an unhandled exception handler to clean up those things which cannot be cleaned up in a finally block and report the error to the user (an possible to an error reporting site).Anonymous
June 15, 2006
Are there any security issues to take into consideration with not catching these exceptions? What comes to mind (though not specifically .NET) is some websites I've been on in the past that have bombed for whatever reason and I get an ADO error message that contains a database name or a table name. From a security standpoint that isn't good to make that type of information public. I always thought it would be better practice to trap the error, log it somewhere, and then give the user a more generic message.Anonymous
June 15, 2006
>>If my program has a feature to auto-save[...]are you suggesting that the program should simply crash out[...]
>Your program should crash out in this case BUT should have a backstop of an unhandled exception handler to clean up those things which cannot be cleaned up in a finally block and report the error to the user (an possible to an error reporting site).
You are seriously suggesting that the program should crash out, instead of reporting the error and allowing the user to save their work? You must be the lead designer for a lot of Microsoft software I have had the misfortune to use.Anonymous
June 15, 2006
There's a post on the FxCop blog by Nick Guerrera that explains why FxCop warns against the use of 'catch(Exception)'. ...Anonymous
June 15, 2006
The comment has been removedAnonymous
June 15, 2006
Oh I might be able to dig up the sample code I submitted or recreate but will take me a while.Anonymous
June 15, 2006
Let the entire aplication crash ? Are you kidding ? Tha's realy unacceptable. And btw if you notify the user that somethig bad has happened in most cases he will help out recover the aplication or some data.If you you just let the aplication close I think that very likely you're going to be in big trouble.
BTW. Whay isn't there a rule saying never let the application crash unless you're absolutly sure you can't do anything about it or recover anything?Anonymous
June 15, 2006
See also http://musingmarc.blogspot.com/2005/09/exception-handling-in-net-some-general.htmlAnonymous
June 15, 2006
As Jeff Parker and Pop Catalin Sever have pointed out, I catch(Exception) in my app.
If my callback from the WebBrowser has a bug and throws (say) NullReferenceException, the WebBrowser control will silently eat the NullReferenceException. I want to know when if my code has a bug, but the WebBrowser hides my bugs.
Also, if my app throws an unhandled exception in the field, I want to know about it. My global unhandled exception handler will log unhandled exceptions and display a pretty error dialog. Maybe .NET 2.0 will send my unhandled exceptions to Microsoft's Error Reporting Service, but I am using .NET 1.1 and I want the error reports sent to me directly.Anonymous
June 16, 2006
Here are another three cases where you absolutely must catch Exception (and at the very least provide for adequate logging, even if you then rethrow):
1. In every method you spin up in a new thread.
2. In every method called arbitrarily on threads you don't control (ie. WebMethods, methods exposed via remoting, delegates passed across remoting, windows service control overrides, etc.)
3. Main
Seeing a pattern here? :)Anonymous
June 17, 2006
On Wednesday, I explained why catch (Exception) is a bad idea, and many of you replied with interesting...Anonymous
June 17, 2006
I've made my first contributions to the FxCop team blog on a subject that's near and dear to my heart...Anonymous
June 17, 2006
Those who have read the book Framework Design Guidelines know that this is a bad practice. Catching System.Exception....Anonymous
June 20, 2006
The comment has been removedAnonymous
October 12, 2006
I said from the beginning that this issue is controversial, and some of your feedback certainly confirmsAnonymous
January 23, 2007
Throwing a general exception type such as System.Exception or System.SystemException in a library orAnonymous
January 30, 2007
PingBack from http://blogs.msdn.com/fxcop/archive/2006/06/17/FAQ_3A00_-Why-does-FxCop-warn-against-catch_2800_Exception_29003F00_-2D00-Part-2-5B00_Nick-Guerrera_5D00.aspxAnonymous
January 30, 2007
PingBack from http://blogs.msdn.com/fxcop/archive/2006/06/19/FAQ_3A00_-Why-does-FxCop-warn-against-catch_2800_Exception_29003F00_-2D00-Part-3-5B00_Nick-Guerrera_5D00.aspxAnonymous
January 30, 2007
Krzysztof Cwalina, owner of the Framework Design Guidelines , has written a great post on How to DesignAnonymous
January 09, 2008
Šodien   Vakar Andrejs jau aizskāra tēmu, par kuru es jau pasen gribu uzrakstīt, bet kaut kā