What is the proper way to rethrow an exception in C#? [duplicate]

You should always use the following syntax to rethrow an exception. Else you'll stomp the stack trace:

throw;

If you print the trace resulting from throw ex, you'll see that it ends on that statement and not at the real source of the exception.

Basically, it should be deemed a criminal offense to use throw ex.

If there is a need to rethrow an exception that comes from somewhere else (AggregateException, TargetInvocationException) or perhaps another thread, you also shouldn't rethrow it directly. Rather there is the ExceptionDispatchInfo that preserves all the necessary information.

try
{
    methodInfo.Invoke(...);
}
catch (System.Reflection.TargetInvocationException e)
{
    System.Runtime.ExceptionServices.ExceptionDispatchInfo.Capture(e.InnerException).Throw();
    throw; // just to inform the compiler that the flow never leaves the block
}

My preferences is to use

try
{
}
catch (Exception ex)
{
     ...
     throw new Exception ("Add more context here", ex)
}

This preserves the original error, but it allows you to add more context, such as an object ID, a connection string, and stuff like that. Often my exception reporting tool will have five chained exceptions to report, each reporting more detail.

If you throw an exception without a variable (the second example) the stack trace will include the original method that threw the exception.

In the first example, the stack trace will be changed to reflect the current method.

Example:

static string ReadAFile(string fileName) {
    string result = string.Empty;
    try {
        result = File.ReadAllLines(fileName);
    } catch(Exception ex) {
        throw ex; // This will show ReadAFile in the stack trace
        throw;    // This will show ReadAllLines in the stack trace
    }

The first preserves the original stack trace of the exception, the second replaces it with the current location.

Therefore the first is by far the better.

I'll agree that most of the time you either want to do a plain throw, to preserve as much information as possible about what went wrong, or you want to throw a new exception that may contain that as an inner-exception, or not, depending on how likely you are to want to know about the inner events that caused it.

There is an exception though. There are several cases where a method will call into another method and a condition that causes an exception in the inner call should be considered the same exception on the outer call.

One example is a specialised collection implemented by using another collection. Let's say it's a DistinctList<T> that wraps a List<T>, but refuses duplicate items.

If someone called ICollection<T>.CopyTo on your collection class, it might just be a straight call to CopyTo on the inner collection (if say, all the custom logic only applied to adding to the collection, or setting it up). Now, the conditions in which that call would throw are exactly the same conditions in which your collection should throw to match the documentation of ICollection<T>.CopyTo.

Now, you could just not catch the exception at all, and let it pass through. Here though, the user gets an exception from List<T> when they were calling something on a DistinctList<T>. It is not the end of the world, but you may want to hide those implementation details.

Or you could do your own checking:

public CopyTo(T[] array, int arrayIndex)
{
  if(array == null)
    throw new ArgumentNullException("array");
  if(arrayIndex < 0)
    throw new ArgumentOutOfRangeException("arrayIndex", "Array Index must be zero or greater.");
  if(Count > array.Length + arrayIndex)
    throw new ArgumentException("Not enough room in array to copy elements starting at index given.");
  _innerList.CopyTo(array, arrayIndex);
}

That's not the worse code because it's boilerplate and we can probably just copy it from some other implementation of CopyTo where it wasn't a simple pass-through and we had to implement it ourselves. Still, it's needlessly repeating the exact same checks that are going to be done in _innerList.CopyTo(array, arrayIndex), so the only thing it has added to our code is 6 lines where there could be a bug.

We could check and wrap:

public CopyTo(T[] array, int arrayIndex)
{
  try
  {
    _innerList.CopyTo(array, arrayIndex);
  }
  catch(ArgumentNullException ane)
  {
    throw new ArgumentNullException("array", ane);
  }
  catch(ArgumentOutOfRangeException aore)
  {
    throw new ArgumentOutOfRangeException("Array Index must be zero or greater.", aore);
  }
  catch(ArgumentException ae)
  {
    throw new ArgumentException("Not enough room in array to copy elements starting at index given.", ae);
  }
}

In terms of new code added that could potentially be buggy, this is even worse. And we don't gain a thing from the inner exceptions. If we pass a null array to this method and receive an ArgumentNullException, we're not going to learn anything by examining the inner exception and learning that a call to _innerList.CopyTo was passed a null array and threw an ArgumentNullException.

Here, we can do everything we want with:

public CopyTo(T[] array, int arrayIndex)
{
  try
  {
    _innerList.CopyTo(array, arrayIndex);
  }
  catch(ArgumentException ae)
  {
    throw ae;
  }
}

Every one of the exceptions that we expect to have to throw if the user calls it with incorrect arguments, will correctly be thrown by that rethrow. If there's a bug in the logic used here, it's in one of two lines - either we were wrong in deciding this was a case where this approach works, or we were wrong in having ArgumentException as the exception type looked for. It's the only two bugs that the catch block can possibly have.

Now. I still agree that most of the time you either want a plain throw; or you want to construct your own exception to more directly match the problem from the perspective of the method in question. There are cases like the above where rethrowing like this makes more sense, and there are plenty of other cases. E.g., to take a very different example, if an ATOM file reader implemented with a FileStream and an XmlTextReader receives a file error or invalid XML, then it will perhaps want to throw exactly the same exception it received from those classes, but it should look to the caller that it is AtomFileReader that is throwing a FileNotFoundException or XmlException, so they might be candidates for similarly rethrowing.

We can also combine the two:

public CopyTo(T[] array, int arrayIndex)
{
  try
  {
    _innerList.CopyTo(array, arrayIndex);
  }
  catch(ArgumentException ae)
  {
    throw ae;
  }
  catch(Exception ex)
  {
    //we weren't expecting this, there must be a bug in our code that put
    //us into an invalid state, and subsequently let this exception happen.
    LogException(ex);
    throw;
  }
}