Is it ok to catch all exception types if you rethrow them wrapped another exception?

Question

I know you're not suppose to write code that caches all exception types like this.

try
{
  //code that can throw an exception
}
catch
{
   //what? I don't see no
}

Instead you're suppose to do something more like the code below allowing any other exception that you didn't expect to bubble up.

try
{
//code that can throw an exception
}
catch(TypeAException)
{
   //TypeA specific code
}

catch(TypeBException)
{
   //TypeB specific code
}

But is it ok to catch all exception types if you are wrapping them with another exception? Consider this Save() method below I am writing as part of a Catalog class. Is there anything wrong with me catching all exception types and returning a single custom CatalogIOException with the original exception as the inner exception?

Basically I don't want any calling code to have to know anything about all the specific exceptions that could be thrown inside of the Save() method. They only need to know if they tried to save a read only catalog (CatalogReadOnlyException), the catalog could not be serialized (CatalogSerializationException), or if there was some problem writing to the file (CatalogIOException).

Is this a good or bad way to handle exceptions?

/// <summary>
/// Saves the catalog
/// </summary>
/// <exception cref="CatalogReadOnlyException"></exception>
/// <exception cref="CatalogIOException"></exception>
/// <exception cref="CatalogSerializingExeption"></exception>
public void Save()
{
    if (!this.ReadOnly)
    {
        try
        {
            System.Xml.Serialization.XmlSerializer serializer = new XmlSerializer(typeof(Catalog));
            this._catfileStream.SetLength(0); //clears the file stream
            serializer.Serialize(this._catfileStream, this);
        }
        catch (InvalidOperationException exp)
        {
            throw new CatalogSerializationException("There was a problem serializing the catalog", exp);
        }
        catch (Exception exp)
        {
            throw new CatalogIOException("There was a problem accessing the catalog file", exp);
        }
    }
    else
    {
        throw new CatalogReadOnlyException();
    }
}

---

Update 1

Thanks for all the responses so far. It sounds like the consensus is I shouldn't be doing this, and I should only be catching exceptions if I actually have something to do with them. In the case of this Save() method there really isn't any exception that may be thrown that I want to handle in the Save() method itself. Mostly I just want to notify the user why they were not able to save.

I think my real problem is I'm using exceptions as a way to notify the user of problems, and I'm letting this inform how I am creating and handling exceptions a little too much. So instead should it sounds like it would be better to not catch any exceptions and let the UI layer figure out how to notify the user, and or crash. Is this correct? Consider the Save Menu event handler below.

    private void saveCatalogToolStripMenuItem_Click(object sender, EventArgs e)
    {
        //Check if the catalog is read only
        if (this.Catalog.ReadOnly)
        {
            MessageBox.Show("The currently opened catalog is readonly and can not be saved");
            return;
        }

        //attempts to save
        try
        {
            //Save method doesn't catch anything it can't deal with directly
            this.Catalog.Save(); 
        }
        catch (System.IO.FileNotFoundException)
        {
            MessageBox.Show("The catalog file could not be found");
        }
        catch (InvalidOperationException exp)
        {
            MessageBox.Show("There was a problem serializing the catalog for saving: " + exp.Message);
        }
        catch (System.IO.IOException exp)
        {
            MessageBox.Show("There was a problem accessing the catalog file: " + exp.Message);
        }
        catch (Exception exp)
        {
            MessageBox.Show("There was a problem saving the catalog:" + exp.Message);
        }
    }

Update 2

One more thing. Would the answer change at all if the Save() method was part of a public API vs internal code? For example if it was part of a public API then I'd have to figure out and document all the possible exceptions that Save() may throw. This would be a lot easier if knew that Save() could only possibly throw one of my three custom exceptions.

Also if Save() was part of a public API wouldn't security also be a concern? Maybe I would want to let the consumer of the API know that the save wasn't successful, but I don't want expose anything about how Save() works by letting them get at the exceptions that may have been generated.

Answer 1

This is a standard practice in .NET and how exceptions should be handled, especially for exceptions that are recoverable from.

Edit: I really have no idea why I'm being downvoted perhaps I read more into the authors intent than everyone else. But the way I read the code the fact he is wrapping those exceptions into his custom exception implies that the consumer of this method is equiped to handle those exceptions and that it is a responsibility of the consumer to deal with the error processing.

Not a single person that DV'd actually left any form of actual dispute. I stand by my answer that this is perfectly acceptable because the consumer should be aware of the potential exceptions this could throw and be equipped to handle them which is shown by the explicit wrapping of the exception. This also preserves the original exception so the stack trace is available and underlying exception accessible.

Answer 2

Doing a generic catch-all and rethrowing as a new type of exception does not really solve your problem and does not give you anything.

What you really need to do is to catch the exceptions that you can handle and then handle them (at the appropriate level - this is where rethrowing may be useful). All other exceptions either need to be logged so you can debug why they happened, or shouldn't be happening in the first place (for example - make sure you validate user input, etc.). If you catch all exceptions, you'll never really know why you're getting the exceptions you're getting and, thus, cannot fix them.

Updated Response

In response to the update of your question (particularly in how you want to handle the save case), my question to you would be - why are you using exceptions as a means of determine the path your program takes? For example, let's take the "FileNotFoundException." Obviously that can happen at times. But, instead of letting a problem happen and notifying the user about it, before saving (or doing whatever) with the file, why not check first that the file can be found. You still get the same effect, but you aren't using exceptions to control program flow.

