69

I typically agree with most code analysis warnings, and I try to adhere to them. However, I'm having a harder time with this one:

CA1031: Do not catch general exception types

I understand the rationale for this rule. But, in practice, if I want to take the same action regardless of the exception thrown, why would I handle each one specifically? Furthermore, if I handle specific exceptions, what if the code I'm calling changes to throw a new exception in the future? Now I have to change my code to handle that new exception. Whereas if I simply caught Exception my code doesn't have to change.

For example, if Foo calls Bar, and Foo needs to stop processing regardless of the type of exception thrown by Bar, is there any advantage in being specific about the type of exception I'm catching?

Maybe a better example:

public void Foo()
{
    // Some logic here.
    LogUtility.Log("some message");
}

public static void Log()
{
    try
    {
        // Actual logging here.
    }
    catch (Exception ex)
    {
        // Eat it. Logging failures shouldn't stop us from processing.
    }
}

If you don't catch a general exception here, then you have to catch every type of exception possible. Patrick has a good point that OutOfMemoryException shouldn't be dealt with this way. So what if I want to ignore every exception but OutOfMemoryException?

Thomas Owens
  • 79,623
  • 18
  • 192
  • 283
Bob Horn
  • 2,327
  • 3
  • 20
  • 26
  • 14
    What about `OutOfMemoryException`? Same handling code as everything else? – Patrick Sep 09 '12 at 15:23
  • @Patrick Good point. I added a new example in my question to cover your question. – Bob Horn Sep 09 '12 at 16:23
  • 1
    Catching general exceptions is about as bad as making general statements about what you everyone should do. There is generally no one-size-fits-all approach to everything. –  Sep 13 '12 at 17:40
  • 5
    @Patrick that would be `OutOfMemoryError`, which is separate from the `Exception` inheritance tree for that very reason – Darkhogg Jan 17 '15 at 17:02
  • What about keyboard exceptions, like Ctrl-Alt-Del? – Mawg says reinstate Monica Jan 28 '15 at 16:04
  • Handling a logging failure is one of the only situation I can imagine where it would make sense to catch and ignore all exceptions thrown. So yeah, the rule has some exceptions. – JacquesB Aug 26 '15 at 15:38
  • @Patrick you take into account in every code path there could be OutOfMemory exception? That must be very exhausting code to read – Dirk Boer Nov 20 '22 at 13:48

12 Answers12

40

These rules are generally a good idea and thus should be followed.

But remember these are generic rules. They don't cover all situations. They cover the most common situations. If you have a specific situation and you can make the argument that your technique is better (and you should be able to write a comment in the code to articulate your argument for doing so) then do so (and then get it peer reviewed).

On the counter side of the argument.

I don't see your example above as a good situation for doing so. If the logging system is failing (presumably logging some other exception) then I probably do not want the application to continue. Exit and print the exception to the output so the user can see what happened.

Martin York
  • 11,150
  • 2
  • 42
  • 70
  • 2
    Concur. At the very *least*, now that regular logging is offline make some kind of provision that'll give *some* kind of debugging output, to STDERR if nothing else. – Shadur Sep 10 '12 at 04:20
  • 11
    I upvoted both of you, but I think that you are thinking like developers, not users. A user generally does not care whether logging is happening, so long as the application is useable. – Mawg says reinstate Monica Dec 10 '14 at 09:19
  • 1
    Interesting, as log4net explicitly doesn't want to stop the app just because logging fails. And i think it depends on what you're logging; security audits, sure, kill the process. But debug logs? Not that important. – Andy Aug 26 '15 at 18:02
  • @Mawg in this instance I would say the consumer of the logs may be a developer but they are in the role of a maintenence user. Failure to log errors can obscure the source and nature of other errors. I think the mantra of don't handle errors you don't understand still applies. – Sprague Aug 05 '16 at 13:54
  • 3
    Why don't you understand them? And should you not strive to do so? – Mawg says reinstate Monica Aug 06 '16 at 08:51
34

Yes, catching general exceptions is a bad thing. An exception usually means that the program cannot do what you asked it to do.

