Why catch(Exception)/empty catch is bad

Why catch(Exception)/empty catch is bad

  • Comments 69

  

You’ve seen the advice before—it’s not a good programming practice to catch System.Exception. Because managed exceptions are hierarchical, catching the top of the hierarchy—e.g., catch(Exception)—is an easy way to make sure that you catch all exceptions.  But do you really want to catch all exceptions?

 

Empty catch statements can be just as bad, depending on the MSIL code that your language generates. C# turns an empty catch statement into catch(System.Object) which means you end up catching all exceptions—even non-CLS compliant exceptions. VB is better-behaved, turning an empty catch statement into catch e as System.Exception which limits you to catching CLS compliant exceptions.

 

Instead, you should catch the exact exception types that you expect because these are the types your code is prepared to handle. Obviously, any code that throws an exception is a good candidate for corresponding catch. MSDN documents all exceptions that can be raised from BCL functions. For example, System.IO.StreamReader () can throw ArgumentException or ArgumentNullException. If you use a StreamReader you should catch—and handle—these two exceptions.

 

We sometimes see advice that suggests you should catch all exceptions. In Eric Brechner’s Hard Code column he said that letting an application crash and invoke Windows Error Reporting (commonly called “Watson” from the pre-Vista days) was “completely idiotic and irresponsible.” He suggests that instead you catch all exceptions and “try to correct” them.

 

There’s one small problem with Eric’s advice: the word “try”. When you catch an exception you MUST handle the exceptional condition. Catching an exception is a statement that you can handle the exception and restore the program to a non-exceptional state. Trying to handle an exception isn’t sufficient. Follow the advice of someone older and wiser: “Do, or do not. There is no ‘try’.”

 

One key problem is that our exception hierarchy is a single mechanism representing multiple exceptional conditions. Krzysztof Cwalina explains this pretty well, breaking exceptional conditions into three sets: usage errors, logical errors and system failures. Usage errors are a condition that should always be fixed by the developer—for example, if an API cannot take null, the developer should ensure that she doesn’t pass in null. Logical errors can’t be detected at development time but should be handled at program runtime by the program itself. For example, a FileNotFoundException should be handled by creating the file or prompting the user for a new filename. System failures, however, cannot be handled by code. System failures indicate that something has gone wrong outside of your program’s scope. Your program can not be expected to correct a system failure. Thus the suggestion that you “try to correct” the error condition cannot reasonably be applied to system failures.

 

Let’s say your program causes an access violation and you catch the AccessViolationException. What can you do now? Find the pointer which has the bad value and set it to point to the right memory? If you could do that, you wouldn’t have AV’d in the first place. Catching the exception is the wrong thing to do if you can’t correct the error. In the case of a system failure you can’t correct the error.

 

We’re making a change in Visual Studio 2010’s CLR to make it harder to make this mistake. When an exception could indicate a corrupted process state—what Krzysztof calls a system failure—you’ll have to use a new attribute to indicate your intention explicitly. You can read about the details in the CLR Inside Out column in February’s MSDN Magazine. Even with our Corrupted State Exceptions work, catching all exceptions is not a good idea. If you can’t correct the condition that resulted in an exception being raised you’re always much better off Watsoning up (that is, letting your program exit and invoke Windows Error Reporting.) Watson will report back from the user’s machine so that you can fix the error in your next servicing or release.

 

 

Andrew Pardoe

Program Manager, CLR

 

