16

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?

Improve this question
6
  • 7
    Do you really trust your users enough to show them the complete contents of and exception message you weren't expecting? If there's nothing the user can do, tell them something went wrong and log the detail so it can be fixed. If there's something they can do, then tell them. Generally, outside of a test environment you don't want people knowing too much about what goes on under the hood of your application. Commented May 20, 2016 at 11:31
  • 2
    The line that says return Json in your try isn't valid C#. Commented May 20, 2016 at 11:38
  • @forsvarir maybe check if they have the silph scope to identify the error. Commented May 20, 2016 at 11:54
  • it generally looks like this? is this example code or the actual code that you are running? Commented May 20, 2016 at 12:18
  • Based on the specific question in the title, and the phrase "generally looks something like this", I take this to be a question about a programming practice rather than an open-ended critique of your function. Migrated from Code Review to Programmers. Commented May 20, 2016 at 18:05

5 Answers 5

Reset to default
12

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.

Improve this answer
2
  • 5
    re: security, anytime you dump exceptions to users you are creating a security risk as it will provide lots of information about what frameworks and libs you are using and often you can determine the versions too. It's a really great way to tell everyone what un-patched vulnerabilities you are subject to. Moreover, it gives a lot of information about what isn't being handled. Commented May 20, 2016 at 18:48
  • This answer is the one that shows the tension between the good UX of "don't show an error to a user that they don't understand" and the good design of "only catch the exceptions you can recover from" Commented May 21, 2016 at 10:54
3

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.

Improve this answer
2

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.

Improve this answer
4
  • StackOverflowException won't be caught anyway, will it? Commented May 20, 2016 at 20:10
  • If you catch Exception or blank? I don't think it'll get back to the try before crashing, no. That's kind of the point: there are always exceptions you cant catch, and exceptions that are a bad idea to try to catch. Not to mention, that even if you implement it that way, what happens to all your argument exceptions and null reference exceptions! Do you really want those in your logs when you're trying to test your code? Crashing fast is just plain safer. Commented May 20, 2016 at 21:12
  • 1
    Incidentally, that's why in Java you have Exception and Error as superclasses for exceptions. Error is for "serious problems that a reasonable application should not try to catch". Commented May 20, 2016 at 21:53
  • I think it should be perfectly possible to correctly handle StackOverflowException because it will be thrown when stack is too high. The exception will then unwind the stack and catching that exception when the stack is partially unwind is definitely possible. Commented Feb 18, 2021 at 10:45
1

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.

Improve this answer
1
  • Note that if the nested code hits some really rare edge case, catching all exceptions will make finding about it much harder. I think at minimum you should always log all exceptions that you catch with Pokemon exception handler. Commented Feb 18, 2021 at 10:43
1

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.

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.