I hope this all makes sense. Let me know if you have any additional questions.

Answer 3

When you re-throw with the original exception as inner exception, you lose the original stack trace, which is valuable debugging information.

I will sometimes do what you are suggesting, but I always log the original exception first to preserve the stack trace.

Answer 4

I don't think its a good idea.

You should only add you own type of exception, if you have anything to add.

And furthermore, you should only catch exceptions that you expect, and that you are able to handle - all other exceptions should be allowed to bubble up.

As a developer I must say, I get angry if you try to "hide" exceptions from me, by swallowing or wrapping them.

Answer 5

In this particular case exceptions should be rare enough that wrapping it shouldn't be a useful thing and will likely just get in the way of error handling down the line. There are plenty of examples within the .Net framework where a specific exception that I can handle is wrapped in a more general one and it makes it much more difficult (though not impossible) for me to handle the specific case.

Answer 6

For some more info on why catch(exception) is bad check out this article: http://blogs.msdn.com/clrteam/archive/2009/02/19/why-catch-exception-empty-catch-is-bad.aspx

Essentially catching 'Exception' is like saying 'if anything goes wrong I dont care carry on' and catching 'Exception' and wrapping it is like saying 'if anything goes wrong treat them as if they all went wrong for the exact same reason'.

This cannot be correct either you handle it because you semi-expected it or you totally don't think it should ever happen EVER (or didn't know it would). In this case you'd want some kind of app level logging to point you to an issue that you had never expected - not just a one size fits all solution.

Answer 7

I have written an article on this very topic before. In it I reiterate the importance of capturing as much data about the exception as is possible. Here's the URL to the article:

http://it.toolbox.com/blogs/paytonbyrd/improve-exception-handling-with-reflection-and-generics-8718

Answer 8

I don't see a problem with what you are doing. The reason for wrapping exceptions in a custom exception types is to create an abstraction between layers of code -- to translate lower-level errors into a higher-level context. Doing this relieves the calling code from having to know too much the implementation details of what Save does.

Your update #1 is an example of the calling code having to know way too much about the implementation details of Save(). In response to your second update, I agree 100%

PS
I'm not saying to do this in every scenario where you encounter exceptions. Only when the benefit outweighs the cost (usually at module boundaries).

Example scenarios for when this is especially useful: you are wrapping a 3rd party library, you don't yet know all the underlying exceptions that might be thrown, you don't have the source code or any documentation, and so on.

Also, he is wrapping the underlying exception and no information is lost. The exception can still be logged appropriately (though you'll need to recursion your way through the InnerExceptions).

Answer 9

What benefit does the user get from being told "There was a problem serializing the catalog"? I suppose your problem domain might be an extraordinary case, but every group of users I've ever programmed for would respond the same way when reading that message: "The program blew up. Something about the catalog."

I don't mean to be condescending towards my users; I'm not. It's just that generally speaking my users have better things to do with their attention than squander it constructing a fine-grained mental model of what's going on inside my software. There have been times when my users have had to build that kind of an understanding in order to use a program I've written, and I can tell you that the experience was not salutary for them or for me.

I think your time would be much better spent on figuring out how to reliably log exceptions and relevant application state in a form that you can access when your users tell you that something broke than in coming up with an elaborate structure for producing error messages that people are unlikely to understand.

Answer 10

My own rule of thumb is to catch and wrap Exception only if I have some useful context I can add, like the file name I was trying to access, or the connection string, etc. If an InvalidOperationException pops up in your UI with no other information attached, you're going to have a hell of a time tracking down the bug.

I catch specific exception types only if the context or message I want to add can be made more useful for that exception compared to what I would say for Exception generally.

Otherwise, I let the exception bubble up to another method that might have something useful to add. What I don't want to do is tie myself in knots trying to catch and declare and handle every possible exception type, especially since you never know when the runtime might throw a sneaky ThreadAbortException or OutOfMemoryException.

So, in your example, I would do something like this:

try
{
    System.Xml.Serialization.XmlSerializer serializer = 
        new XmlSerializer(typeof(Catalog));
    this._catfileStream.SetLength(0); //clears the file stream
    serializer.Serialize(this._catfileStream, this);
}
// catch (InvalidOperationException exp)
// Don't catch this because I have nothing specific to add that 
// I wouldn't also say for all exceptions.
catch (Exception exp)
{
    throw new CatalogIOException(
        string.Format("There was a problem accessing catalog file '{0}'. ({1})",
            _catfileStream.Name, exp.Message), exp);
}

Consider adding the inner exception's message to your wrapper exception so that if a user just sends you a screenshot of the error dialog, you at least have all the messages, not just the top one; and if you can, write the whole ex.ToString() to a log file somewhere.

Answer 11

To answer this question, you need to understand why catching System.Exception is a bad idea, and understand your own motivations for doing what your propose.

It's a bad idea because it makes the statement that anything that could have gone wrong is okay, and that the application is in a good state to keep running afterwards. That's a very bold statement to make.

So the question is: Is what you are proposing equivalent to catching System.Exception? Does it give the consumer of your API any more knowledge to make a better judgement? Does it simply encourage them to catch YourWrappedException, which is then the moral equivalent of catching System.Exception, except for not triggering FXCop warnings?

In most cases, if you know there's a rule against doing something, and want to know if something similar is "okay", you should start with understanding the rationale for the original rule.