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 4 and 4 and type the answer here:
  • Post
  • This is a bit unrelated to the CLR specifically but the mention of logic exceptions kicked off my frustration again :)

    I find logic exceptions in the BCL like FileNotFoundException really anoying in cases where the circumstances simply aren't exceptional. I might be expecting a file to not exist but I have no way to avoid this exception. File.Exists(...) doesn't cut it because there's a race with other processes that might delete the file so I have to handle the exception anyway. The same goes for things like ServiceController.Start(), even if I check the status, another process may have already started the service so I get an exception.

  • @all: While the CLR team doesn't own WER/Watson we are working all the time to make the WER experience better for managed users. The next version of the CLR will have better information in WER reports and Visual Studio 2010 supports debugging managed programs from the minidump included in a WER crash dump.

    Users should be pretty familiar with crash reporters. WER is one, definitely, but Apple has "Crash Reporter" and Firefox has "Talkback". They are very common and should be seen as an opportunity to help the user instead of just being the bearer of bad news. That said, I understand that crash reporters have a bit of a perception issue that they need to address.

    As I've said, look for a blog post on WER in this space Real Soon Now.

    As for documentation errors, I realize MSDN isn't perfect. If you find any errors you should report them. Our documentation manager, Maira, has a blog entry where she talks about the "Feedback" link on MSDN pages: http://blogs.msdn.com/mairaw/archive/2008/04/30/the-power-of-your-feedback.aspx. I'm pretty sure that text entered in the feedback box ends up as a bug in her bug database so this may be more efficient than using Connect. (The feedback box pops up when you mouse over "Click to Rate and Give Feedback *****" on the top right corner of the web page.)

    As for what constitutes "sensible advice", well, I'll start preaching that you shouldn't let your program execute in an unknown state. That's actually a fantastic opportunity because it lets me complain about more than just exceptions :)

    (If you want my real opinion on the matter, read Richard Rorty's _Contingency, irony and solidarity_ wherein he postulates that truth cannot exist only represented in sentences. Words are signs that point at an idea, not the ideas themselves. Ideas can only exist in our minds. I believe saying "Don't catch(Exception e)!" puts an easy-to-remember idea into peoples' heads that will pop back up whenever they write catch(Exception). If I just say "Don't let your program execute in an unknown state" the natural reaction is to say "Oh, don't worry, *I* would never do that!" without actually thinking about what that implies.)

  • @Andrew Pardoe: Nice response.  I particularly liked the last paragraph.  Of course the idea of not catching System.Exception is a good one.  I just wish I could follow it more readily.

    With documentation improvements I will be able to follow it more, but at least with the new CSE mechanism (I've read the MSDN Mag article now) I won't have to worry about catching exceptions I really don't want to catch.  The irony of CSE is that it makes catching System.Exception more acceptable than before.

    Re. documentation: Yes, using Connect is slow and painful.  And I have ignored the Feedback link mainly because I often don't notice it, and also because I've feared that the feedback gets dumped in digital Tumbolia and forgotten about.  Anyway, I've given feedback on XmlDocument.Save(String), and asked Maira (via the blog entry you posted to) to confirm that she gets it as a bug entry.  If that works then I'll happily report more as I find them.

    I wonder: could the process of determining what exceptions an FCL method can throw be automated?

  • @Andrew Pardoe: One of the reasons why I might catch System.Exception is because I call some method, and want to report if/why it fails to the user, but Intellisense says that it can throw 10 or more exceptions!  Rather than write out 10 catch blocks that all do the same thing, it's soooo tempting to just catch System.Exception.

    So in .NET 4.0, why not let people write this:-

    catch (<comma-delimited list of exceptions> ex)

    {

     MessageBox.Show ("Oh dear:  " + ex.Message);

     // return, or continue

    }

    How many times have I wished I could write that?!  I've lost count.

    Of course the CLR would have to upcast 'ex' to System.Exception, but that's totally fine because the only reason you'd catch >1 exception in one statement is to access the 'Message' property or call 'ToString', or similar.  And if you really needed to differentiate in the handler, you could always use 'is', 'as' and Object.GetType().

    The throw; statement would re-throw the original exception, and the order of the exceptions in the comma-delimited list would be honoured in the same way as the order of vertically stacked catch blocks is now.

    Naive?  Possible?  Unlikely?  Never; forget about it?  Maybe in v5.0?  Yes, we'll do it right away?

  • "If a suitable catch clause is found, the CLR will unwind the stack as normal but will only execute finally and fault blocks (and in C#, the implicit finally block of a using statement) in functions marked with the attribute. The HandleProcessCorruptedStateExceptions attribute is ignored when encountered in partially trusted or transparent code because a trusted host wouldn't want an untrusted add-in to catch and ignore these serious exceptions."

    [HPCSE]

    void A( ) { try {  B( ); } catch( Exception e ) { } }

    void B( ) { using( SomeResource obj = Util.KidnapPresident( ) ) { } }

    void C( ) { /* Do something hideous here so that a CSE is thrown. */ }

    With this little piece of code, congratulations to the designer of this feature because you have successfully held the president hostage.

  • @Andrew Webb: Catching multiple exceptions using a comma-delimited list would have to be exposed through a language as well. It's an interesting idea...I could see the benefit of catch(ExceptionA | ExceptionB | ExceptionC).

    At the end of your suggestion you've hit on one of the key points of feature design: Features are easy. Interactions between features are hard. What does e.ToString() do? What does throw do? And what exception do you show if you create a minidump?

    @Tanveer Badar: The fix is to add the attribute on method B as well. We had long debates about not running finally blocks in the presence of CSE--especially for CERs--but decided in the end to not run them. I don't think it's unreasonable to suggest that privileged code should be written correctly.

    If you hadn't used the [HPCSE] attribute on A() then your resource (or president) would have been released as soon as your process exited. Your code is harmful because you've used the feature to swallow the exception but not used the feature to clean up the state you changed under escalated privilege. You can't have it both ways.

    We could produce knives with handles on both ends instead of a handle and a blade but I don't think you'd find them useful when cooking.

  • @Andrew Pardoe: "Features are easy. Interactions between features are hard."  100% right.  Which is why I was serious about querying whether the suggestion might be naive.  Maybe a nice idea, but is it really doable?  But I like to throw out ideas as 'triggers to thought' - my own thought, if nothing else.

    Actually getting interactions between features right is something I think that Microsoft is remarkably good at.  At the time of adding generics to .NET 2.0 I was mindful that it wasn't just about allowing programmers to write and consume generics... the work that went into making it all work really well in Intellisense as well was pretty amazing.  I can think of lots of other examples of this sort of thing.

  • I assume then something (compiler itself or fxcop or some other tool) will have to ensure that you don't have a code path like the example I suggested. Because it will happen and people won't be happy when it does.

  • @Tanveer,

    I don't see why an FxCop rule couldn't be written for it. Although I think the assumption is that if you choose to use an extremely advanced feature, you will take the time to learn how to use it correctly. As Andrew said, a sharp knife is both useful and dangerous, and you can't have one without the other.

  • How often did you see a C# catch(Exception e) statement in code? How often did you write it yourself

  • While doing some reading on exeptions, I found the following link.  In try/catches, the author catches System.Exception and then checks whether the exception is 'critical' or not (critical meaning OutOfMemoryException, ExecutionEngineException, ...).  If it is critical, then the exception is rethrown.  Is that approach good or bad?

    http://netcode.ru/dotnet/?lang=&katID=30&skatID=261&artID=7099

  • System.Threading.ThreadAbortException is just plain weird. For instance, most exceptions happen because

  • Good Lord, why wouldn't it be so hard to just write perfect code in the first place? Well, for one, Windows itself is nowhere NEAR perfect. I'd say that every coder should decide which exceptions to catch and why, given their application. If everyone would just think about their code and test it EXTENSIVELY, you probably wouldn't have to deal with many exceptions at all in the first place. If I run into exceptions, I just go back and review what I've written, THEN I decide what kind of errors to handle. Most of the time that works well, but not always. Nevertheless, I rarely release any code that is not well tested and bug-free (as far as I can tell anyway). If there is a bug, I catch and kill it with correct code. The only bad thing is that MS often causes errors on the fly that I may never be aware of until it's too late. Safe to say it's mostly Windows that causes exceptions if the code is well-written!

  • 家出中の女性や泊まる所が無い女性達がネットカフェなどで、飲み放題のドリンクで空腹を満たす生活を送っています。当サイトはそんな女性達をサポートしたいという人たちと困っている女性たちの為のサイトです

  • セレブ女性との割り切りお付き合いで大金を稼いでみませんか?女性に癒しと快楽、男性に謝礼とお互い満たしあえる当サイト、セレブラブはあなたの登録をお待ちしております。

Page 3 of 5 (69 items) 12345