Do Not Catch General Exception Types

Question

I have a question about the fxCop rule on "Do Not Catch General Exception Types"...I started some discussion on this in another thread but I didn't get a full answer to my question and I thought this topic was worth starting a new thread.

I think I'm using the recommended Microsoft approach for Exception Management:

• I'm using the Enterprise Library Exception Management Application Block to handle exceptions
  
• At the "top of the food chain" I have defined some policies for handling exceptions and use the EM application block to determine how to handle, report, and log the error.  In all cases at the moment, it simply reports the error message to the user and logs it to the event log.  I have two policies set up:
  
•  A "Debug Policy" dumps a fairly complete error message and is used for development and debugging
  
• A "Global Policy" provides a very brief and more user friendly error message with a much more limited amount of information
  
• At lower levels in "the food chain", lower level routines generally rethrow the exception so that it is processed at the highest level and not processed multiple times

I have about 50 places in my code that are at the top level of the food chain that do the handling of exceptions through the Exception Management Application Block and several hundred lower level places in the code that simply rethrow the exception up to the higher level.  At every one of these 50 places in my code, fxCop declares an issue "Do Not Catch General Exception Types" and that doesn't seem to make any sense to me.  Am I supposed to let these exceptions go unhandled??

Here's my code that implements the exception handling at the top of the food chain looks like this:

              try
                   {
                   Some code to do something...
                   }
            catch (Exception ex)
                   {
                   HandleError(ex);
                   }

The "HandleError" routine above calls a method that uses the Exception Management Application Block to handle, report, and log the exception.

Is there another way to do this that would achieve the same result and make fxCop happy?

Thanks,

Chuck
                  

Answer 1

Chuck,

Sorry I missed your reply to the other thread.

I'm confused as to how you could have 50 places where this is needed. What do you consider top of the food chain? How would FxCop go about determining that these cases are valid?

There are only 2 places where I would recommend catching exception; 1 is in the main method, and the other is when you are catching exception just that it can be passed to another thread.

Answer 2

Hi David,

Thanks for your response...

An example of what I would consider the "top of the food chain" would be any action that is initiated by user input...for example:

• A user selects a menu item
• A user clicks a button on a form or a control
• etc.

Does that make sense?  I think if I rethrow these exceptions, they have no place to go because it was triggered by an event (user input) that is not in the mainstream flow of the application, right?

Chuck

Answer 3

Chuck,

Ah, okay I seem what you mean. In this situation, you still shouldn't be catching Exception, you should instead attach to Application.ThreadException and handle the exception in that.

For other exceptions thrown out of WinForms, you should also look at attaching to AppDomain.UnhandledException.

Your event handlers should only catch exceptions that they can actually handle, and let others flow through to the Application.ThreadException event.

Answer 4

I would disagree with this advice; I think the pragmatic thing to do is to catch Exception in top level handlers.

E.g. consider a WinForms apps which uses an n-tier architecture, where calls to the business tier use a service locator pattern, which can be configured to call the business tier in-proc, using remoting, or using a web service.

The client app should assume that any call to the business tier can throw an exception, but has no idea whether this will be a RemotingException, TargetInvocationException, SoapException, or in the in-proc case an exception thrown from the business/data tier code.  And in the in-proc case, the client app has no business knowing whether an exception from the data tier is a SqlException, OleDbException.

IMHO the only practical thing to do is to wrap all calls to the business tier in a try/catch which catches general exceptions - then log the exception and display an error message.

 

Answer 5

Joe, this topic has gone round-n-round on many different venues.

It's reasonable to catch Exception in the entry method because you're simply going to shutdown right afterwards (even if you log the exception).  With the case of catching Exception to give to another thread, ideally you're shutting down the thread that threw the exception as well.

One of the things that came out of some of the other discussions on the topic of why not to catch Exception: when catching Exception: you don't know that you can't handle it.  For example, if the exception you caught was a StackOverflowException, do you really think making a call to display a message box is a good thing?  This leads into the rule "Only catch exceptions you know you can handle"

In your example of having no business knowing about OleDbException, that's true, and it shouldn't catch it as Exception either.  If you don't know you can handle the exception, you don't know if your application can continue after receiving it.  OleDbException, for example, if you caught that as Exception, displayed an error, then let the user continue; you don't know what is going to happen with any further interaction with that tier.  So, you might as well just shut the application down by simply not catching it and letting the top-level exception handler log it, if it wants.  And that leaves you with just the one top-level handler.

Answer 6

Joe,

With regards to data access exceptions, this has been improved in .NET 2.0 in that all data acesss exceptions now derive from DbException, so it is possible to catch to do a catch(DbException) to catch all of these.

On top of what Peter said, it basically comes down to the fact that you can't possible know how to handle all exceptions that get thrown your way now or in future versions of the Framework, so catching exception, logging it and then continuing to run can be a very dangerous thing, as your application could be in an unknown state from which you might not be able to recover.

David

Answer 7

Thanks for your advice, David...

