Too much reuse
A recent user question:
I have code that maintains a queue of pending work items waiting to be completed on various different worker threads. In certain unfortunate fatal error situations I complete each of these by throwing an exception. Can I create just one exception object? Are there any issues throwing the same exception object multiple times on multiple threads?
Anyone who has ever seen this in a code review knows the answer:
catch(Exception ex)
{
Logger.Log(ex);
throw ex;
}
This is a classic “gotcha”; almost always the right thing to do is to say “throw;” rather than “throw ex;” – the reason being that exceptions are not completely immutable in .NET. The exception object’s stack trace is set at the point where the exception is thrown, every time it is thrown, not at the point where it is created. The “throw;” does not reset the stack trace, “throw ex;” does.
Don’t reuse a single exception object. Every time gets thrown the stack trace will be reset, which means that any code up the stack which catches the exception and logs the trace for later analysis will almost certainly be logging someone else’s trace. Making exceptions is cheap, and you’re already in a fatal error situation; it doesn’t matter if the app crashes a few microseconds slower. Take the time to allocate as many exceptions as you plan on throwing.
Comments
Anonymous
March 03, 2010
I know a lot about exceptions and exception handling, but this was actually new to me. Thank you.Anonymous
March 03, 2010
Would it be a good idea to create a new exception and assign the caught exception to InnerException?Anonymous
March 03, 2010
I have seen this pattern more times than I can count, and it is a mistake that is made very often even by good, experienced, knowledgeable developers. I think a lot of people don't even realize that catch(Exception){ throw; } is valid syntax. The similarity of the C# throw statement to Java probably doesn't help since java requires the "throw somethingThrowable" syntax. Also, people become so accustomed to writing catch(Exception e){ throw new Exception("Uh oh!", e); } that they assume they must "throw anException;". It makes me wonder if there is something that the compiler (or some other verification tool) could do to make the correct pattern more obvious. Since it is perfectly valid code and could very well be intended, a warning doesn't really make sense. I know new syntax is definitely low on the list of things to be considered, but the source of the confusion seems to be two behaviors from what is interpreted to be identical code.Anonymous
March 03, 2010
So how expensive are exceptions to throw? Are they cheap enough to use them for nonlocal transfer of control?Anonymous
March 03, 2010
I agree with Chris B that catch(Exception) { throw; } is valid and better than throw ex, but isn't it simply better to not catch the exception at all. Doesn't that do the same thing? In Eric's example it's being logged, so you need the catch, but if you're not doing anything with it, why catch it.Anonymous
March 03, 2010
Again, this is another great candidate for the Messages pane.Anonymous
March 03, 2010
@Gabe... using exceptions (even if they were "Free") for "nonlocal transfer of control" is pure EVIL. In some environments, every exception (even those that are caught) are treated as "support alerts". Under "normal" circumstances the PerformanceCounters for Exceptions should remain at 0. Unfortunately even Microsoft's CLR/BCL violates this principal. Just set "Break on Exception" to true for many application types, and watch how many exceptions you have to step through just during application startup!!!!!Anonymous
March 04, 2010
Brian: There are plenty of reasons why you might want to inspect an exception before possibly rethrowing it. Perhaps you know how to handle some kinds of IOExceptions but not others, or you want to look for a certain type of InnerException. Or you might just want to optionally ignore it, like here: while (true) try { .... } catch (Exception) { if (!retry) throw; }Anonymous
March 04, 2010
@Gabe... understand, but if you're not doing anything with it, don't catch it. Maybe I'm just being pedantic... or not pedantic enough.Anonymous
March 04, 2010
@Brian, I didn't intend the code to be a good example of exception handling, so I omitted the actual handling (logging, wrapping, cleanup, whatever) for brevity. Sorry for not being as clear as I should have.Anonymous
March 04, 2010
@Gabe: Your original question of how cheap they are hasn't been looked at recently from what I could see. http://www.codeproject.com/KB/exception/ExceptionPerformance.aspx http://www.yoda.arachsys.com/csharp/exceptions2.html The second one is actually a better way of describing the situation. "If you ever get to the point where exceptions are significantly hurting your performance, you have problems in terms of your use of exceptions beyond just the performance." Your answer is :"depends."Anonymous
March 04, 2010
"it doesn’t matter if the app crashes a few microseconds slower" This should go on a t-shirt or something.Anonymous
March 04, 2010
A little off topic but I’ll ask anyway. I sometimes catch an exception in a utility method and then re ‘throw;’ it, but before rethrowing it I use another aspect of an exceptions ‘mutability’ the ‘Data’ property. I add local contextual information to it. My base exception handler logs all the information it can about the exception i.e. message, stack, type, innerexception etc Plus the Data property. Is this a good idea? Do others use this pattern? Does anyone out there use Exception.Data at all? private void ExecuteSqlExample(string sql) { try { //To stuff here //....... } catch (Exception ex) { ex.Data["Sql"] = sql; throw; } }Anonymous
March 04, 2010
Unfortunately, Microsoft documentation does not help the situation. Below is the documentation in the try-catch section of Visual Studio C# MSDN Library: A throw statement can be used in the catch block to re-throw the exception, which has been caught by the catch statement. For example: catch (InvalidCastException e) { throw (e); // Rethrowing exception e }Anonymous
March 04, 2010
One extension method we have at my office is this public static TException Rethrow<TException>(this TException exception) where TException : Exception { var field = typeof(Exception).GetField("_remoteStackTraceString", BindingFlags.Instance | BindingFlags.NonPublic); field.SetValue(exception, exception.StackTrace); return exception; } This allows us to do throw ex.Rethrow(); without reseting the stack. This is useful if we hand an exception off to a function that decides whether it should be rethrown.Anonymous
March 04, 2010
The comment has been removedAnonymous
March 04, 2010
@Banjobeni....Changing where the strack trace is captured would be a major breaking change. Also there are times where you want to build up a (potential) exception, and throw it at a fairly distant point. @Eric, What I WOULD like to see is a better mechanism of getting information about the CallStack WITHOUT Exceptions being involved at all....but that is another topid..Anonymous
March 04, 2010
Robert: the same effect can be achieved without reflection, FWIW static void PreserveStackTrace (Exception e) { var ctx = new StreamingContext (StreamingContextStates.CrossAppDomain) ; var mgr = new ObjectManager (null, context) ; var si = new SerializationInfo (e.GetType (), new FormatterConverter ()) ; e.GetObjectData (si, ctx) ; mgr.RegisterObject (e, 1, si) ; // prepare for SetObjectData mgr.DoFixups () ; // ObjectManager calls SetObjectData }Anonymous
March 04, 2010
I ask what the difference is between "throw" and "throw ex" as an interview question. No one has answered it correctly yet. It's not the greatest question because someone may have have been using throw and it's not really very important to know what throw ex does. On the other hand I've inherited server-side code with "throw ex" sprinkled liberally throughout and it makes troubleshooting from logs very difficult. Someone who intends to write server-side or widely-deployed client-side code probably should know about this.Anonymous
March 04, 2010
David V. Corbin: Would you be satisfied if the CLR provided an ExceptionLite, which is like an exception only it doesn't capture the stack frame (for performance) and doesn't break into your debugger or increment the "exceptions" counter? Because I'd like to be able to write the following without it being so annoying: class NonlocalReturn : ExceptionLite { public object Value; public NonlocalReturn (object value) { Value = value; } } void Return(object value) { throw new NonlocalReturn(value); } int FirstItemOverX(int x, IList<int> someList) { try { someList.ForEach(i => { if (i > x) Return(i); }); } catch (NonlocalReturn ret) { return ret.Value; } throw new NotFoundException(); }Anonymous
March 04, 2010
David V Corbin: Stack Trace info can be retrieved via System.Diagnostics.StackTrace class, unfortunately, this relies A LOT on VS's pdb files being in place and the class doesn't mesh well with System.Reflection. Which is unfortunate.Anonymous
March 04, 2010
Like many others I'm surprised by this behaviour. While the feature is good in the sense that the shorter syntax is the one that should be used more often, the discoverability is extremely low.Anonymous
March 04, 2010
@Robert [please feel free to contact me off-list david.corbin@dynconcepts.com] - I am aware of System.Diagnostics.StacckTrace, unfortunately I am looking for something the is ReflectionFriendly [i.e. based on MetaDaa, NOT PDB]Anonymous
March 04, 2010
The comment has been removedAnonymous
March 04, 2010
By the way, on the subject of catching to log the exception, I'm pretty sure in BCL 4 the default recommended config will be that you are unable to catch various kinds of fatal exception. The CLR will simply let them fly straight through the try/catch block, invisibly, even if you write "catch (Exception ex)". So that won't even work as a way of logging many fatal exceptions. So if you want to log them, you'll have to use AppDomain.UnhandledException. (This may be out-of-date, depends what has happened since I read about this change last year).Anonymous
March 04, 2010
Regarding "catching for logging" of exceptions you can not really "deal with" is definately a dangerous practice. However, it is often useful (expecially for placing a breakpoint to see local [outside of try block) state). My approach is to have a "CHECKED" Build Configuration so that I can have this ability, but ONLY when needed.Anonymous
March 04, 2010
The comment has been removedAnonymous
March 04, 2010
Why on earth would I not want my finally blocks to run? Isn't that the whole point of writing them in the first place -- to make sure that everything is in a known or consistent state even in exceptional cases? When my app crashes in the middle of an operation, I want it to rollback the transaction, unlock the record, and release the semaphore. I simply can't imagine how it would ever be a good idea to leave a mess all over the system to be manually cleaned up just because something timed out or ran out of memory when nobody expected it to. Furthermore, I don't understand the advice about exception filtering. Am I just not supposed to write my code in C# when I need to handle an exception in only certain cases? Should I be using some separate VB.Net assembly to do that? And this is all to make sure that somebody who doesn't know anything about my code can determine whether my carefully-crafted finally blocks actually run?Anonymous
March 04, 2010
@Gabe +1 Maybe I'm too stupid for this discussion, but the reason a finally block is inserted to have it run in a 'good' and a 'bad' situation... so what's the point the nested finally blocks are called? I understand the discussion about not catching Exceptions, if there isn't a good reason for it, but not to catch it when it might corrupt your peace of code is on my opinion more dangerous. The first 3 advices of the CLR team sounds solid and clear, BUT the 4th sounds more like C# is not prepared for handling exceptions like it should be in a perfect sense - maybe this should be kept in silence, until C# has to offer something in this direction. When I was switching from Java to C#, the missing exception declarations in method level made me feel very uncomfortable - discussions like this reminds me of that. Of course it's a religious question, but personally, I would be more happy to force (optionally) the C# compiler to respect exceptions which might be thrown from a method instead of hoping for a good documentation.Anonymous
March 04, 2010
The comment has been removedAnonymous
March 04, 2010
The comment has been removedAnonymous
March 04, 2010
@Robert Davis: That is a strange way of preserving the stack trace. You can achieve the same by wrapping the original exception, so instead of doing this: throw ex.Rethrow(); I would do something like: throw new InvalidOperationException("Descriptive error message. " + ex.Message, ex);Anonymous
March 04, 2010
@ CuttingEdge, The benefit of tweaking the stacktrace of the original exception is that you can rethrow the exact same exception. By wrapping it like you did you change the type from AnyException into InvalidOperationException. Handlers up the stack hohever might depend on the original type they are trying to catchAnonymous
March 04, 2010
Using throw instead of throw ex is not enough. Unfortunately, throw does not always preserve the stack trace. See http://weblogs.asp.net/fmarguerie/archive/2008/01/02/rethrowing-exceptions-and-preserving-the-full-call-stack-trace.aspxAnonymous
March 04, 2010
Sorry, I hadn't seen someone has already added a comment about that.Anonymous
March 04, 2010
The comment has been removedAnonymous
March 05, 2010
@CuttingEdge: That is certainly the reasonable default practice. But it is not always the best option. Say for example you have a virtual method that in one case uses reflection to invoke some other method. If that method throws an exception, Reflection will wrap it in a TargetInvokationException. If you can determine that it wasn't the reflective code that threw the exception then its probably a good idea to rethrow the inner exception of the TargetInvokationException. The fact that I used reflection is an implementation detail that the caller shouldn't have to care about.Anonymous
March 05, 2010
The comment has been removedAnonymous
March 05, 2010
The comment has been removedAnonymous
March 05, 2010
The comment has been removedAnonymous
March 05, 2010
The comment has been removedAnonymous
March 05, 2010
I remember that Java had two different class trees for the "bad exceptions" (Exception) and the "ugly exceptions" (Error), with Throwable as the superclass for both - anyone know the reason this wasn't followed for .NET?Anonymous
March 05, 2010
Java actually has three kinds of exceptions. Exception, RuntimeException, and Error. Exceptions were designed to be documented and handled (hence the throws clauses), RuntimeExceptons were similar but didn't have to be declared in a throws clause (like divide-by-zero), Errors were for very serious, I'm going to crash now, occurances like Stack Overflow or corrupted program state.Anonymous
March 05, 2010
@Random832... This has already been covered. For "Ugly" situations, you have NO IDEA what the application state may be, or what the effect of executing ANY code will be. While truely random events are much less likely in managed code than in environments like C++, this is still true. The code that is in your finaly block may have been overrwitten, the stack may be corrupted such that and call/return invokes random code [there are actually some techniques to force the system to run malicious code!] Even simple things like floating point calculations bay return incorrect results. ANYTHING IS POSSIBLE. So if you don't have any information about the state of the system, and if you don't have any knowledge as to what the code would actually do.... WHY would you WANT to run it????Anonymous
March 05, 2010
I'm surprised, Eric, that you didn't point out that exceptions are painfully expensive to throw. If someone is worried about the cost of re-using an exception object (easy to create; expensive to throw), then it sure seems like there's probably a fundamental perf problem in the code that's going to be caused by throwing exceptions so regularly. Maybe I'm missing something though... Eric, is it not horribly expensive to throw an exception? I normally go way out of my way to avoid an exception occurring anywhere in my code. In .NET 1.1 it killed me that TryParse didn't exist - it made simple form validation way too expensive if you didn't want to write your own try parse methods that didn't throw exceptions internally. Even today I go out of my way to write my own TryParse routine for GUIDs (in cases where I need to validate them, like if they're coming from a URL). Of course, why the Guid class doesn't follow much the same patterns of Decimal and other classes is a mystery to me; I constantly wish Guid had a Parse and a TryParse (and New Guid() did the same thing as Guid.NewGuid)...Anonymous
March 05, 2010
The comment has been removedAnonymous
March 05, 2010
I guess I'm at a loss over what you are trying to accomplish in finally blocks anyway. For example, I wouldn't commit or rollback and database transaction in finally, commit would be within the try, rollback within the catch. Finally would only essentially only contain resource cleanup, there would be nothing to change data state within that part of the code. To be honest, I've never really used that particular block that often. I catch what I can fix (more accurately, provide notification of a common error I expect to happen, such as a duplicate in a unique index field during a user registration), throw what I can't. I also guess I'm "lucky" to not have worked on huge applications using untold amounts of 3rd party utilities and am usually in full control over the code base and therefore any exceptions that arise are of my own making (or at least happening in code I have full access to troubleshoot).Anonymous
March 05, 2010
@Anthony... Do you ever write code like: using (SomeType x = new SomeType) { } If so then you ARE using finaly blocks!!!! If not, the you are most likely; a) Allowing IDisposable objects to live longer than they shoulf b) Duplicating calls to Dispose in multple paths c) Using an over complicated means of dealing with IDisposable objectsAnonymous
March 05, 2010
d) Calling .Dispose() in the same block that instantiated the resource.Anonymous
March 05, 2010
If we're already grazing the java discussion, I'd be really interested why the C# team didn't impose similiar rules for exceptions like the java guys did. One of the few things I'm missing in C# is that you don't have to specify which Exceptions a method can throw and to guarantue that you handle it somewhere up the callstack. Well that doesn't solve any problems automatically (I've seen too much catch(Exception e) code to believe that), but I personally find that rather helpful. So is there any specific reason why the C# team didn't implement something similar but went for the supposedly easier way with only one exception group (compared to java with Error, Exception, RuntimeException)? Or was it just deemed not important enough for the amount of work involved? Just curious ;) Though other than that we still got the exactly same problem in every programming language and I think there can't be one solution for every case per definitionem. If you can't guarantee any invariants in your programming any longer and have no idea how messed up the situation is, in most cases trying to fail grazely will make things only worse. Though there are mission critical applications where just crashing also isn't a solution (medical apparatus, ..)Anonymous
March 05, 2010
The comment has been removedAnonymous
March 05, 2010
The comment has been removedAnonymous
March 05, 2010
The comment has been removedAnonymous
March 05, 2010
The comment has been removedAnonymous
March 05, 2010
Voo: There's a reason that Java is the only language with compile-time checked exceptions. To inflame the discussion a bit more and diverge even further off-topic, checked exceptions are like communism: in theory they work great (like in communes), but in practice they don't work because they don't scale because they fail to take human nature into account. People are lazy and will only do the minimum work needed unless they're truly committed to the cause. So when a leaf function is changed to throw a new exception that needs to be handled way up at the top of your callstack, only somebody who's really not lazy would bother to add it to the hundred methods in the middle. The other 99% of programmers would just swallow it or rethrow it as Exception, neither of which helps your cause. The best solution to this is probably to just document the exceptions that each method throws and have either the compiler or a static analysis tool warn you if your documentation doesn't match reality. That way as a programmer you can easily tell if you're going to get an exception you didn't expect, yet the language doesn't encourage bad behavior (at worst it encourages bad documentation).Anonymous
March 05, 2010
@David, I appreciate the feedback and was not aware that (a) using instituted a try/finally structure behind the scenes or that (b) it was best practice to use a using statement when using objects that implement IDisposable. Now thanks to you and our trusty friend MSDN I have that clarification. In my own defense, I've worked alone my entire (brief) .NET programming career so the only exposure I get to best practices are through outlets such as this blog. I will admit to being guilty of leaving cleanup to the garbage collector (but not always) in the event of the rare exception and I'll consider changing that in the future.Anonymous
March 05, 2010
I apologize if I am being dense, but what exactly is a "nested finally"? I assumed it meant something like try{ A(); try{ B(); } finally { } } finally{ } but after experimenting with similar code I am starting to believe I have assumed incorrectly. Most of my curiosity comes from trying to create a case where I could cause a finally block not to be run as was suggested (without unplugging or shooting the machine ;) ).Anonymous
March 05, 2010
The comment has been removedAnonymous
March 08, 2010
So what's the recommended practice? The gist I'm getting is that finally blocks are okay if the exception is Bad, but not Ugly. AppDomain.CurrentDomain.UnhandledException += new UnhandledExceptionEventHandler(CurrentDomain_UnhandledException); void CurrentDomain_UnhandledException(object sender, UnhandledExceptionEventArgs e) { Type exception = e.GetType(); if( exception == typeof( ???? ) || exception == typeof(???) ) { System.Environment.FailFast(); // ugly exceptions .. prevent finally blocks running. } } So what are all the Ugly exceptions we should think about?Anonymous
March 10, 2010
The comment has been removedAnonymous
March 10, 2010
The comment has been removedAnonymous
March 10, 2010
@Daniel, an excellent post. One "pattern" I have used where there is code I want/need to run even if "Ugly" is to register specific routines so they can be executed within the context of AppDomain.UnhandledException. This provides better control thant yes/no on ALL finally blocks. This approach is not without risk, and must be used sparingly, but for certain, well considered scenarios, it can be a life saver [especially when I am dealing with Industrial Automation...]Anonymous
March 10, 2010
Daniel, thanks for taking the time to answer so thoroughly. That helps me to realize what is going on much better than I did before. I never knew that the implications of catching an exception were so significant, but it is definitely something I'll bear in mind in the future.Anonymous
March 10, 2010
Is there a reason that BadException and UglyException aren't part of the Exception class hierarchy? If they were, it would be possible to catch(BadException e){ Log(); Rollback(); Fix(); etc... }. In this case, you could assume the exception's author and the thrower knew enough about the conditions which caused the exception to say, this is bad enough I don't know where to go, but I do know where I am and where I've been. On the other hand, the CLR, or possibly, the compiler, could say that catch(UglyException) will never run. No assumptions can be made about the state of the system and its best if we all just leave and leave now. For me, this pattern is easier to understand than having the CLR look for special exceptions that, from a type definition perspective, have no clearly defined attribute.Anonymous
March 10, 2010
The comment has been removedAnonymous
March 10, 2010
The comment has been removedAnonymous
March 12, 2010
OK, so now you're saying that all exceptions should be derived directly from Exception and all unhandled exceptions should immediately crash the app? That seems like a recipe for extremely fragile apps. Let's say that instead of throwing generic IOExceptions every time they came up with a new way to prevent you from opening a file (e.g. offline file unavailable, file involved in a transaction, broken symlink), they decided to throw new OfflineFileUnavailableException, TransactedFileException, or BrokenLinkExceptions. Instead of being able to catch IOException and tell the user "Sorry, the file isn't available in some way I can't explain", you would have to say "Sorry, but something unanticipated happened and now you lose all your unsaved data". Can you imagine writing a C program that looked at the error returned by every system call and crashed if it wasn't in a list of acceptable errors known at compile time? Raymond Chen would write a blog post about how your app required a different appcompat shim for every different version of your software!Anonymous
March 15, 2010
The comment has been removedAnonymous
March 15, 2010
The comment has been removedAnonymous
March 16, 2010
Daniel Earwicker: I think I fell asleep and woke up in some alternate universe where exceptions are part of a contract and programs need to be recompiled to run on new versions of the platform. This is still a .Net blog, isn't it?Anonymous
March 16, 2010
The comment has been removed