There are a few types of exceptions that you could handle:

  • Fatal exceptions: out of memory, stack overflow, etc. Some supernatural force just messed up your universe and the process is already dying. You cannot make the situation better so just give up
  • Exception thrown because bugs in the code that you are using: Don't try to handle them but rather fix the source of the problem. Don't use exceptions for flow control
  • Exceptional situations: Only handle exception in these cases. Here we can include: network cable unplugged, internet connection stopped working, missing permissions, etc.

Oh, and as a general rule: if you don't know what to do with an exception if you catch it, it is better to just fail fast (pass the exception to caller and let it handle it)

Victor Hurdugaci
  • 3,243
  • 18
  • 20
  • 1
    What about the specific case in the question? If there is a bug in the logging code, it's probably okay to ignore the exception, because the code that actually matters shouldn't be affected by that. – svick Sep 09 '12 at 17:52
  • @svick that highly depends on how critical the logging is. Suppose yo are trying to track down another bug, and you need a full log to reconstruct the situation, you definitely do not want to ignore logger exceptions – stijn Sep 09 '12 at 19:07
  • 4
    Why not fix the bug in the logging code instead? – Victor Hurdugaci Sep 09 '12 at 21:30
  • 4
    Victor, it's probably not a bug. If the disk the log lives on runs out of space, is that a bug in the logging code? – Carson63000 Sep 10 '12 at 00:05
  • 4
    @Carson63000 - well, yes. The logs aren't being written, therefore: bug. Implement fixed size rotating logs. – detly Sep 10 '12 at 00:36
  • 6
    This is the best answer imo, but it is missing one crucial aspect: **security**. There are some exceptions that occur as a result of the bad guys trying to f-up your program. If an exception gets thrown that you are unable to handle, then you cannot necessarily trust that code anymore. Nobody likes to hear that they wrote the code that let the bad guys in. – riwalk Sep 10 '12 at 02:04
  • You could throw away cached data in case of an OutOfMemoryException, then try again. That might make the situation better. Or, for a desktop application, you might save as much of the user's data as possible to a temporary file and try to restore it on restart. – nikie Sep 10 '12 at 17:52
  • 2
    @detly: The disk can still run out of space, even if you implement fixed size rotating logs. Is it still a bug in the logging code? – Jay Sullivan Aug 15 '13 at 17:50
  • @notfed - Maybe, maybe not. The bug is: if something goes wrong, you should find out about it from the logs. If your logs have no errors in them, but your app is behaving incorrectly, yes that is a bug in the overall behaviour of your app. In the context of the original question then, yes, you should stop processing. Stop your application and sort out your environment until it's operable again. – detly Aug 15 '13 at 23:59
  • @VictorHurdugaci: Stack overflow exception is a special case, since .net does not allow you to catch it. Even a general exception handler will not catch it. – JacquesB Aug 27 '15 at 08:32
  • This answer is not good. It says that yes general exceptions are bad, but does not say why. Without backing evidence, this is merely an opinion. – Kenneth K. Dec 22 '15 at 21:09
  • 1
    Wait, you say "exceptions you *could* handle", then the first item you list is "you cannot make the situation better so just give up." This is not handling the exception. – ErikE Jan 12 '16 at 19:02
  • Isn't exception (as well as the `finally` clause) just another kind of flow control? Sometimes it's just plainly intuitive to use exceptions for flow control, instead of `goto`s or `break`s etc. – iBug Dec 04 '18 at 15:38
17

