Is indiscriminately catching exceptions (Pokemon exception handling) ever acceptable? [duplicate]

Question

Normally, I don't anticipate exceptions and if I get them, its something that I can't fix in my code - bad user input or connectivity issues with the database.

But errors do occur so at the very least, I want to inform my user that something went wrong and log the error.

My exception handling generally looks something like this:

public JsonResult Create(UserCreateEditViewModel model)
{
    try
    {
        if (!ModelState.IsValid) return Json(new { IsOk = false, Message = "Some of your data was invalid, please try again." });

        var User = new User
        {
            // map UserCreateEditViewModel properties to User properties
        };

        OrtundEntities Db = new OrtundEntities();
        Db.Users.Add(User);
        Db.SaveChanges();

        return Json
    }
    catch(Exception ex)
    {
        // can't correct user data or repair connection
        // issues to the database so just report the error to the user
        return ReportError(ex, "Create User");
    }
}

public JsonResult ReportError(Exception ex, string action)
{
    return Json(new { IsOk = false, Title = action, Message = ex.Message });
}

I'm aware that in most cases, Pokemon exception handling is discouraged, but since there's nothing really I can do with an error, is Pokemon exception handling not okay to use simply to report the error?

Answer 1

Yes. Anytime exceptions could be exposed to end users or external systems, it might be a good idea to catch all possible exceptions, perform some generic handling (like logging) and emit a generic message. Both for usability reasons and for security reasons (an exception might contain sensitive information).

But often this top-level exception handler would not be implemented as an actual try..catch block, but would be handled by the application framework. In a typical web framework you wouldn't need to write try...catch around every request handler or page generator, but rather you would register the generic exception handling code in a single place. If you use ASP.NET MVC you can use OnException or similar and save a lot of boilerplate exception handling.

The important thing is that the application or request is still terminated. What is discouraged is catching exceptions indiscriminately and then continuing the execution as if nothing happened. Since an unexpected exception would mean the application state is compromised in some unknown way, continuing execution will just lead to further errors and data corruptions down the line, which can be incredibly hard to debug.

Another (less common) situation where it makes sense to catch everything: If you have a unreliable library which provides a non-essential service. Say you have an application with a plug-in architecture where third-parties can provide modules which will be loaded dynamically. It would make sense to catch all exceptions when calling into these plug-ins, since you dont want a faulty plug-in to take the whole application down.

Even internal modules could be treated as unreliable. For example, I have heard rumors that Microsoft treats subsystems in Office like this. If the grammar checker module throws an unexpected exception, it is better to show a dialog with "unexpected error in grammar checker" and continue without the check, rather than having the whole application terminate.

Note that this only works if the module can be clearly isolated from the main application, so you can be sure the error inside the module does not corrupt the state of the program at large. For example you would need to disable the plugin or module after the first unexpected exception, since it has a corrupted state. So this is only possible if the application has been explicitly designed to allow this level of isolation of the modules in question.

Answer 2

Pokemon exceptions ... Ever acceptable?

Never say (N)ever, but here goes. A concise conceptualization of exceptions shows why.

---

A try block tells me what situations I choose to not handle

Where to try, catch, and after-the-fact handle exceptions is a design issue.

Focused try blocks convey the program's concerns. Unreliable connections, undefined user data combinations, specific logic paths that are dead ends and so on. We all know that we're supposed to wrap uncontrollable resources like a DB or file system call but that must integrate with, be consistent with, the design.

Sure, we can't do much given a failed DB connection but catching and/or throwing and/or re-throwing an exception is still a choice not an absolute necessity. Further, there may be business domain circumstances where we do not want to "recover."

Be as informative as possible

It is sadly funny that I spend too much time debugging exceptions because I have no idea what state/data caused it. Just letting the application blow up is essentially the same thing.

If you know precisely where it was thrown, or know the exception's type then you can get relevant details to pass along. For example parameter values used in an SelectCommand if we're talking about an SqlException.

The exception's type itself can be an expression of the application's business domain. I might create one or more application-specific exceptions that codify, reinforce, express application-unique dead ends. These classes might have constructors ensuring we capture particular user data.

---

Pokemon is Pointless

Remove the try/catch from the above code and the behavior is exactly the same. I.E. trying everything is the same as trying nothing.

That a "Create User" message was added is superfluous because we know the origination point through the stack trace. And the message adds zero context for the specific circumstances that caused the exception.

Narrow try scope == Context

If I'm wrapping only the DB call then I know exactly what exception(s) to expect. And I know what relevant data to harvest to put in the Exception.Data property.

Narrow catch scope == Context

Bob's "Inverse Catch Clause Context rule" is an old maxim that I just made up. The more broadly scoped the try (for good reason we assume) then the more specific exception types we might want to catch.

Answer 3

No.

Others have noted that you may perhaps want to show users all exceptions in a nice dialog, or save them to a log. These are not terribly bad ideas.

But what happens when the exception is a StackOverflowException? An OutOfMemoryException? You may even get a ClrException!

In essence, you simply can't catch them all. You can try, and it may even provide some value, but it will be less than you expect: logging things well is likely to be enough even without the exceptions themselves.

Answer 4

Catching Exception is usually the wrong thing to do, however there are cases where it is acceptable and the right thing to do.

When writing web APIs, you might not want to reveal too much information about exceptions to your clients. You may also have designed into your API an error indicator so that clients can use this to decide what to do next. In this situation, catching all exceptions at the boundary of your API may be the right thing to do. Sure, you should be catching exceptions closer to where they are thrown, but sometimes third party libraries don't fully document/publish exceptions that they can throw so when you write an API you need to be careful. The boundary of an API can also be an excellent place to log the error because you can design your handling so that you have access to the request you are dealing with. Working from a failed request, combined with an exception log can be a very effective way of determining what caused a failure.

Now, looking at your particular case, as I said in my comment, returning the exception message to the client seems like the wrong thing to do. Most users aren't going to be able to interpret exception messages, at best they're going to be able to report what it says in an error report. Cut out the middle man and log the error instead. Return some kind of generic error to user instead.

Also, make sure whatever is calling your Create knows what to do when it gets IsOk==false because it's going to treat the call as a success rather than a failure unless you tell it otherwise.

Answer 5

Yes. At the very least, it's justifiable on the client side for applications that serve non-technical users, provided you're giving the user meaningful instructions.

For example, "There was an internal error. Please reload the page."

Exceptions should always and only be caught at the point that something meaningful can be done to correct the problem.