Common programming mistakes in .Net when handling exceptions?

Question

What are some of the most common mistakes you've seen made when handling exceptions?

It seems like exception handling can be one of the hardest things to learn how to do "right" in .Net. Especially considering the currently #1 ranked answer to Common programming mistakes for .NET developers to avoid? is related to exception handling.

Hopefully by listing some of the most common mistakes we can all learn to handle exceptions better.

Answer 1

Empty catch:

//What's the point?
catch()
{}

Rethrowing:

//Exceptions are for *adding* detail up the stack
catch (Exception ex)
{throw ex;}

Answer 2

Re-throwing exceptions like this:

try 
{ 
   // some code here
}
catch(Exception ex)
{
   // logging, etc
   throw ex;
}

This kills the stack trace, making is far less usable. The correct way to rethrow would be like this:

try 
{ 
   // some code here
}
catch(Exception ex)
{
   // logging, etc
   throw;
}

Answer 3

Catching all exceptions when in many cases you should attempt to catch specific exceptions:

try {
  // Do something.
} catch (Exception exc) {
  // Do something.
}

Rather than:

try {
  // Do something.
} catch (IOException exc) {
  // Do something.
}

Exceptions should be ordered from most specific to least.

Answer 4

Not using using on IDisposable objects:

File myFile = File.Open("some file");
callSomeMethodWhichThrowsException(myFile);
myFile.Close();

myFile does not get closed until myFile's finalizer is called (which may be never) because an exception was thrown before myFile.Close() was called.

The proper way to do this is

using(File myFile = File.Open("some file"))
{
    callSomeMethodWhichThrowsException(myFile);
}

This gets translated by the compiler into something like:

File myFile = File.Open("some file");
try
{
    callSomeMethodWhichThrowsException(myFile);
}
finally
{
    if(myFile != null)
        myFile.Dispose(); //Dispose() calls Close()
}

So the file gets closed even in the face of exceptions.

Answer 5

Wrong

try
{
   // Do something stupid
}
catch
{
   // ignore the resulting error because I'm lazy, and later spend
   // a week trying to figure out why my app is crashing all over
   // the place.
}

Better

try
{
    /// do something silly.
}
catch (InvalidOperationException ex)
{
    /// respond, or log it.
}
catch (Exception e)
{
    /// log it.
}

Answer 6

Rethrowing an exception with a meaningless message.

try
{
    ...
}
catch (Exception ex)
{
   throw new Exception("An error ocurred when saving database changes").
}

You won't believe how often I see code like this running in production.

Answer 7

Assuming an exception that covers many scenarios was something specific. A real life scenario was a web app where the exception handling always assumed all errors were session time outs and logged and reported all errors as session time outs.

Another example:

try
{
     Insert(data);
}
catch (SqlException e)
{
   //oh this is a duplicate row, lets change to update
   Update(data);
}

Answer 8

Forget to set the inner exception when rethrowing a catched exception

try
{
    ...
}
catch (IOException ioException)
{
    throw new AppSpecificException("It was not possible to save exportation file.")
    // instead of
    throw new AppSpecificException("It was not possible to save exportation file.", ioException);
}

When I posted this answer, I forget to mention that we should always consider when to include inner exception or not due to security reasons. As Eric Lippert pointed out on another answer for this topic, some exceptions can provide sensitive information about the implementation details of the server. Thus, if the caller who will be handling the exception is not trusted, it is not a good idea to include the inner exception information.

Answer 9

To log Exception.Message instead of Exception.ToString()

Many times, I see code logging only the exception message while it should log the return of ToString method. ToString provides much more information about the exception than the Message. It includes information like inner exception and stack trace besides the message.

Answer 10

Trying to catch OutOfMemoryException or StackOverflowException - those lead to a shutdown of the runtime, hence to way to catch them from within the same Process (or even from the CLR as a whole?)

OutOfMemoryException: The exception that is thrown when there is not enough memory to continue the execution of a program.

"Starting with the .NET Framework version 2.0, a StackOverflowException object cannot be caught by a try-catch block and the corresponding process is terminated by default. Consequently, users are advised to write their code to detect and prevent a stack overflow."