I implemented this solution to attach to these exceptions rather than trying to handle them directly and it works fine...I think it accomplishes the same thing as handling them with code in the catch block.

The code I implemented looks like this:

// --- Create an Exception Handler for Unhandled Exceptions ----------

Application.ThreadException += new ThreadExceptionEventHandler(OnThreadException);

AppDomain currentDomain = AppDomain.CurrentDomain;

currentDomain.UnhandledException += new UnhandledExceptionEventHandler(OnUnhandledException);

//Exception Handlers:

private static void OnThreadException(object sender,

ThreadExceptionEventArgs t)

{

Exceptions.HandleError(t.Exception);

}

 

private static void OnUnhandledException(object sender,

UnhandledExceptionEventArgs args)

{

Exceptions.HandleError(args.ExceptionObject as System.Exception);

}

Thanks for your help...

Chuck

PS Can someone please tell me how to post code samples like the above so that it comes out formatted neatly?  I haven't been able to figure out how to do that and its not in the FAQs.

Answer 8

You can paste code samples by doing the following:

1. Copy from Visual Studio
2. Paste into notepad
3. Copy from notepad
4. Paste into the forums
5. Surround with the following tags (remove the spaces)

[code language="c#" ]

[ /code]

Answer 9

> all data acesss exceptions now derive from DbException
Not all.  E.g. ConstraintException derived from DataException not DbException.  And what's to stop the data access layer using XML files for persistence?

> ... so catching exception, logging it and then continuing to run can be a very dangerous thing, as your application could be in an unknown state from which you might not be able to recover.

Surely try/finally should be used throughout the application to make sure it is in a known state. 

I really don't see what risks there are that make continuing a "very dangerous thing".  In many business applications, the only thing an application could do which is potentially dangerous is to persist invalid data (e.g. to a database) - I really don't see how this will happen as a result of catching an Exception.

Of course there are some exceptions (like ExecutionEngineException) that there is no point in catching - and .NET 2.0's new FailFast method is an improvement here.  I guess in .NET 1.1 we would ideally have a list of fatal exceptions which should not be caught in a top-level handler:

try
{
  ...
}
catch(Exception ex)
{
    LogException(ex);
    if (IsFatalException(ex)) throw;
    MessageBox...
}

 

Answer 10

Joe, the problem is what is a "fatal" exception now (in 2.0, or in 1.1) and what is a fatal in future versions?

You just can't know.  You can know--when you write your code--what exceptions you can handle (of the exceptions that are likely to be thrown).  This means not catching "Exception" in anything but the top level handler.

In a WinForms application, a top level handler could be something like this:

[STAThread] static void Main() {  try  {   Application.EnableVisualStyles();   Application.SetCompatibleTextRenderingDefault(false);   Application.Run(new Form1());  }  catch (Exception e)  {   System.Diagnostics.Trace.WriteLine(e.ToString());  }  // application exits here }

Even that isn't guaranteed to work all the time (StackOverflowException, OutOfMemoryException, permission exceptions, etc. will influence whether the call to WriteLine will succeed); but, you at least offer the ability to log all uncaught exceptions where possible.

Other than cross-thread or cross-AppDomain exceptions, that should be the only place you see catch (Exception).

And yes, this requires knowing what exceptions a method is likely throw back to you and deciding what is handle-able.

Yes, you should be using try/catch/finally to *try* and ensure your application is a known state; but, there are circumstances outside of your control where you can't do that. Arbitrarily catching all exceptions without knowing any detail of what caused the exception is an excellent example of that.  There are all sorts of exceptions that could be thrown that can only really be handled by terminating the application: StackOverflowException, AccessViolationException, OutOfMemoryException, TypeLoadException, etc., etc.

If you catch "Exception" you're basically guaranteeing that, under certain circumstances, the user will be continuously barraged by warning messages and manually have to abort the program--e.g. StackOverFlowException.

Answer 11

In .NET 2.0 they have changed the way that the runtime handles StackOverflowException and most OutOfMemoryException exceptions in that they now just teardown the process so you won't be able to catch these (which is a good thing as you can't recover from them).

However, other unknown exceptions should be treated the same, they could be the sign of a bug, or a system failure that you won't be able to recover from. Catching all exceptions and effectively ignoring them, may only cause to delay the time when your application will fail - perhaps during through a bank account transfer, or halfway through a heart operation, etc

The rule is - if you don't know how to recover from it, don't catch it.

 

 

Answer 12

> However, other unknown exceptions should be treated the same, they could be the
> sign of a bug, or a system failure that you won't be able to recover from. Catching all
> exceptions and effectively ignoring them, may only cause to delay the time when your
> application will fail - perhaps during through a bank account transfer, or halfway
> through a heart operation, etc

Consistency in a bank account transfer is handled using transactions, and has nothing to do with exception handling.  And if something minor goes wrong during an operation on my heart I'd prefer the robot surgeon to make an attempt to sew me up again rather than just leaving me on the operating table.

> The rule is - if you don't know how to recover from it, don't catch it.

NIce in theory, but the reality is that in very many cases, the caller does not and can not know what exceptions may be thrown. 

