.NET and me Coding dreams since 1998!

1Apr/075

Gotcha – Improper exception handling

I was reading today a document written by Juval Lowy from IDesign "C# Coding standard - Guidelines and Best Practices V2.2" and under No 17 rule in Coding Practices I found next thing:

17. In a catch statement that throws an exception, always throw the original exception (or another exception constructed from the original exception) to maintain the stack of original error:

catch (Exception exception)
{MessageBox.Show(exception.Message);

throw exception;
}

I think this definitely is not a best practice, and I'll try to explain why:

throw ex

The problem this practice tries to solve is that in case of rethrowing of exception in catch block stack pointer moves from the original exception line to line of rethrowing of exception

   1: using System;
   2: namespace ThrowExample
   3: {
   4:     internal class Program
   5:     {
   6:         public static void ThrowException()
   7:         {
   8:             throw new NotImplementedException("Exception thrown!"); //Line 8
   9:         }
  10:  
  11:         private static void Main()
  12:         {
  13:             try
  14:             {
  15:                 ThrowException(); //Line 15
  16:             }
  17:             catch (Exception e)
  18:             {
  19:                 Console.WriteLine(e.Message);
  20:                 throw e; //Line 20
  21:             }
  22:         }
  23:     }
  24: }

Stack result would be

"Exception thrown!"

at ThrowExample.Program.Main() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 20

at System.AppDomain.nExecuteAssembly(Assembly assembly, String[] args)
...

That's why IDesign suggest MessageBox printing of stack trace before it is been destroyed, but this is bad practice because it is:

  • Approach tied to specific UI technology (what with ASP NET app?)
  • Disables loging of exception through Log4Net  or EntLib block
  • Relies on developer to read the message and makes some action (when he clicks ok button all traces of the exception log are disappearing)

throw;

Instead of MessageBox printing the proper way is to use throw; call which doesn't destroy stack trace.

So our example. enhanced with throw; would be:

   1: using System;
   2: namespace ThrowExample
   3: {
   4:     internal class Program
   5:     {
   6:         public static void ThrowException()
   7:         {
   8:             throw new NotImplementedException("Exception thrown!"); //Line 8
   9:         }
  10:  
  11:         private static void Main()
  12:         {
  13:             try
  14:             {
  15:                 ThrowException(); //Line 15
  16:             }
  17:             catch
  18:             {
  19:                 Console.WriteLine("Exception happened");
  20:                 throw; //Line 20
  21:             }
  22:         }
  23:     }
  24: }

and then stack would be

"Exception thrown!"

at ThrowExample.Program.ThrowException() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 8

at ThrowExample.Program.Main() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 20

at System.AppDomain.nExecuteAssembly(Assembly assembly, String[] args)
...

no try - catch

But there is one more problem: In "throw;" case of we are not seeing in stack trace line 15 which we would see in case we didn't have try-catch-block.

   1: using System;
   2: namespace ThrowExample
   3: {
   4:     internal class Program
   5:     {
   6:         public static void ThrowException()
   7:         {
   8:             throw new NotImplementedException("Exception thrown!");
   9:         }
  10:  
  11:         private static void Main()
  12:         {
  13:             /*            try
  14:             {*/
  15:             ThrowException();
  16:             /*}
  17:             catch
  18:             {
  19:                 //Console.WriteLine(e.Message);
  20:                 throw; //Line 20
  21:             }        */
  22:         }
  23:     }
  24: }

and then stack would be

"Exception thrown!"

at ThrowExample.Program.ThrowException() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 8

at ThrowExample.Program.Main() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 15

at System.AppDomain.nExecuteAssembly(Assembly assembly, String[] args)
...

The way how we can see all of the important stack exception data is next one:

throw ex + inner exception

   1: using System;
   2: namespace ThrowExample
   3: {
   4:     internal class Program
   5:     {
   6:         public static void ThrowException()
   7:         {
   8:             throw new NotImplementedException("Exception thrown!"); // Line 8
   9:         }
  10:  
  11:         private static void Main()
  12:         {
  13:             try
  14:             {
  15:                 ThrowException(); // Line 15
  16:             }
  17:             catch (Exception e)
  18:             {
  19:                 Console.WriteLine(e.Message);
  20:                 throw new Exception("Some message", e); //Line 20
  21:             }
  22:         }
  23:     }
  24: }

the main stack result would be

"Exception thrown!"

at ThrowExample.Program.Main() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 20

and the inner exception stack trace would be

at ThrowExample.Program.ThrowException() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 8
at ThrowExample.Program.Main() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 15

So only on this way we could get all the data needed

 

Therefore, I believe that only the last approach (maybe the throw; approach could be ok)  is the approach which deserves to be described as best practice

Don't you think so?

Technorati tags: , , , ,

Share this post :

Comments (5) Trackbacks (0)
  1. Nice explanation, there are many devs that don't know the real and useful information of the stack…

  2. I think that "throw;" is your best option if you don't have any additional data to add to your exception. If you do have data, then passing the old exception as an inner exception is the best way to go. They are both "best practices" for their respective situations. Which is why I hate the term "best practice", it makes it sound way too much like a universal solution. But I digress.

    Also, you should point out that raising the type "Exception" is not best practice and it should always be subclassed, and preferably from the ApplicationException class if you are really being anal about it. :-)

  3. I always laugh when I see people discussing best practices and then do something like throw a System.Exception object.

    However, I agree with you. If you must catch and throw, wrap a base exception in another exception type and throw that. Primarily because people up the call chain won't have to attempt to catch twenty thousand different types of exceptions.

  4. Justin,

    I agree 100% that raising the Exception type of exception is totally wrong practice but IMHO using of ApplicationException is equally bad and I am sure I read it on couple of places not recomended any more from MS.

    What we should use is either some specific exception which match the exception case scenario such are InvalidOperationException, ArgumentException etc or our own custom exceptions.

    I am really glad that FXCop reports an issue with usage of Exception, which is very useful in answering the "why not just use Exception" :)

    mcgurk,

    I agree it is funny and small thing but I spent an descent amount of time hunting bugs in cases where the stack was destroyed or in even worse swallowing catch {} case (which btw makes me bloodthirsty :) )

  5. FYI:
    System.Exception has a property "Data" (type IDictionary) (new in .NET 2.0). Thus it is no longer necessary to provide a derived exception class "only" to add some piece of information.

    For me the reasons to wrap an exception are usually a) to provide a better suited exception class to _describe_ the error or b) to provide better destinction of errors for code that _handles_ the error (i.e. catch-handlers). Providing the inner exception is of course mandatory…


Leave a comment

No trackbacks yet.