Answer 11

What are some of the most common mistakes you've seen made when handling exceptions?

I can think of lots.

First read my article on categorization of exceptions into vexing, boneheaded, fatal and exogenous:

http://blogs.msdn.com/ericlippert/archive/2008/09/10/vexing-exceptions.aspx

Some common errors:

• Failure to handle exogenous exceptions.
• Failure to handle vexing exceptions.
• Construction of methods that throw vexing exceptions.
• Handling exceptions that you actually cannot handle, like fatal exceptions.
• Handling exceptions that hide bugs in your code; don't handle a boneheaded exception, fix the bug so that it isn't thrown in the first place

Security error: failure to the unsafe mode

try
{
  result = CheckPassword();
  if (result == BadPassword) throw BadPasswordException();
}
catch(BadPasswordException ex) { ReportError(ex); return; }
catch(Exception ex) { LogException(ex); }
AccessUserData();

See what happened? We failed to the unsafe mode. If CheckPassword threw NetworkDriverIsAllMessedUpException then we caught it, logged it, and accessed the user's data regardless of whether the password was correct. Fail to the safe mode; when you get any exception, assume the worst.

• Security error: production of exceptions which leak sensitive information, directly or indirectly.

This isn't exactly about handling exceptions in your code, it's about producing exceptions which are handled by hostile code.

Funny story. Before .NET 1.0 shipped to customers we found a bug where it was possible to call a method that threw the exception "the assembly which called this method does not have permission to determine the name of file C:\foo.txt". Great. Thanks for letting me know. What is stopping said assembly from catching the exception and interrogating its message to get the file name? Nothing. We fixed that before we shipped.

That's a direct problem. An indirect problem would be a problem I implemented in LoadPicture, in VBScript. It gave a different error message depending upon whether the incorrect argument is a directory, a file that isn't a picture, or a file that doesn't exist. Which means you could use it as a very slow disk browser! By trying a whole bunch of different things you could gradually build up a picture of what files and directories were on someone's hard disk. Exceptions should be designed so that if they are handled by untrustworthy code, that code learns none of the user's private information from whatever they did to cause the exception. (LoadPicture now gives much less helpful error messages.)

• Security and resource management error: Handlers which do not clean up resources are resource leaks waiting to happen. Resource leaks can be used as denial-of-service attacks by hostile partial trust code which deliberately creates exceptions-producing situations.

• Robustness error: Handlers must assume that program state is messed up unless handling a specific exogenous exception. This is particularly true of finally blocks. When you're handling an unexpected exception, it is entirely possible and even likely that something is deeply messed up in your program. You have no idea if any of your subsystems are working, and if they are, whether calling them will make the situation better or worse. Concentrate on logging the error and saving user data if possible and shut down as cleanly as you can. Assume that nothing works right.

• Security error: temporary global state mutations that have security impacts need to be undone before any code that might be hostile can run. Hostile code can run before finally blocks run! See my article on this for details:

http://blogs.msdn.com/ericlippert/archive/2004/09/01/224064.aspx

Answer 12

Nobody is talking about seeing empty catch blocks like these....

 try{  
      //do something
    }
catch(SQLException sqex){  
        // do nothing  
    }

Also never use Exception handling for creating alternate method flows...

 try{  
     //do something  

 }catch(SQLException sqex){  

     //do something else  
 }

Answer 13

Using exceptions for normal flow control. Exceptions should exceptional. If it's a good / expected operation, use return values, etc.

Answer 14

Failing to catch possible exceptions inside a catch handler. This can cause the wrong exception to be propagated upwards.

For example:

try
{
    DoImportantWork();
}
catch
{
    Cleanup();        
    throw;
}

What happens if Cleanup() throws an exception? You don't want to see an Exception pointing to the Cleanup() method in this catch handler. You want the original error. You could try to log the cleanup error, but even your logging code needs exception handling to avoid throwing exceptions from it.

try
{
    DoImportantWork();
}
catch
{
    try
    {
        Cleanup();        
    }
    catch
    {
        // We did our best to clean up, and even that failed.
        // If you try to log this error, the logging may throw yet another Exception.
    }
    throw;
}