There’s something I ran into recently that even experienced programmers can
get wrong. We were adding error handling to something, and I saw this in the code:
catch (Exception e)
{
LogError(e);
throw e;
}
The idea being that we log all errors in our components, but then throw them again
to let the higher level systems actually figure out what to do. This works well. The
problem comes in with the throw – it actually works much better to do this:
catch (Exception e)
{
LogError(e);
throw; // Rethrows the exception we just caught
}
Notice the absence of an argument to the throw statement in the second variation.
The difference between these two variations is subtle but important. The second variation
is the only one that actually re-throws the original exception. Meaning that
the exact same information is passed along up the stack. With the first variation,
the higher level caller isn’t going to get all the information about the original
error. For example, the call stack in the exception is replaced with a new call stack
that originates at the “throw e” statement – which is not what we
want to record.
Doh!
ReplyDeleteI hope that's a "D'oh! I did it all the wrong way but now I know!" and not a "D'oh! You got it all wrong, Craig!"
ReplyDelete:)
In my case, it's a "D'oh. Why the sam hill didn't I know this? Thanks, Craig".
ReplyDeleteThanks, Craig.
We aim to please. ;)
ReplyDeleteDon't feel bad about not knowing: I ran into two very senior developers who didn't know either.
I didn't know this - good stuff like this makes reading blogs worthwhile :)
ReplyDeleteThanks!
ReplyDeleteIf you are unable to directly deal with an exception, but you have context about the error you want to add in a catch block, I find it best to do this:
ReplyDeletecatch(Exception ex)
{
throw(new Exception("my additional error context here, maybe some local variable values etc.",ex));
}
Here, we have put the original exception as the inner exception of the newly created exception. Then, at the point where we are "really" going to deal with the exception, you do all your logging in one spot (walking down through the inner exceptions.) Then you get one log entry for the original exception, rather than entries at every point you happen to catch. You still have all of the stack traces availble to you, albeit split among the exceptions.
Of course, this is only workable if you are comfortable changing the type of the exception. (I used System.Exception above when creating a new one, but you could use any type you like.) But you often are willing to change the type - you might want to turn a low-level exception into something more relevant to the API you are exposing, etc.
I think that's a great approach. Aside from not wanting to change the type of the exception, there's another good reason for rethrowing: an exception in an "inner" component may be an error, where it might only be a warning or a trace message. If you're using (as we are) a framework like EIF that lets you filter messages by component, this lets you set it up so you can ignore the exception as caught by the outer code, but still log it to whatever is monitoring the inner code that caught the exception first.
ReplyDeleteThis might be a bit of a kludge, but I think I might want to change the stack trace. Please tell me if there is a better way of doing this, but we prefer to log all Exceptions in EventHandlers so that we don't end up with code resuming after an exception is handled, we know the "real-world" source of the error (all errors are the User's fault, right? ~wink~), etc.
ReplyDeleteI've been wrestling with TargetSite.ReflectedType and other properties just to get the name of the EventHandler that does the final catch into our log, but Craig's insight into the shortened StrackTrace just may be the ticket. Here is what I'm after:
---
public void MisDirect1()
{ MisDirect2(); }
public void MisDirect2()
{ MisDirect3(); }
public void MisDirect3()
{
System.Data.SqlClient.SqlConnection conn = new System.Data.SqlClient.SqlConnection();
conn.Open();
conn.Close();
}
private void button1_Click(object sender, System.EventArgs e)
{
try
{
try
{ MisDirect1(); }
catch( Exception ex0 )
{
Ceoimage.Basecamp.Functions.EventLog.Debug( ex0.StackTrace );
throw ex0;
}
}
catch( Exception ex1 )
{ Ceoimage.Basecamp.Functions.EventLog.Debug( ex1.StackTrace ); }
}
---
Does this make sense?
Sorry, I don't quite follow. You want the name of the method that's doing the catch? But you want to figure it out inside the call to your Debug method? Why not just pass the method name in? Otherwise, if you pass in the exception, you should be able to do something like this:
ReplyDeleteprivate void SayCatchMethodName(Exception ex) {
Console.WriteLine(ex.StackTrace.GetFrame(0).GetMethod.Name);
}
Does that do what you want? It pulls the topmost stack frame off the stack trace and prints the name of the method it corresponds to.
Ah. I think I understand what you're after now. I'd probably change the code you have to have a single try-catch block, where the catch block calls your EventLog.Debug method much as it does not, but I'd change EventLog.Debug to take in the name of the method doing the catch. This trivial, as you can use the MethodInfo.GetCurrentMethod().Name to find out the currently executing method name, making it easy to cut and paste your code around.
ReplyDeleteI don't think I'd do what you're doing: your error handling is the last place you want to try to get clever.
Point well taken. I'm not really doing that, just considering it based on the point you brought up in this post.
ReplyDeleteAnd thanks for the MethodInfo tip. There's just so much out there to keep track off ...
There is indeed much to keep track of...for example how to spell "now" (I had it "not") and how to form complete sentences (e.g. "This trivial" instead of "This is trivial") :)
ReplyDeleteBut yes, the class libraries are gigantic.
That's cool! ... but can you do the throw outside of the catch block and have it still be valid? IE:
ReplyDeletecatch (Exception e)
{
LogError(e);
}
throw; // Rethrows the exception we just caught
Without actually trying it (which you should), I'd say, "No, you can't do that."
ReplyDeleteCraig,
ReplyDeleteI have opposite question:
How to stop the exception being further bobbled up?
Thanks
Catch it and don't rethrow it.
ReplyDeleteE.g.
catch (Exception ex)
{
LogException(ex);
}
(You should probably be logging exceptions if you're not going to rethrow them.)
In my experience, there is almost NEVER a good reason to catch a general Exception type, and even fewer reasons to not rethrow it. If you aren't catching a specific exception type, (e.g. SqlException), you don't know WHAT the problem is. How can you expect to handle it? Worse, you can ignore it. There are some exceptions that you just cannot handle gracefully, such as an OutOfMemoryException. Think very hard before you catch and ignore a general Exception. You will pay for it when it comes time to debug because you wont know that there was a problem until your code stops working and you are forced to look at your log files. Do you really want to be bound to reading your log files every day just to check for exceptions?
ReplyDeleteThe rethrow does not retain the *exact* original stack trace: It modifies the top call frame. Code to demonstrate is below:
ReplyDeleteusing System;
public class C
{
public static void Main(string[] args) {
try
{
Rethrow();
}
catch (Exception e)
{
Console.Out.WriteLine("Rethrown exception is " + e);
// output original stack trace except the Rethrow() stack frame frame now indicates the line where the exception was rethrown rather than the line where the exception was thrown
}
Console.ReadKey();
}
static void Rethrow()
{
try
{
throw new Exception();
}
catch (Exception e)
{
Console.Out.WriteLine("Original exception is " + e); // outputs original stack trace
throw;
}
}
}
I didn't know that! thanks! :)
ReplyDeleteI have a question, have you ever encountered a case in which you have a specific catch block followed by an Exception catch block, when you rethrow the exeption you just recieved in the specific catch block the Exception catch block can't catch the exception, or example like this code:
public class Excp{
public static void main(String[] args) throws Exception{
String s ="abc";
int k= 0; int j;
try {
if (k == 0) j = 10/k;
}
catch (ArithmeticException ex) {
System.out.println("arithmetic error");
throw ex;
}
catch (Exception e) {
System.out.println("Error");
} finally {
System.out.println("in finally clause");
}
System.out.println("after try block");
System.out.println(s);
}}
"error" message in Exception catch block doesn't print! it really surprised me...
-S
It doesn't print because if you have multiple exception-catching blocks, the semantics are that only the first matching block is executed. So the throw continues outside the context of the try/catch/finally block. You'd need to use nested try statements to do what you're trying to do here.
ReplyDeleteAlso, your code looks to be a weird mix of Java and C#. Are you trying to figure out how C# works, or how Java works?