Exception handling using an HttpModule

Question

We're reviewing one of the company's system's exception handling and found a couple of interesting things.

Most of the code blocks (if not all of them) are inside a try/catch block, and inside the catch block a new BaseApplicationException is being thrown - which seems to be coming from the Enterprise Libraries. I'm in a bit of a trouble here as I don't see the benefits off doing this. (throwing another exception anytime one occurs) One of the developers who's been using the system for a while said it's because that class's in charge of publishing the exception (sending emails and stuff like that) but he wasn't too sure about it. After spending some time going through the code I'm quite confident to say, that's all it does is collecting information about the environment and than publishing it.

My question is: - Is it reasonable to wrap all the code inside try { } catch { } blocks and than throw a new exception? And if it is, why? What's the benefit?

My personal opinion is that it would be much easier to use an HttpModule, sign up for the Error event of the Application event, and do what's necessary inside the module. If we'd go down this road, would we miss something? Any drawbacks?

Your opinion's much appreciated.

Answer 1

Never¹catch (Exception ex). Period². There is no way you can handle all the different kinds of errors that you may catch.

Never³ catch an Exception-derived type if you can't handle it or provide additional information (to be used by subsequent exception handlers). Displaying an error message is not the same as handling the error.

A couple of reasons for this, from the top of my head:

• Catching and rethrowing is expensive
• You'll end up losing the stack trace
• You'll have a low signal-to-noice ratio in your code

If you know how to handle a specific exception (and reset the application to pre-error state), catch it. (That's why it's called exception handling.)

To handle exceptions that are not caught, listen for the appropriate events. When doing WinForms, you'll need to listen for System.AppDomain.CurrentDomain.UnhandledException, and - if your doing Threading - System.Windows.Forms.Application.ThreadException. For web apps, there are similar mechanisms (System.Web.HttpApplication.Error).

As for wrapping framework exceptions in your application (non-)specific exceptions (i.e. throw new MyBaseException(ex);): Utterly pointless, and a bad smell.⁴

---

Edit

¹Never is a very harsh word, especially when it comes to engineering, as @Chris pointed out in the comments. I'll admit to being high on principles when I first wrote this answer.

^2,3 See ¹.

⁴ If you don't bring anything new to the table, I still stand by this. If you have caught Exception ex as part of a method that you know could fail in any number of ways, I believe that the current method should reflect that in it's signature. And as you know, exceptions is not part of the method signature.

Answer 2

If I am reading the question correctly, I would say that implementing a try / catch which intercept exceptions (you don't mention - is it catching all exceptions, or just a specific one?) and throws a different exception is generally a bad thing.

Disadvantages:

At the very least you will lose stack trace information - the stack you will see will only extend to the method in which the new exception is thrown - you potentially lose some good debug info here.

If you are catching Exception, you are running the risk of masking critical exceptions, like OutOfMemory or StackOverflow with a less critical exception, and thus leaving the process running, where perhaps it should have been torn down.

Possible Advantages:

In some very specific cases you could take an exception which doesn't have much debug value (like some exceptions coming back from a database) and wrap with an exception which adds more context, e.g id of the object you were dealing with.

However, in almost all cases this is a bad smell and should be used with caution.

Generally you should only catch an exception when there is something realistic that you can do in that location- ie recovering, rolling back, going to plan B etc. If there is nothing you can do about it, just allow it to pass up the chain. You should only catch and throw a new exception if there is specific and useful data available in that location which can augment the original exception and hence aid debugging.