What is the right way to pass on an exception? (C#) [duplicate]

This question already has answers here: Best practices for catching and re-throwing .NET exceptions (11 answers) Closed 9 years ago.

I'm wondering what the correct way is to pass on an exception from one method 开发者_Go百科to another.

I'm working on a project that is divided into Presentation (web), Business and Logic layers, and errors (e.g. SqlExceptions) need to be passed down the chain to notify the web layer when something goes wrong.

I've seen 3 basic approaches:

try  
{  
    //error code
} 
catch (Exception ex)
{
    throw ex;
}

(simply rethrow)

try  
{  
    //error code
} 
catch (Exception ex)
{
    throw new MyCustomException();
}

(throw a custom exception, so that a dependency on the data provider is not passed on)

and then simply

//error code

(not doing anything at all, letting the error bubble up by itself)

Naturally there's some logging happening in the catch block too.

I prefer number 3, while my colleague uses method 1, but neither of us can really motivate why.

What are the advantages/disadvantages of using each method? Is there a better method I don't know of? Is there an accepted Best Way?

---

If you do nothing you should simply let it go upper where some one will handle it.

You can always handle a part of it (like logging) and re-throw it. You can re-throw by simply sending throw; without having to explicit the ex name.

try
{

}
catch (Exception e)
{
    throw;
}

The advantage to handle it is that you can ensure that some mechanism is there to notify you that you have an error where you do not suspect to have one.

But, in some case, let say a Third Party, you want to let the user handle it and when it's that case you should let it continue to bubble up.

---

I think you should start with a slightly different question

How do I expect other components to interact with exceptions thrown from my module?

If the consumers are quite capable of handling the exceptions thrown by the lower / data layers then quite simply do nothing. The upper layer is capable of handling the exceptions and you should only do the minimum amount necessary to maintain your state and then rethrow.

If the consumers cannot handle low level exceptions but instead need a bit higher level exceptions, then create a new exception class which they can handle. But make sure to pass on the original exception a the inner exception.

throw new MyCustomException(msg, ex);

---

The correct way to re-throw an exception in C# is like below:

try
{
    ....
}
catch (Exception e)
{
    throw;
}

See this thread for specifics.

---

Only use try/catch blocks around exceptions you expect and can handle. If you catch something you cannot handle, it defeats the purpose of try/catch which is to handle expected errors.

Catching big exception is rarely a good idea. The first time you catch an OutOfMemoryException are you really going to be able to gracefully handle it? Most API's document the exceptions each method can throw, and those should be the only ones handled and only if you can gracefully handle it.

If you want to handle the error further up the chain, let it bubble up on its own without catching and rethrowing it. The only exception to this would be for logging purposes, but logging at every step is going to do an excessive amount of work. It is better to just document where exceptions your public methods can be expected to allow to bubble up and let the consumer of your API make the decision on how to handle it.

---

No one has yet pointed out the very first thing you should be thinking of: what are the threats?

When a back-end tier throws an exception, something terrible and unexpected has happened. The unexpected terrible situation might have happened because that tier is under attack by a hostile user. The last thing you want to do in that case is serve up to the attacker a detailed list of everything that went wrong and why. When something goes wrong on the business logic tier the right thing to do is to carefully log all information about the exception and replace the exception with a generic "We're sorry, something went wrong, administration has been alerted, please try again" page.

One of the things to keep track of is all the information you have about the user and what they were doing when the exception happened. That way if you detect that the same user always seems to be causing problems, you can evaluate whether they are likely to be probing you for weaknesses, or simply using an unusual corner of the application that wasn't sufficiently tested.

Get the security design right first, and only then worry about diagnosis and debugging.

---

I've seen (and held) various strong opinions about this. The answer is that I don't think there currently is an ideal approach in C#.

At one point I felt that (in a Java-minded way) the exception is part of the binary interface of a method, as much as the return type and parameter types. But in C#, it simply isn't. This is clear from the fact that there is no throws specification system.

In other words, you can if you wish take the attitude that only your exception types should fly out of your library's methods, so your clients don't depend on your library's internal details. But few libraries bother to do this.

The official C# team advice is to catch each specific type that might be thrown by a method, if you think you can handle them. Don't catch anything you can't really handle. This implies no encapsulation of internal exceptions at library boundaries.

But in turn, that means that you need perfect documentation of what might be thrown by a given method. Modern applications rely on mounds of third party libraries, rapidly evolving. It makes a mockery of having a static typing system if they are all trying to catch specific exception types that might not be correct in future combinations of library versions, with no compile-time checking.

So people do this:

try
{
}
catch (Exception x) 
{
    // log the message, the stack trace, whatever
}

The problem is that this catches all exception types, including those that fundamentally indicate a severe problem, such as a null reference exception. This means the program is in an unknown state. The moment that is detected, it ought to shut down before it does some damage to the user's persistent data (starts trashing files, database records, etc).

The hidden problem here is try/finally. It's a great language feature - indeed it's essential - but if a serious enough exception is flying up the stack, should it really be causing finally blocks to run? Do you really want the evidence to be destroyed when there's a bug in progress? And if the program is in an unknown state, anything important could be destroyed by those finally blocks.

So what you really want is (updated for C# 6!):

try
{
    // attempt some operation
}
catch (Exception x) when (x.IsTolerable())
{
    // log and skip this operation, keep running
}

In this example, you would write IsTolerable as an extension method on Exception which returns false if the innermost exception is NullReferenceException, IndexOutOfRangeException, InvalidCastException or any other exception types that you have decided must indicate a low-level error that must halt execution and require investigation. These are "intolerable" situations.

This might be termed "optimistic" exception handling: assuming that all exceptions are tolerable except for a set of known blacklisted types. The alternative (supported by C# 5 and earlier) is the "pessimistic" approach, where only known whitelisted exceptions are considered tolerable and anything else is unhandled.

Years ago the pessimistic approach was the official recommended stance. But these days the CLR itself catches all exceptions in Task.Run so it can move errors between threads. This causes finally blocks to execute. So the CRL is very optimistic by default.

You can also enlist with the AppDomain.UnhandledException event, save as much information as you can for support purposes (at least the stack trace) and then call Environment.FailFast to shut down your process before any finally blocks can execute (which might destroy valuable information needed to investigate the bug, or throw other exceptions that hide the original one).

---

I'm not sure that there really is an accepted best practices, but IMO

try  // form 1: useful only for the logging, and only in debug builds.
{  
    //error code
} 
catch (Exception ex)
{
    throw;// ex;
}

Makes no real sense except for the logging aspect, so I would do this only in a debug build. Catching an rethrowing is costly, so you should have a reason for paying that cost other than just that you like looking at the code.

try  // form 2: not useful at all
{  
    //error code
} 
catch (Exception ex)
{
    throw new MyCustomException();
}

This one makes no sense at all. It's discarding the real exception and replacing it with one that contains less information about the actual real problem. I can see possibly doing this if I want to augment the exception with some information about what was happening.

try  // form 3: about as useful as form 1
{  
    //error code
} 
catch (Exception ex)
{
    throw new MyCustomException(ex, MyContextInformation);
}

But I would say in nearly all cases where you are not handling the exception the best form is to simply let a higher level handler deal with it.

// form 4: the best form unless you need to log the exceptions.
// error code. no try - let it percolate up to a handler that does something productive.

---

Normally you'd catch only exceptions which you expect, can handle and let application work in a normal way further. In case you'd like to have some additional error logging you'll catch an exception, do logging and rethrow it using "throw;" so the stack trace is not modified. Custom exceptions are usually created for the purpose of reporting your application specific errors.