FAQ: Why does FxCop warn against catch(Exception)? - Part 2 [Nick Guerrera]
This is the second 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
On Wednesday, I explained why catch (Exception) is a bad idea, and many of you replied with interesting comments. Since it was too cumbersome to write everything I had to say in the comments section, I’ve replied to your comments below.
K: “Do you have a list of the 'unchecked' exceptions?”
No. I don’t have a definitive list of the exceptions which should always be prevented and never caught. The best advice I can offer here is to follow the API documentation on a case-by-case basis. Don’t immediately assume that you need catch blocks for each of the exceptions that are listed in the documentation for a particular API. Instead, for each exception, ask you yourself if there’s a simple way to prevent it. If there is, then put in the preventative code and leave out the catch block.
This can be tough to discern in some circumstances. For example, it may seem that a call to File.Exists can prevent FileNotFoundException, but there’s a race condition where the operation will still fail if the file is deleted in between the time File.Exists returns true and the call is initiated. Also, there are some methods in the .NET Framework which misuse exception types. For example, in an ideal world, you should never catch ArgumentException, but Enum.Parse throws it to indicate that the input does not represent one of the enum values. Since there’s no way to prevent that short of rewriting Enum.Parse, you have to consider ArgumentException ‘checked’ in that specific case.
If you’re unsure about a specific API, try asking for help on https://forums.microsoft.com
Gill: “… 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.”
I don’t think there are that many methods out there which throw 10 unrelated and unpreventable exceptions, but I can relate to your frustration with API which throw unrelated exceptions to indicate related failures. Refactoring the shared handler code in to a helper method is the simplest way to deal with the issue. If you find yourself catching the same set of exceptions from the same method in more than one place, consider refactoring out the entire try/catch/catch/… series in to a helper method of its own.
Gill: “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.”
I don’t follow the reasoning here. If you hit a bug in 3rd party code, then catch (Exception) is just going to make things worse. You should treat this unhandled exception no different from any other. It may turn out that you can work around the problem by using the API differently or adding a catch handler for the specific exception type that was raised if it turns out to be recoverable.
Richard: “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")”
Youch! You’re right; it’s absolutely terrible to throw an instance of System.Exception and FxCop warns against that as well. I will do my best to argue for reopening and fixing this bug!
Anonymous: “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?”
Yes, that is in fact exactly what I’m suggesting. A bug is a bug is a bug. It doesn’t matter if it’s in your code or 3rd party code, catch (Exception) will just make it harder to find and fix. Having an auto-save feature is a great way to mitigate the risk of data loss. However, if the auto-save code itself is buggy, then all bets are off. It’s sort of like discovering that your parachute won’t open…
Jeff: “Well here is my slight problem with this. The best example I can give you as to why you absolutely have [to] Catch(Exception ex) to do this is a bug I reported back in Beta 2 … The Browser Control …”
Chris: “… 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.”
That’s awful! I will talk to the owners of the Web browser control about getting this fixed. One of the things that I failed to mention in my original post is that it’s much worse to swallow exceptions in library code than in application code. If you’re developing a reusable library, this rule (like many others in FxCop) is doubly important!
Pop Catalin Sever: “Let the entire application crash? Are you kidding? That's really unacceptable. And btw if you notify the user that something bad has happened in most cases he will help out recover the application or some data. If you just let the application close I think that very likely you're going to be in big trouble.”
Let the application continue to run in an indeterminate state and carelessly corrupt data? Are you kidding? ;)
While I agree that the user should be notified that something has gone wrong, I don’t agree that the application should continue running afterwards. Instead, a friendly dialog should allow the user to send an error report before the application closes. If data loss is a potential issue in your application, then implement an auto-save feature. It’s worth noting that this is exactly the approach that’s used in Microsoft Office.
Beginning in .NET 2.0, you don’t need to implement the crash dialog box yourself. If you let the exception go unhandled, a default dialog box will be displayed that allows the user to send an error report to Microsoft. Note that for Windows Forms applications, you need to call Application.SetUnhandledExceptionMode(UnhandledExceptionMode.ThrowException) to make this work.
If you prefer, you can hook the AppDomain.UnhandledException event and display your own dialog that allows users to send a bug report directly to you.
Jeremy: “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? :)”
It sounds like you got burned by the default exception handling policy for secondary threads in .NET 1.0 and 1.1. Thankfully, the situation is vastly improved in .NET 2.0. See https://msdn2.microsoft.com/en-us/library/ms228965.aspx.
If you can’t move to 2.0 just yet, then a better approach than sprinkling catch (Exception) in all of these places is to hook AppDomain.UnhandledException (and Application.ThreadException for Windows Forms applications). In your handler, you can throw up a dialog like the default dialog in .NET 2.0 and call Environment.Exit afterwards.
Comments
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
The only place I use a catch(exception) is right after db code then I immediatlly rethrow the original exception.Anonymous
June 17, 2006
Same here DeepICE2 - I'm curious what Nick has to say about situations like:
try
{
}
catch (Exception)
{
dbTransaction.Rollback();
throw;
}
Obviously with a bit more error checking around the dbTransaction.Rollback call to make sure it's state is valid.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 18, 2006
The link http://msdn2.microsoft.com/en-us/library/ms2208965.aspx. above does not seem to lead anywhere useful. Can you correct it?Anonymous
June 18, 2006
The link is now fixed. Sorry about that.Anonymous
June 18, 2006
DeepIce2, ShadowChaser: That brings up an issue that I haven't discussed yet -- rethrowing from a catch (Exception) block. It's obviously better than swallowing the exception outright, but there are still some things watch out for.
1) As your example demonstrates, "throw;" is better than "throw ex;" since the latter will trash the original stack trace.
2) catch (Exception ex) { throw new SpecificException(...); } is about as bad as swallowing the exception because callers are probably under the impression that they they can handle SpecificException, but it can actually represent an arbitrary bug!
3) catch (Exception) { throw; } won't hide bugs, but it does make them harder to debug, so use it sparingly. The problem is that the exception isn't considered unhandled until the exception is rethrown. If you hit debug on the unhandled exception dialog, you'll be taken to the rethrow rather than the original failure. I generally prefer the following construct:
bool succeeded = false;
try {
...
succeeded = true;
}
finally {
if (!succeeded) {
...
}
}Anonymous
June 18, 2006
PingBack from http://blogs.msdn.com/fxcop/archive/2006/06/14/631923.aspxAnonymous
June 18, 2006
The comment has been removedAnonymous
June 18, 2006
The comment has been removedAnonymous
June 19, 2006
The comment has been removedAnonymous
June 19, 2006
PingBack from http://blogs.msdn.com/fxcop/archive/2006/06/19/638453.aspxAnonymous
June 21, 2006
+1 to Jeremy Gray's comment.
You've been silent here on elsewhere on what to do when programming to interfaces, a very common scenario out here in the real world.
If you keep ignoring common real-world situations (and no, "Wait until you have hard evidence that a particular exception can be raised before you attempt to handle it" doesn't cut it), your recommendations will continue to sound like they come from an ivory tower.
I think most people agree that in general we should avoid catching general exception types. The FxCop rule is definitely very valuable. But it's wrong to say there are no exceptions to this rule.
I think it would be interesting to look for examples in the .NET Framework itself where general exceptions are caught, and discuss whether the designers were right to do so.
A first example from .NET 2.0,
- GetDesignTimeHtml() for some of the WebControl designers.Anonymous
June 21, 2006
> “Do you have a list of the 'unchecked' exceptions?”
There is an internal class ClientUtils in System.Windows.Forms 2.0 which has a method bool IsCriticalException(Exception ex).
Typical use is in a catch block for general exceptions such as:
try
{
...
}
catch(Exception ex)
{
if (ClientUtils.IsCriticalException(ex)) throw;
}
The exceptions it considers critical are:
NullReferenceException
StackOverflowException
OutOfMemoryException
ThreadAbortException
ExecutionEngineException
IndexOutOfRangeException
AccessViolationException
I think some more would need to be added to the list for a more general list, e.g. ArgumentExceptionAnonymous
June 21, 2006
JocularJoe: "You've been silent here on elsewhere on what to do when programming to interfaces, a very common scenario out here in the real world."
You're not suggesting that every interface call needs to be wrapped in catch (Exception), are you? Is the point that since interfaces have arbitrary implementations, then they can throw arbitrary exceptions, and therefore you're left with no choice but to catch them all?
I know you will disagree, but interfaces really don't change things at all. A well defined interface should document the exceptions that implementers are allowed to raise and that callers are supposed to handle. All other excceptions should be considered either a bug in the caller (pre-condition violation) or a bug in the callee, which shouldn't be caught or the bugs may never get fixed, and some of them risk being more severe than a crash.Anonymous
June 21, 2006
JocularJoe: "Here is an internal class ClientUtils in System.Windows.Forms 2.0 which has a method bool IsCriticalException(Exception ex)."
Do not copy ClientUtils.IsCriticalException in to your code, it's a bad idea. If you must catch (Exception), don't bother trying to filter out only the bad ones. Looking for this list is the precise trap I described in the thought experiment about using the exception hierarchy to express the nature of 'checked' vs. 'unchecked' exceptions. If the goal is to simulate a catch block which catches every possible 'checked' exception, then you are still likely to hide bugs.Anonymous
June 21, 2006
> You're not suggesting that every interface call needs to be wrapped in catch (Exception), are you?
Absolutely not. Just let me reiterate that I agree the rule is very valuable, and that in most cases I agree it is wrong to catch general exception types. The rule is great for identifying inappropriate catching of exceptions such as that described by Ploeh in comments to your second followup.
But I don't agree with the FxCop documentation that states that catching general exceptions should never be allowed. There are a few cases where it is appropriate, notably in a top-level exception handler. The top-level exception handler should typically log the exception and inform the user that an error has occurred.
A company I respect developed a framework for serving web pages, I think they called it ASP.NET. The framework would dynamically compile and run third party code, which of course could throw exceptions whose type they could not know.
I think their framework must have a top-level exception handler that catches general exceptions thrown from third party code then serves an error page.
Are you suggesting that this design is wrong? What alternative would you propose?Anonymous
June 21, 2006
JocularJoe: "Are you suggesting that this design is wrong? What alternative would you propose?"
No, there's nothing wrong with a top-level handler in Main and FxCop won't even fire against it. However, I prefer to use the unhandled exception events: AppDomain.CurrentDomain.UnhandledException and Application.ThreadException for Windows Forms.
The problem with a top-level handler is that it will miss exceptions raised on secondary threads, which in turn will force you to do a lot of book-keeping to transfer the exceptions to the main thread. If all that you need to do is display a polite error message and offer the user a way to inform you of the defect, then the events are the easiest way to make that happen.Anonymous
June 21, 2006
The comment has been removedAnonymous
June 21, 2006
> nothing wrong with a top-level handler in Main
My definition of top-level handler is more genral: I would also include say a try/catch block in a button click event handler in a WinForms application. Essentially a try/catch block in a method which is not called directly from other user code. In my button click event handler I'm typically calling into the business tier, then updating some controls on the Form. In the general case I may not always know what exceptions are raised by the business tier, so I must use catch(Exception) or my application will crash.
Consider the following requirement:
- WinForms Form with a grid and a button.
- Clicking on the button should retrieve all ASP.NET Membership users and display them in a grid (i.e. business tier will call System.Web.Security.Membership.GetAllUsers).
- If the persistence medium used to store all Membership users is inaccessible, the application should display a warning but not exit.
As far as I can see it is impossible to do this without a general exception handler.
And this is far from atypical: in general the presentation tier should not know how data is persisted, so does not know whether to catch a SqlException, OleDbException, XmlException, etc.Anonymous
June 21, 2006
> A well defined interface should document the exceptions that implementers are allowed to raise and that callers are supposed to handle.
The only sure way implementers of the interface can do this is by having a general exception handler which catches and wraps all exceptions.
It's interesting to look at the design of the ASP.NET Membership functionality (uses abstract base classes rather than interfaces, but the principle is the same). IIRC Beta 1 would always return an HttpException. This was done by catching exceptions from the underlying provider in a general execption handler and wrapping in an HttpException. In RTM this has been changed, so that the underlying provider's original exception is propagated unchanged.Anonymous
June 22, 2006
JocularJoe: "The only sure way implementers of the interface can do this is by having a general exception handler which catches and wraps all exceptions."
Not true. Any exceptions that are thrown in the interface method call that implementor of the interface wasn't expecting, is a bug.Anonymous
June 22, 2006
JocularJoe: "In the general case I may not always know what exceptions are raised by the business tier, so I must use catch(Exception) or my application will crash."
So you're willing to hide bugs in the business tier to prevent a crash? That's a policy decision that you're free to make, but that's basically what you're deciding.
JocularJoe: "If the persistence medium used to store all Membership users is inaccessible, the application should display a warning but not exit."
Agreed, but do you feel as strongly about "If the data tier dereferences a null pointer, then the application should display a warning, but not exit?" If so, what is the right warning message? Can you come up with a single error message that works in both cases?
JocularJoe: "And this is far from atypical: in general the presentation tier should not know how data is persisted, so does not know whether to catch a SqlException, OleDbException, XmlException, etc. "
I agree that this isn't atypical. However, the right solution is to document an exception contract for your business tier so that the presentation tier knows what to catch. Obviously, you wouldn't document all of those domain specific exceptions. Instead you could catch each of them (but not all exceptions) in the business tier and resurface them as a single exception type that's part of the business tier's contract.
try {
...
}
catch (OleDbException ex) {
throw new PersistenceMediumNotAvailableException(..., ex);
}
This technique is exactly how checked exceptions in Java are made to work with virtual methods and interfaces. You have to handle or declare that you rethrow all of the checked exceptions from your callers. However, if you implement an interface or override a virtual method, you cannot change the exception signature. In that case, the only solution is to catch each of the checked exceptions from your callers and handle them appropriately or re-surface them using the interface contract's checked exceptions.
Once again, I'm not advocating that .NET should have checked exceptions, but I do think that the thought experiment, "how would this be handled if the system had checked exceptions", can help to find the right approach in many cases.Anonymous
June 22, 2006
The comment has been removedAnonymous
October 12, 2006
PingBack from http://blogs.msdn.com/fxcop/archive/2006/06/19/follow-up-2-more-on-catch-exception-nick-guerrera.aspxAnonymous
January 30, 2007
This is the third in a three-part series on why FxCop warns against catch(Exception): FAQ: Why does FxCopAnonymous
January 30, 2007
This is the first installment in a three-part series on why FxCop warns against catch(Exception): FAQ:Anonymous
January 30, 2007
Krzysztof Cwalina, owner of the Framework Design Guidelines , has written a great post on How to Design