The topmost outer loop should have one of these to print all it can, and then die a horrific, violent and NOISY death (as this shouldn't happen and someone needs to hear).

Otherwise you should generally be very careful as you most likely have not anticipated everything that could happen at this location, and hence will most likely not treat it correctly. Be as specific as possible so you only catch those you know will happen, and let those not seen before bubble up to the above mentioned noisy death.

  • 2
    I agree with this in theory. But what about my logging example above? What if you simple want to ignore logging exceptions and continue? Should you need to catch every exception that could be thrown and handle each of those while letting the more serious exception bubble-up? – Bob Horn Sep 09 '12 at 16:45
  • 6
    @BobHorn: Two ways of looking at that. Yes, you can say that logging isn't particularly important and if it fails then it shouldn't bring your app to a halt. And that's a fair enough point. But, what if your application fails to log for five months and you had no idea? I've seen this happen. And then we needed the logs. Disaster. I would suggest doing everything you can think of to stop logging failing is better than ignoring it when it does. (But you're not going to get much of a consensus on that.) – pdr Sep 09 '12 at 17:12
  • 1
    @pdr In my logging example, I would typically send an email alert. Or, we have systems in place to monitor logs, and if a log isn't being actively populated, our support team would get an alert. So we'd become aware of the logging issue in other ways. – Bob Horn Sep 09 '12 at 17:23
  • @BobHorn your logging example is rather contrived - most logging frameworks I know of, go to great length to _not_ throw any exceptions _and_ would not be invoked like that (as it would make any "log statement happened at line X in file Y" useless) –  Sep 09 '12 at 17:44
  • @pdr I've raised the question at the logback list (Java) on how to make the logging framework halt the application if logging configuration fails, simply because it is SO important we have the logs, and any silent failure is unacceptable. A good solution is still pending. –  Sep 09 '12 at 17:46
  • any reason you'd use a top level catch Exception rather than an UnhandledExceptionEventHandler? – jk. Sep 10 '12 at 07:25
  • @jk. I come from Java. It doesn't have that. –  Sep 10 '12 at 07:35
  • @ThorbjørnRavnAndersen and what about the [UncaughtExceptionHandler](http://docs.oracle.com/javase/6/docs/api/java/lang/Thread.UncaughtExceptionHandler.html) – ratchet freak Sep 10 '12 at 09:39
  • @ratchetfreak well, in that case it didn't have that. We've only recently gone to 1.5. I still like to have the top loop explicitly testing, instead of relying on a handler. –  Sep 10 '12 at 09:40
12

It's not that it's bad, it's just that specific catches are better. When you're specific, it means that you actually understand, more concretely, what your application is doing, and have more control over it. In general, if you come upon a situation where you just catch an Exception, log it and continue, there's probably some bad things that are going on anyway. If you're specifically catching the exceptions that you know a code block or method can throw, then there's a higher likelihood you can actually recover instead of just logging and hoping for the best.

Ryan Hayes
  • 20,139
  • 4
  • 68
  • 116
7

The two possibilities are not mutually exclusive.

In an ideal situation, you would catch all possible types of exception your method could generate, handle them on a per-exception basis, and in the end add a general catch clause to catch any future or unknown exceptions. This way you get the best of both worlds.

try
{
    this.Foo();
}
catch (BarException ex)
{
    Console.WriteLine("Foo has barred!");
}
catch (BazException ex)
{
    Console.WriteLine("Foo has bazzed!");
}
catch (Exception ex)
{
    Console.WriteLine("Unknown exception on Foo!");
    throw;
}

Keep in mind that in order to catch the more specific exceptions, you must put them first.

Edit: For reasons stated in the comments, added a rethrow in the last catch.

Rotem
  • 1,946
  • 16
  • 16
  • 2
    Wait. Why would you ever want to log an unknown exception to console and carry on? If it's an exception that you didn't even know could be thrown by the code, you're probably talking about a system failure. Carrying on in this scenario is probably a really bad idea. – pdr Sep 09 '12 at 15:43
  • 1
    @pdr Surely you realize it's a contrived example. The point was to demostrate catching of both specific and general exceptions, not how to handle them. – Rotem Sep 09 '12 at 15:48
  • 1
    @Rotem I think you have a good answer here. But pdr has a point. The code, as is, makes it look like catching and continuing is fine. Some beginner, seeing this example, might think that's a good approach. – Bob Horn Sep 09 '12 at 16:14
  • Contrived or not, it makes your answer wrong. As soon as you start catching exceptions like Out Of Memory and Disk Full and just swallowing them, you're proving why catching general exceptions is in fact bad. – pdr Sep 09 '12 at 16:16
  • 1
    If you catch a general exception only to log it, then you should rethrow it after logging. – Victor Hurdugaci Sep 09 '12 at 16:20
  • @all You're totally right, I see now all my answer does is prove the opposite point. Edited. – Rotem Sep 09 '12 at 16:21
  • It might be more helpful with examples like "OutOfMemory" or "NullReference" or what not. But this here is the right answer. The code analysis rule *title* is poorly worded. But the best-practice is to try to handle the kind of exceptions you will get, with a certain understanding of what they would be, assuming you wrote the code or have access to documentation, and *then* have a general exception catch. A real-world example would be in the olden days when performing a server-side Response.Redirect() in an ASP.NET page would result in ThreadAbortExceptions, which were quite unnecessary. – lorddev Sep 14 '12 at 06:36
6

I have been pondering the same recently, and my tentative conclusion is that the mere question arises because the .NET Exception hierarchy is severely messed up.

Take, for example, the lowly ArgumentNullException which might be a reasonable candidate for an exception you don't want to catch, because it tends to indicate a bug in the code rather than a legitimate runtime error. Ah yes. So does NullReferenceException, except that NullReferenceException does derive from SystemException directly, so there's no bucket you can place all "logical errors" into to catch (or not catch).

Then there is the IMNSHO major botch of having SEHException derive (via ExternalException) SystemException and thus make it a "normal" SystemException When you get an SEHException, you want to write a dump and terminate as fast as you can -- and starting with .NET 4 at least some SEH exceptions are considered Corrupted State Exceptions that will not be caught. A good thing, and a fact that makes the CA1031 rule even more useless, because now your lazy catch (Exception) will not catch these worst types anyway.

Then, it seems that the other Framework stuff rather inconsistently derives either Exception directly or via SystemException, making any attempt at grouping catch-clauses by something like severity moot.

There's a piece by Mr. Lippert of C# fame, called Vexing Exception, where he lays out some useful categorization of exceptions: You could argue you only want to catch "exogenous" ones, except ... C# the language and the design of the .NET framework exceptions make it impossible to "catch only exogenous ones" in any succinct manner. (and, e.g, an OutOfMemoryException may very well be a totally normal recoverable error for an API that has to allocate buffers that are somehow large)

Bottom line for me is, that the way C# catch blocks work and the way the Framework exception hierarchy is designed, rule CA1031 is beyond totally useless. It pretends to help with the root issue of "do not swallow exceptions" but swallowing exceptions has zero to do with what you catch, but with what you then do:

There are at least 4 ways to legitimately handle a caught Exception, and CA1031 only seems to superficially handle one of them (namely the re-throw case).

As a side note, there's a C# 6 feature called Exception Filters that will make CA1031 slightly more valid again since then you will be able to properly, properly, properly filter the exceptions you want to catch, and there's less reason to write an unfiltered catch (Exception).

Glorfindel
  • 3,137
  • 6
  • 25
  • 33
Martin Ba
  • 7,578
  • 7
  • 34
  • 56
  • 1
    A major problem with `OutOfMemoryException` is that there's no nice way code can be sure that it "only" indicates the failure of a particular allocation one was prepared to have fail. It's possible that some other more serious exception was thrown, and an `OutOfMemoryException` occurred during stack unwinding from that other exception. Java may have been late to the party with its "try-with-resources", but it handles exceptions during stack unwinding a bit better than .NET. – supercat Apr 21 '15 at 18:05
  • 1
    @supercat - true enough, but that's pretty much true of any other general System exception, when stuff goes bad in any finally block. – Martin Ba Apr 21 '15 at 18:13
  • 1
    That's true, but one can (and generally should) guard `finally` blocks against exceptions that one can realistically expect to occur in such a way that the original and new exceptions both get logged. Unfortunately, doing that will often require memory allocation, which could cause failures of its own. – supercat Apr 21 '15 at 18:33
4

Pokemon exception handling (gotta catch em all!) is certainly not always bad. When your exposing a method to a client, especially an end user its often better to catch anything and everything rather than have your application crash and burn.

Generally though they should be avoided where you can. Unless you can take specific action based on the type of the exception its better not to handle it and allow the exception to bubble up rather then swallow the exception or handle it incorrectly.

Have a look at this SO answer for more reading.

Community
  • 1
Tom Squires
  • 17,695
  • 11
  • 67
  • 88
  • 1
    This situation (when a developer must be a Pokemon Trainer) appears when you are making an entire software yourself: you made the layers, database connections and user's GUI. Your app must recover from situations like connectivity lost, user´s folder deleted, data corrupted, etc. End Users don't like apps that crash and burn. They like apps that can work on a burning exploding crashing computer! – Broken_Window Mar 03 '14 at 16:00
  • I hadn't thought of this until now, but in Pokemon, it's generally assumed that by 'catch them all' you want exactly one of each, and have to handle catching it specifically, which is the opposite of the common use of this phrase among programmers. – Magus Apr 03 '14 at 19:45
  • 2
    @Magus: With a method like `LoadDocument()`, it's essentially impossible to identify all the things that might go wrong, but 99% of exceptions that could be thrown will simply mean "It was not possible to interpret the contents of a file with the given name as a document; deal with it." If someone tries to open something that isn't a valid document file, that shouldn't crash the application and kill any other open documents. Pokemon error handling in such cases is ugly, but I don't know any good alternatives. – supercat Jul 09 '14 at 20:53
  • @supercat: I was just making a linguistic point. However, I don't think invalid file contents sound like something that should throw more than one kind of exception. – Magus Jul 09 '14 at 22:56
  • 1
    @Magus: It can throw all sorts of exceptions--that's the problem. It's often very difficult to anticipate all the kinds of exceptions that might get thrown as a consequence of invalid data in a file. If one doesn't use PokeMon handling, one risks having an application die because e.g. the file one was loading contained an exceptionally-long string of digits in a place which required a decimal-formatted 32-bit integer, and a numerical overflow occurred in the parsing logic. – supercat Jul 09 '14 at 23:02
  • In addition to @supercat 's points, .NET for example throws several exceptions that essentially mean "Your path is invalid" but there's no way to check if a path *is* valid other than just trying to open / save a file. Also any file reading / writing can fail at any time due to things happening in the background like ejecting flash drives, loss of permissions, a disk suddenly becoming full, etc. – jrh Jun 29 '18 at 19:16
  • ...for a method like `LoadDocument` I'd personally (carefully) catch and rethrow `IOException` at different stages with a more descriptive error message of what happened. The best course of action is likely a combination of careful argument validation, a sensible recovery strategy (make sure it doesn't make a mess or worse, give false positives on failure), and careful testing. If done right it will be unlikely that you will propagate a false `IOException` (unless the API itself is faulty, which has happened to me many times, in which case the calling code might just prefer an `IOException`). – jrh Jun 29 '18 at 19:21
1

Catching general exception is bad because it leaves your program in an undefined state. You don't know where stuff went wrong so you don't know what your program has actually done or hasn't done.

Where I would allow catching all is when closing a program. As long as you can clean it up alright. Nothing as annoying as a program you close which just throws an error dialog that does nothing but sit there, not going away and preventing your computer from closing down.

In a distributed environment your log method could backfire: catching a general exception could mean your program still holds a lock on the log-file preventing other users from making logs.

Pieter B
  • 12,867
  • 1
  • 40
  • 65
0

I understand the rationale for this rule. But, in practice, if I want to take the same action regardless of the exception thrown, why would I handle each one specifically?

As others have said, it's really difficult (if not impossible) to imagine some action that you'd want to take regardless of the exception thrown. Just one example are situations where the program's state has been corrupted and any further processing can lead to problems (this is the rationale behind Environment.FailFast).

Furthermore, if I handle specific exceptions, what if the code I'm calling changes to throw a new exception in the future? Now I have to change my code to handle that new exception. Whereas if I simply caught Exception my code doesn't have to change.

For hobby code it's just fine to catch Exception, but for professional-grade code introducing a new exception type should be treated with the same respect as a change in the method signature, i.e. be considered a breaking change. If you subscribe to this point of view then it's immediately obvious that going back to change (or verify) the client code is the only correct course of action.

For example, if Foo calls Bar, and Foo needs to stop processing regardless of the type of exception thrown by Bar, is there any advantage in being specific about the type of exception I'm catching?

Sure, because you will not be catching just exceptions thrown by Bar. There will also be exceptions that Bar's clients or even the runtime might throw during the time that Bar is on the call stack. A well-written Bar should define its own exception type if necessary so that callers can specifically catch the exceptions emitted by itself.

So what if I want to ignore every exception but OutOfMemoryException?

IMHO this is the wrong way to think about exception handling. You should be operating on whitelists (catch exception types A and B), not on blacklists (catch all exceptions apart from X).

Jon
  • 527
  • 3
  • 15
  • It's often possible to specify an action which should be performed for all exceptions that indicate that an attempted operation failed with no side-effect, and or adverse implication about the state of the system. For example, if a user selects a document file to load, a program passes its name to a "create document object with data from a disk file" method, and that method fails for some particular reason the application hadn't anticipated (but which meets the above criteria), proper behavior should be to display an "Unable to load document" message rather than crashing the application. – supercat Oct 15 '18 at 21:27
  • Unfortunately, there is no standard means by which an exception can indicate the most important thing client code would want to know--whether it implies anything bad about the state of the system beyond what would be implied by the inability to perform the requested operation. Thus, even though situations where *all* exceptions should be handled are rare, there are thus many situations in which many exceptions should be handled the same way, and there's no criterion that is satisfied by all such exceptions would also be satisfied by some rare ones that should be handled differently. – supercat Oct 15 '18 at 21:33
0

Maybe. There are exceptions to every rule, and no rule should be followed uncritically. You example might conceivably be one of the instances where it makes sense to swallow all exceptions. For example if you want to add tracing to a critical production system, and you want to make absolutely sure that you changes don't disrupt the main task of the app.

However, you should think carefully about possible reasons for failure before you decide to silently ignore them all. For example, what if the reason for the exception is:

  • a programming error in the logging method which causes it to always throw a particular exception
  • an invalid path to the logging file in the configuration
  • the disk is full

Don't you want to be notified immediately that this problem is present, so you can fix it? Swallowing the exception means you never know something went wrong.

Some problems (like the disk is full) might also cause other parts of the application to fail - but this failure is not logged now, so you never know!

JacquesB
  • 57,310
  • 21
  • 127
  • 176
0

I would like to address this from a logical perspective rather than a technical one,

Now I have to change my code to handle that new exception.

Well, someone would have to handle it. That is the idea. Writers of library code would be prudent about adding new exception types though because it is likely to break clients, you should not encounter this very often.

Your question is basically "What if I don't care what went wrong? Do I really have to go through the hassle of finding out what it was?"

Here's the beauty part: no you do not.

"So, can I just look the other way and have whatever nasty thing pops up swept under the carpet automatically and be done with it?"

No, that is not how it works either.

The point is, the collection of possible exceptions is always larger than the collection you expect and are interested in in the context of your local little problem. You handle those you anticipate and if something unexpected happens, you leave it to the higher level handlers rather than swallowing it. If you do not care about an exception you did not expect, you bet someone up the call stack will and it would be sabotage to kill an exception before it reaches its handler.

"But... but... then one of those other exceptions I do not care about could cause my task to fail!"

Yes. But they will always be more important then the locally handled ones. Like a fire alarm or the boss telling you to stop what you are doing and pick up a more urgent task that popped up.

Martin Maat
  • 18,218
  • 3
  • 30
  • 57
-2

You should catch general exceptions at the top level of every process, and handle it by reporting the bug as well as possible and then terminating the process.

You should not catch general exceptions and try to continue execution.

ddyer
  • 4,060
  • 15
  • 18