A nasty problem I've been tangling with for a while now is that C# likes to eat exceptions. If one is already in an exception context and another exception gets thrown then the first exception, by default, is just lost. I explore below some ways to deal with this and honestly they all suck. Does anyone have a better idea?
So it all started with Using. I often am in a situation where I create stateful objects, do something with them and then need to make sure they get cleaned up so I don't run out of resources (threads, connections, memory, etc.)
Typically the way this is handled is via Using. Just wrap the object in Using and one has a (sorta) guarantee that things will be cleaned up. But Using is generally not considered a good idea because it can hide exceptions. See here for a description with examples of the two flavors of problems with Using.
The short description of the problem is that Using appears to essentially be some syntactic sugar on top of Finally. To see the issue it's useful to understand that:
using (SqlConnection sqlConnection = new SqlConnection(connectionString))
{
doStuff(sqlConnection);
}
Is, near as I can tell anyway, functionally identical to:
SqlConnection sqlConnection = null;
try
{
sqlConnection = new SqlConnection(connectionString);
doStuff(sqlConnection);
}
finally
{
if (sqlConnection != null)
{
sqlConnection.Dispose();
}
}
Now imagine that doStuff() throws an exception. Before the exception is caught the finally clause will be called. Now imagine that sqlConnection.Dispose() throws an exception. What will happen is that the sqlConnection.Dispose() exception will mask the doStuff() exception. In the general case this is usually bad. In many cases the thing that will cause sqlConnection.Dispose() to fail will probably be explained by something bad that doStuff() did. So by losing the doStuff() exception we lose the reason why things went wrong in the first place.
As far as I can tell these are the options for how to deal with this situation:
Lose one of the exceptions I can set things up so that one of the two exceptions (either doStuff's or Dispose's) is simply thrown away. The easy one to get rid of is doStuff since I get that for 'free' via finally. Getting rid of Dispose's is also pretty easy, just wrap the Dispose call in a try/catch with an empty catch. It isn't pretty but it would work. This approach sucks if both exceptions have important system data and picking which one to get rid of is kind of random. After all doStuff's exception might have caused the Dispose exception, or it might not, I won't know if I get rid of doStuff's exception.
Make one of the exceptions an inner exception In essence I would have to create a new exception that tries to replicate one of the exceptions and puts the other exception as an inner exception. But creating a new exception gets rid of the old stack trace which is losing important information and damned if I can tell which exception should be outer and inner. In reality we are really just abusing inner exception. The point of inner exception is that it somehow caused the outer exception. But in this case the two exceptions might be unrelated so using the inner/outer isn't great.
Log one of the exceptions Another option, which is really just slightly better than losing one of the exceptions, is log something about the exception that we choose to lose. If one of the exceptions is a 'show stopper' exception (e.g. the program is going to exit) then I only need to know about the other exception for investigative purposes. So I can just dump the hidden exception's data into the log and let the 'killer' exception go up the stack.
Use aggregate exceptions This is a new feature introduced by .NET 4.0 that allows one to throw exceptions that consist of collections of exceptions. But unless the code was written from the ground up to deal with aggregate exceptions this pattern is crazy intrusive. Imagine we have handlers for both doStuff and Dispose. If they aren't expecting an aggregate exception and pattern matching properly then the aggregate exception containing both doStuff and Dispose's exceptions inside of it will just blow right past those handlers.
To be clear, all these options suck. Losing an exception offends me so I just can't bring myself to do that. The inner exception trick is just wrong. It's the kind of semantics that will drive other programmers crazy. “I see this inner exception but I swear it couldn't have caused the outer exception, WTF?!?!??” Aggregates seem really wacky to me unless code was written from the ground up to deal with them and even then they are really painful. Here is an example of an aggregate exception just to drive home the point:
SqlConnection sqlConnection = new SqlConnection(connectionString);
try
{
doStuff(sqlConnection);
}
catch (Exception e)
{
try
{
connection.Close();
}
catch (Exception closeE)
{
throw new AggregateException(new Exception[] { e, closeE });
}
throw;
}
connection.Close();
Think about that code for a second. If doStuff screws up and Close doesn't have a problem then a doStuff exception is thrown. If doStuff doesn't throw an exception but the Close() outside the catch block throws an exception then a Close() exception is thrown. But if both doStuff and Close() throw then an aggregate exception gets thrown. Does anyone want to write the catch clauses for this stuff?!?! The only sane solution is to always throw Aggregate exceptions and then write handlers that pick through them to see what's there. ICK. It means having to wrap all calls in all cases with a try/catch just to translate their non-aggregate exceptions into aggregate exceptions. No thanks, that's just nuts.
So I'm left with logging. But remember that isn't free either. For example:
SqlConnection sqlConnection = new SqlConnection(connectionString);
try
{
doStuff(sqlConnection);
}
catch (Exception)
{
try
{
sqlConnection.Dispose();
}
catch(Exception e)
{
log(e);
}
throw;
}
sqlConnection.Dispose();
In most cases if I have to pick between doStuff's exception and the Dispose exception I want doStuff's. So this means I can't use “using” (which forces me to hide doStuff's exception if Dispose throws) which bloats my code. And I have to remember to repeat Dispose twice (once inside of the catch and once outside how's that for begging for bugs?). And I have to remember to wrap the Dispose inside the catch in its own try/catch and log. This sucks.
So all the choices seem to suck. I think the least sucky is logging but it's still pretty high on the sucky scale.
Anyone have any better ideas?
Dispose isn’t supposed to ever throw an exception. And though some classes are buggy, I have never seen SqlConnection screw up here. Are you sure you aren’t chasing shadows?
I was actually just using SqlConnection as an example because I happened to be writing code using it recently. I probably should have just used something more generic.
In general it is bad behavior for dispose to throw but alas bad behavior (as indicated in the article I link to) does happen and when it does happen that is typically exactly the kind of exception one needs to catch and investigate because it means something ‘doubleplusungood’ has happened.
Task.Dispose() throws exceptions so I would say this post isn’t chasing shadows.
I don’t think you lose the stack trace when you create a new Exception with an inner Exception.
But yes, exceptions in Dispose methods are a problem.