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 http://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 http://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 (28)

  1. I’ve made my first contributions to the FxCop team blog on a subject that’s near and dear to my heart…

  2. DeepICE2 says:

    The only place I use a catch(exception) is right after db code then I immediatlly rethrow the original exception.

  3. ShadowChaser says:

    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.

  4. Those who have read the book Framework Design Guidelines know that this is a bad practice. Catching System.Exception….

  5. dermot says:

    The link http://msdn2.microsoft.com/en-us/library/ms2208965.aspx. above does not seem to lead anywhere useful. Can you correct it?

  6. MSDNArchive says:

    The link is now fixed. Sorry about that.

  7. MSDNArchive says:

    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) {
             …
         }
     }

  8. Niall says:

    I think the definition that catching Exception is always bad is too simple. Catching Exception and doing nothing about it is bad.

    In a previous project I worked on, we had a lot of places where exceptions were caught and error reports were sent back to the dev team. Therefore, the exceptions weren’t just disappearing quietly, they were known to the developers.

    In many circumstances, the application would then continue. This is because a lot of our handling of exceptions would result in the form the exception came from closing or not appearing. Basically that element of the workflow did not occur. For almost all cases, this left the application in a state that allowed it to continue without problems. Of course, it wasn’t 100% perfect. We did have a threshold of a certain number of exception reports in a certain amount of time would cause the handler to exit the app, though.

    I think you have to realise that exception handling is not just for the developer’s benefit. The application is there to serve the user, not the developer, so simply bombing out because you can’t display a new form is going to require the user to restart the application more than is really required.

    Especially when the environment is such that you cannot deploy a fix at a day’s notice, having the users restarting the application frequently when you could provide better is basically developing to suit the developers instead of the users. Of course, it can make diagnosing problems more difficult, but the role of the software is to make the user’s job easier, not the developer’s.

    Sometimes the developer just has to work a bit harder because otherwise the work goes on the user’s plate.

  9. Niall says:

    I guess, as you may have too, that my point of view is that of the application developer, not the library developer ;P

    Whenever (it happens a lot) I find libraries just catching Exception, I do indeed think this is evil and should result in people going straight to hell without collecting $200.

  10. Jeremy Gray says:

    Riiight.

    To be clear, I really do appreciate all of the writings of this blog and intend to continue to read it.

    That out of the way, I hope you do realize that a number of the recommendations you have given to people, myself included, basically amount to "let your app crash, your customers and business partners be damned." The auto-save example is a particularly telling point.

    I think all of your readers know full well that applications need to be designed to fail fast and do so clearly in order to ease the diagnosis and correction of software defects. We do this, however, by modularizing software and testing it at an appropriate level of granularity, not by letting the whole app fall down in production. Your recommendations will only work well for the people replying to your posts if you are interested in personally supporting their products when deployed in the field. Their customers and partners don’t care about what FxCop might have recommended to the developer. Given that your own readership is raising so many legitimate issues with the recommendation that FxCop is providing, this is almost surely just one of those places where FxCop is, to be blunt, simply wrong.

    MSFT, for all their good intention and staff expertise can’t always see the realities of the experience developers have out in the real world. Wrong is an okay thing to be from time to time.

  11. JocularJoe says:

    +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.

  12. JocularJoe says:

    > “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. ArgumentException

  13. MSDNArchive says:

    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.

  14. MSDNArchive says:

    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.

  15. Joe says:

    > 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?

  16. MSDNArchive says:

    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.

  17. nicholg says:

    Joe, I guess I read your last comment a little too quickly, sorry.

    If you’re building a framework as sophisticated as ASP.NET, where you are hosting arbitrary managed code and the process is a precious shared resource which cannot be aborted abrubtly, then clearly you have to take measures to isolate the arbitrary components. Catch (Exception) will be required in that case, which I acknowledged in my second follow-up. If you have a similar application architecture, then it is reasonable to exclude the message when catching exceptions at component boundaries. Note that getting this right entails more than just adding catch (Exception) blocks. For example, ASP.NET also uses separate AppDomains for separate web applications.

    I have to admit that I haven’t worked much with ASP.NET and it is very far from my area of expertise. Since I’m sure that this has made it hard for me to relate to some of the feedback that I’ve received, I think I will take some time to research exception handling in web applications and post another follow-up…

  18. JocularJoe says:

    > 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.

  19. JocularJoe says:

    > 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.

  20. davkean says:

    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.

  21. MSDNArchive says:

    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.

  22. Joe says:

    > So you’re willing to hide bugs in the business tier to prevent a crash?

    Certainly not.  The exception handler must log the error and inform the user that an error has occurred.

    > If so, what is the right warning message? Can you come up with a single error message that works in both cases?

    In a business app the right message in both cases is probably something like "An unexcepted system error has occurred.  Please contact support".  Support will of course have access to the logged exception details.

    > Instead you could catch each of them (but not all exceptions) in the business tier …

    The point of this example is that my business tier component is calling Membership.GetAllUsers() and can not know all the exceptions that could possibly returned from all possible providers.  And therefore the only way to fulfill an exception contract is to use a general exception handler:

    try {

      …

    }

    catch (Exception ex) {

      throw new PersistenceMediumNotAvailableException(…, ex);

    }

    It’s interesting that the approach of having an exception contract was tried in Beta 1 of the ASP.NET Membership functionality (which returned HttpException), then rejected in the RTM version – maybe you could get the developers to give the rationale for that decision.  IMHO the only way to have an exception contract in a loosely coupled system without general exception handlers is if each tier below also exposes an exception contract – which almost amounts to checked exceptions.

  23. This is the third in a three-part series on why FxCop warns against catch(Exception): FAQ: Why does FxCop

  24. This is the first installment in a three-part series on why FxCop warns against catch(Exception): FAQ:

  25. Krzysztof Cwalina, owner of the Framework Design Guidelines , has written a great post on How to Design