For example, if you call a provider-based API such as Membership.CreateUser (.NET 2.0), you have no way of knowing what provider is configured, so no way of knowing what exceptions may be thrown.  But this doesn't mean the only alternative is for the application to fail.

 

Answer 13

Maybe it was just a bad example; but, why would you want to catch anything other than MembershipCreateUserException when calling Membership.CreateUser()?

You have to know what types of exceptions can be thrown in order to catch them.

The other side effect of using catch(Exception) is that you circumvent layered exception handling (circumventing any handlers higher in the chain).  For example, if you have a method that knows about EndOfStreamException and can compensate by putting the application in a "known" state and catches EndOfStreamException, but makes a method call that eventually makes its way to a method that has catch(Exception) in it, that method now cannot put the application in that "known" state is is forced into an unknown state.

Answer 14

> Maybe it was just a bad example; but, why would you want to catch anything other than MembershipCreateUserException when calling Membership.CreateUser()?

Because I don't want my application to exit just because, say, the Membership database is unavailable.  And since I don't know what provider is configured, I don't know what exceptions I may get (it could be SqlException if a SqlMembershipProvider is configured).

The bottom line is that unless the exceptions which can be thrown are part of an interface contract, the caller can't know what exceptions to catch.

This is particularly true in a loosely-coupled design, such as the provider-model pattern, or when calling via interfaces or virtual methods. 

> The other side effect of using catch(Exception) is that you circumvent layered exception handling...
I agree with this, I would generally only catch general exception types in top-level han dlers.

Answer 15

> You can paste code samples by doing the following:

Great, I asked this on the "Suggestions" forum ages ago, but had no response.  This info really does need to go into the FAQ.

Answer 16

How to handle the Exception between UI layer & BL?  Posted on:  07/14/2006 04:50:25 
    

 

I have some confussion.

I am following the below steps. Can any one say is it good or bad? If bad then how to do it?

1) I UI Layer if any error comes I trap it in to catch block.
2) In catch block I am passing the exception into the business layer method
  (The business layer method will write into the Event Viewer)

I hope
When we user Business layer, it will write the error it into the event viewer of server
If we use UI Layer it will write the error in to the client machine

So give me a good/better way of approching the exception handling.

my sample code is like this

UI Layer

try

{

  i=1;j=0;

  k=1/0;

}

catch(Exception Ex) 

{

   LogFileUsingbusinessLayer(Ex);

}

BL

LogFileUsingbusinessLayer(Exception x)

{

   //writing into the log file.

}

Thanks in advance.

 

Answer 17

rajaram From Hyd wrote:

I am following the below steps. Can any one say is it good or bad? If bad then how to do it? 1) I UI Layer if any error comes I trap it in to catch block. 2) In catch block I am passing the exception into the business layer method   (The business layer method will write into the Event Viewer)

It's best to ask new or related questions in a new thread.  Asking a new question by adding a post to an existing, answered, thread will usually get lost.

To answer your question, which is one of the most misunderstood aspects of modern programming:

In your exception processing you should avoid calling other methods that could throw exceptions in normal circumstances.

It's been pointed out in this thread already, but I'll point it out again.  You shouldn't catch general exceptions in any exception handler but the top-level (last-chance) handler.  This is a handler that performs logging before passing the exception on to the CLR to terminate the application.  This handler, in a WinForms app., usually wraps everything in the Main method, performs some logging, then rethrows the exception. e.g.

[STAThread]
static void Main()
{
   try
   {
      Application.EnableVisualStyles();
      Application.SetCompatibleTextRenderingDefault(false);
      Application.Run(new MainForm());
   }
   catch (Exception e)
   {
      System.Diagnostics.Trace.WriteLine(e.ToString());
      throw; // rethrow
   }
}

In your case you're actually handling all exceptions, not just catching them, hiding many bugs as a result.  For example, if a OutOfMemoryException occurred, you would try to log it (which would probably cause more exceptions) then continue running the application.  There's really no way the application can continue in circumstances like this, that basically just causes the application to appear to hang while it fills up your log.  Not a good user experience.

It may seem easier (there's lots of things you can do in design/implementation that seem easier but really make for a less reliable, less robust, application) to simply catch(Exception); but, that just hides bugs and doesn't offer any real value.

Answer 18

"And if something minor goes wrong during an operation on my heart I'd prefer the robot surgeon to make an attempt to sew me up again rather than just leaving me on the operating table"

Of course, that's an exception type that the surgical team could recover from. If they get a PowerOutageException, then can handle that by invoking the StartBackupPower() method. If they get the HeartStoppedException, they can go through a list of known procedures to try and recover from that failure. But if the AllSurgeonsSuddenlyDieException...well, you're kind of screwed at that point, right?

My point is that if your application does "something wrong", you may be able to recover from that if you know what that "something wrong" is. But there's no way you can gracefully recover from something when you really don't know what it is. That's why having a generic exception handler without rethrowing the exception is really, really bad idea.

Regards,

Jason Bock

http://www.jasonbock.net