Leave a Comment
  • Please add 5 and 3 and type the answer here:
  • Post
  • @stefan.wenig: Code contracts are not "the answer". They are just something that help developers write better programs. You mentioned "a FileNotFoundException indicates a violation of some internal precondition"... Code contracts use words like "internal precondition" and "developer error"...

    As for the rest of your comment...well, I believe I've already said everything I can say. If you believe that catching only the exceptions you expect is useless and that releasing resources in finally blocks is the sign of an obsession then I bet I'm not able to say anything that would convince you otherwise.

  • A very nice comment tail is growing at the post " Why catch(Exception)/empty is bad " on the

  • Andrew

    you twist my words.

    I made a point about catching ArgumentExceptions usually being a bad sign, because you'd be catching your own bugs instead of fixing them.

    When you call a method that might throw an ArgumentNullException, would you

    a) check the arguments you pass to that method, or

    b) happily pass nulls and then catch the resulting exception?

    You chose b), but I still don't understand why you would do that.

    This debate has _nothing_ to do with whether or not code contracts would help me (they would). This is about how to deal with those situations in the absence of code contracts. I believe catching such exceptions is only useful under special circumstances (I enumerated some). I did not see a plausible argument to the contrary yet.

    Exceptions are a powerful tool, and it's easy to hurt oneself with powerful tools. Still, I find it hard to accept that I should not be able to catch a wide range of exceptions at a high level at all. Sure, there are traps to fall into when you do this (I mentioned inconsistently modified state that outlives your try block). But instead of talking about those traps, and helping to identify and avoid them, you want to outlaw anything but

    try {

     MyMethod();

    } catch (MyMethodException) {...}

    There's so much more that exception handling can do.

    > If you believe [...] that releasing resources in finally blocks is the sign of an obsession

    Nice twisting again. I am obsessed with this, because it's something you have to get right the first time. I'm not saying it's a bad obsession. But if all we did use exceptions for is small local try/catch blocks that catch exceptions thrown one or two stack frames above, and if anything else would result in a process kill, there would be little trouble with missing cleanup code in reality.

  • @Stefan Wenig: I don't mean to twist your words. It's hard to have a conversation in a comment box; I'm sorry if I'm focusing on the wrong points. The particular words you used evoked "code contracts" in my mind as reliably as if I were Pavlov's dog.

    With regards to your last comments, I think we're in agreement. It's much better to check arguments that you pass to a method, regardless of whether or not it can throw ArgumentNullException. I'm all for validating inputs either at runtime or through assertions or code contracts.

    I also agree that small, local try/catch blocks that only go a single frame are much, much better than letting exceptions travel over a long stack. Sometimes exceptions have to travel a few frames but handling errors as close to origination as possible is usually the right thing to do.

    Of course with either of these points there are arguments to the contrary for particular scenarios. I'm aware of arguments against both but don't think they need discussion here.

    I certainly don't want to outlaw anything but try { MyMethod(); } catch (MyMethodException) {...}. I do think that most implementations of exception handling are *too* powerful (including the CLR's implementation) but that's my own personal opinion and not something I'll push as CLR doctrine :)

  • OK, now we're talking the same language again. Thanks for not letting the conversation die like it began (probably my fault for the way I started pointing out mistakes).

    As for whether it's NEVER a good idea to let exceptions travel a lot on the stack, I think we have to agree to disagree. (Which is certainly a progress, considering how this discussion went up to now ;-))

    I only ask you to acknowledge that catching exceptions in the catch-all way for message processing and similar stuff has some advantages. Whether they outweigh the disadvantages they sure bring should be up to every developer to decide. I believe you're right to warn people about the disadvantages. But you should not stop there and make this pattern a purely at-your-own-risk thing.

    If I would decide that doing exception handling in a non-local way fits my needs, I'd like to get more information and advice about what to do to enable this (state, immutable objects etc.), and which exceptions never to catch. That was my whole point.

  • No worries...in the end we're all interested in the same thing: better code.

    I maintain that the further an exception gets from where it was raised the less likely it is you can correct the error. Codebases differ, though, and I'm thinking about the common case. There are reasons to let exceptions fly over many stack frames and the fact that exceptions can travel far up the stack is one key reason that exceptions can better than checking error codes on every function call.

    I'll acknowledge that a top-level catch-all can be appropriate in some scenarios. But I will add caveats:

      - You need to make sure the program state is consistent if you want to keep running code. This is not an easy thing to accomplish in the presence of unexpected exceptions. Your mileage may vary, though, especially in hosted scenarios.

      - Windows Error Reporting offers better logging facilities than most programs offer. Some programs have need for their own custom logging or have reason to just ignore the error (like the instance I mentioned above where I used catch(...) around main). However, WER frees up most programs from having to create and maintain their own top-level logging facilities.

    As for exceptions never to catch, read the article I wrote about Corrupted State Exceptions. The main point of that work is to keep catch(Exception e) from catching exceptions that indicate a possibly corrupted process. The CLR cannot be allowed to continue to execute code if the process is corrupt. This work makes it impossible to catch this worst case of exceptions without explicitly stating your intention to do so.

    In the article I link to another MSDN article by Grunkemeyer and Cantorcini about writing reliable .NET code. It explains a bit about how to deal with state in the presence of failure.

    I promise my next blog post will be less exciting!

  • "Do not let your program execute in an unknown state. For most programmers this statement implies "do not catch exceptions your program is not prepared to handle"."

    THIS is the sensible advice that Microsoft should be spreading, along with analysis of the scenarios when general exception handlers can and should be employed. Stating repeatedly that "catch(Exception) is bad" simply reinforces the idea that Microsoft is out of touch with "real-world" developers.

  • There is nothing "always wrong". I think this kind of checks should be left to fxCop or StyleCop. In general, I rather much prefer to have application level handler where exception details can be shown to user and/or reported back to vendor. Windows Error reporting is UGLY and had huge UX issue in itself. Just a mere site of is scary and gives a bad impression to users about program that "it crashed" without any warnings. It's physcological issue but very important and it would be naive to ignore it. Also how do you expect vendors to recieve error reports generated by Windows? Most professional applicatiosn DO use catch all and send report to vendors. Many of the applications I love says it in much better way

    "Oops! Sorry. Something bad happened. Below is the detailed message. If you understand it then may be it will help you to fix the problem. Otherwise gives a call at 1-800-343-1111".

    Compare this to raw ugliness of Windows error reporting. If I start seeing that instead of message like above when app has run time error I'll probably give on those apps.

  • What an interesting discussion!

    I don't do catchalls very often and I always make contributors cry because I force them to remove them. They are actually programmers out there that think catch-ing the Exception IS handling it (and making the problem go away).

    But what makes me use or accept them is the inability of the C# language (and others) to let me express what I really want, and that is categorizing exceptions similar to what Krzsystof Cwalina and others are proposing.

    For example, if I look at, say, APIs in the System.IO namespace, do I really want to handle each and every documented or undocumented exception, probably with the same code over and over again? Or might I resort to just catch IOException, UnauthorizedException and SecurityException, given that most exceptions aren't specific enough anyway to handle them correctly (see Directory.Move for examples). Database exceptions are another example of notoriously hard to handle exceptions, even if it's something mundane such as a Timeout or ConstraintViolation.

    Some of the exceptions are indirectly caused by the user and I would rather have her repeat the action with a different set of input, than restart the whole shebang. Not everybody's app is as simple as a Twitter client.

    And no, it is not reasonable to expect a file can be opened and read in its entirety after File.Exists returns true.

    Then there are frameworks such as Enterprise Library that stipulate the use of catchalls and second-order handlers, but mostly making debugging much harder than it should be.

    And finally, WER (on XP) is just a nuisance for larger organizations, and access to [Verisign] code-signing certificates and OCA-responsibilities is not as easy and straight-forward as it is for a 5-head ISV. And AFAIK is not at all useful for any deeper .NET crash analysis.

    So I fire up my own catchalls on the layer boundaries and my unhandled exception handlers, knowing that there is little if anything I can do about EngineExecutionException, AccessViolationException, OutOfMemoryException, StackOverflowException or arbitrary objects thrown from some managed C++ wisenheimer.

    But at least I can try to get a (full) "mini"-dump if it is something else.

  • You say that "MSDN documents all exceptions that can be raised from BCL functions".  Unfortunately I have found that this isn't the case.

    So often I have encountered exceptions being thrown from a BCL method (and from the wider FCL) that are not documented in the MSDN docs.  And I have come encountered situations where the exception was documented, but the specific reason for it being thrown as not documented.  (These are errors of ommision.  Errors of commision - the docs say that an exception is thrown, but it isn't - also exist.  However, I do believe that these have been rooted out of the docs by now.  The .NET 1.x docs for System.IO.File.Exists is an example of an error of commision.)

    Here are some entries I've made on Microsoft Connect on this subject:-

    https://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=93997

    https://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=94819

    https://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=94818

    A few years ago I mentioned to a former BCL team member that the docs for the System.IO namespace don't list all the exceptions and exception reasons.  He agreed, but said it would be virtually impossible for it to be done with any reasonable degree of completeness.

    Now, if a BCL method's docs fail to include an exception that can be thrown, this is going to have a knock-on effect.  It's going to adversely affect the docs for other BCL methods, FCL methods, 3rd-party library / component methods, etc. that use it.

    What this all leads up to is this:- when I call a method, I have learned from bitter experience to regard the docs for that method to be potentially incomplete wrt the exceptions that it can throw.  So how can I, with confidence, write a catch handler for each individual exception that that method is going to throw?

    I agree that with a method in a library or component or framework, it's best to not catch System.Exception.  Let the exception percolate to the calling app.  But in the top-level application itself?  I think that's a different matter.  Say I have a WinForms app that wants to delete a file or folder that the user no longer needs.  I call the appropriate Delete method.  I catch System.Exception, and in the catch handler I could ignore the error, log it to file, or throw up a message box.  In the latter two cases I log the exception's message so that the user can see what the error is.  Is this so bad?  Remember that I'm wary that the docs don't list all the possible exceptions.  Why should I take the chance that an undocumented (and thus unknown) exception crashes out the whole app just because an unimportant file didn't get deleted?

    On more thing: Yes, when you catch System.Exception in your code you're also potentially catching system failure / corrupted state exceptions, and that's not good.  Simply logging these and carrying on in your app (app, not library!) isn't good.  But you know what?  In the seven years I've been programming .NET, I've never, ever seen one been logged or tossed up on screen in a message box.  They are incredibly rare, simply because the .NET CLR is so good at preventing them from happening in the first place.

    So I'm conflicted.  I agree with your advice in theory, and certainly agree with it in practice for libraries.  But for apps?  Hmmm.  For apps, I'd be so much more ready to follow your advice on never catching System.Exception if I knew that I could rely on the docs.

  • What started as a personal goal to motivate me to post more frequently has reached a milestone – one year! Looking back I realize that I never quite explained what these posts have been about. The intention of the Weekly Web Nuggets was twofold – to share

  • There have been couple of comments on this post that mention need to report the exception state to their own error reporting system without resorting to Windows Error Reporting (WER) for various reasons. Hence, they "need" to do a catch (Exception ex) at the top-level handler, else such an oppurtunity would be lost.

    I am bit surprised to hear this being a use-case for catch(Exception ex) pattern since when an exception goes unhandled, the CLR will deliver the AppDomain.UnhandledException event that can be used for this very purpose (of custom exception reporting) without requiring an application to catch exceptions it has no idea about.

    Was this approach considered over catch(Exception ex)?

    Thanks,

    Gaurav Khanna

    Dev, CLR Team.

  • @Andrew Thank you for your considered responses to our comments. I was unaware that developers could sign up for access to their application logs and I agree that a blog post on the WER would be valuable. I will save futher comments on this subject until then ;-)

    @Gaurav Hmmm... you are quite right, we actually use AppDomain.UnhandledException and Application.ThreadException to catch unhandled exceptions. We don't put a catch-all in every thread entry point, which would be tedious to maintain. The effect is the same, but the mechanism is far more elegant.

  • Yesterday I said that it isn't the case that MSDN documents all exceptions that can be raised by BCL/FCL methods.  Today I went looking for an example in the docs for .NET 3.5... and found one inside of 60 seconds.

    According to the very latest documentation for System.Xml.XmlDocument.Save(String):-

    http://msdn.microsoft.com/en-us/library/dw229a22.aspx

    the method can only throw XmlException!

    Hello?  What if the filename is null, or empty, or too long, or contains invalid chars?  What if the folder doesn't exist?  What if the file is inaccessible or read-only?  What if there are IO errors during writing?

    This sort of doc error is very common.  I raised the problem of widespread errors of ommission of raised exceptions in the MSDN docs on Ladybug in 2005.  Not much has improved since.

    In an app, how I can follow the directive to only catch those exceptions that the method could throw (and never catch System.Exception) when I know for a fact that the docs omit certain exceptions?

    (BTW: I've yet to read the MSDN Magazine article - I plan to soon - but on the face it, having a mechanism that separates the system failure exceptions from the rest sounds like an EXCELLENT idea!)

  • @Gaurav,

    The problem with AppDomain.UnhandledException is that it still lets the program "crash", which invokes Watson. As has been pointed out several times already, Watson is ugly and "scary" to users, and magnifies the feeling that the application is unstable and not fully supported by the people who created it ("They want me to report the error to Microsoft? What can Microsoft do about it? Why aren't they letting me report the error to them directly?").

Page 2 of 5 (69 items) 12345