24

Ok I know catching throwable is not a good idea:

    try {
         // Some code
    } catch(Throwable e) { // Not cool!
        // handle the exception
    }

But recently I was reading through an open sourced code and I saw this interesting (at least to me) piece of code:

    try {
        // Some Code
    } catch (Throwable ex){
        response = handleException(ex, resource);
    }

    private handleException(Throwable t, String resource) {
        if (t instanceof SQLEXception) {
               // Some code
        } else if (t instanceof IllegalArgumentException) {
               //some code
        } //so on and so forth
    }

This doesn't seem to be that bad? What is wrong with this approach?

CKing
  • 14,153
  • 4
  • 39
  • 77
rhel.user
  • 1,358
  • 1
  • 12
  • 17
  • 4
    Sometimes, particularly in frameworks, you have no other option than to catch `Throwable`. It still isn't a good idea in application-level code, unless you want to have some kind of "handler of last resort". – biziclop Jul 23 '15 at 10:56
  • Have you got a link to the open sourced code in question (such as the source file on GitHub)? I can think of a couple of use cases where, depending on the context, this may actually be a good strategy. – James_pic Jul 23 '15 at 13:46
  • @James_pic there you go https://github.com/Eldelshell/jongo/blob/master/jongo-core/src/main/java/jongo/RestController.java – rhel.user Jul 23 '15 at 14:18

9 Answers9

21

There are various reasons why you should not catch a Throwable. First of all is, that Throwable includes Errors - and there's normally not much an application can do if one of these appears. Also Throwable reduces your chances of finding out, WHAT has happened. All you get is "something bad has happened" - which might be a catastrophe or just a nuisance.

The other aproach is better but of course I still would not catch Throwable, but try to catch more specific Exceptions, if possible at all. Otherwise you are catching everything and then try to sort out which kind of bad thing happened. Your example could be written as...

try {
    ...
} catch (SQLEXception ex){
    response = ... ;
} catch (IllegalArgumentException ex){
    response = ...;
}

...which would reduce the amount of if ( ... instanceof ... ) blocks (which are only needed because the author first decided to catch everything in one big bucket). It something actually throws Throwable, then you don't have much choice, of course.

Florian Schaetz
  • 9,670
  • 3
  • 27
  • 50
  • 2
    The problem is that there are `Error`s that are perfectly easy to recover from. If a framework tries to load a class and it fails, things could still be fine. – biziclop Jul 23 '15 at 11:06
  • In such a case the framework should not produce an error, but a more meaningfull exception. Of course, that's not always the case, true. – Florian Schaetz Jul 23 '15 at 11:20
  • 3
    No, I meant the framework itself has to catch the error and deal with it. Unfortunately the authors of Java didn't differentiate between recoverable and non-recoverable errors, so sometimes (very rarely) catching `Throwable` is the only sensible option. – biziclop Jul 23 '15 at 11:59
  • 1
    Ah, ok. Yes, you are right there, but these are very, very specific situations for very low-level use cases. This is why I wrote that "normally" an application can't do much. If you are the Spring framework and encounter an Error, there might be something you can do. If you are a HelloWorld application, your best choice is to simply take a holiday while someone fixes your bugs ;-) – Florian Schaetz Jul 23 '15 at 12:01
  • @FlorianSchaetz If you are a web server and you encounter an Error handling a request, you want to log it and abort that request only, without bringing down the server. (Unless *every* request throws an Error) – user253751 Jul 23 '15 at 22:06
19

You are right when you say that catching Throwable is not a good idea. However, the code that you present in your question is not catching Throwable in an evil way but let's talk about that later. For now, the code that you present in your question has several advantages :

1. Readability

If you look at the code carefully, you will notice that even though the catch block is catching a Throwable, the handleException method is checking the type of exception thrown and possibly taking different actions based on the exception type.

The code presented in your question is synonymous to saying:

try {
    doSomething();
} catch (SQLEXception ex){
    response = handleException(resource);
} catch(IllegalArgumentException ex) {
    response = handleException(resource);
} catch(Throwable ex) {
    response = handleException(resource);
}

Even if you have to catch 10+ exceptions only, this code can easily take up a lot of lines of code and the multi-catch construct is not going to make the code any cleaner. The code that you present in your question is simply delegating the catch to another method to make the actual method that does the work more readable.

2. Reusability

The code for the handleRequest method could easily be modified and placed in a utility class and accessed throughout your application to handle both Exceptions and Errors. You could even extract the method into two private methods; One that handles Exception and one that handles Error and have the handleException method that takes a Throwable further delegate the calls to these methods.

3. Maintainibility

If you decide that you want to change the way you log an SQLExceptions in your application, you have to make this change in a single place rather than visiting every method in every class that throws an SQLException.

So is catching Throwable a bad idea?

The code that you present in your question is not really the same as catching Throwable alone. The following piece of code is a big no-no:

try {
   doSomething();
} catch(Throwable e) {
    //log, rethrow or take some action
}

You should catch Throwable or Exception as far away in the catch chain as possible.

Last but not the least, remember that the code you present in your question is framework's code and there are certain errors that the framework can still recover from. See When to catch java.lang.Error for a better explanation.

Community
  • 1
  • 1
CKing
  • 14,153
  • 4
  • 39
  • 77
  • Why would you use `Throwable` for that and not just `Exception`? The method would have to be called handleThrowable... – maraca Jul 23 '15 at 11:06
  • 2
    @maraca Remember that this is framework code. Fameworks would want to catch errors that are recoverable. (Linkage errors being a prime example). That being said, I agree that the method name they used is not really a good choice here but that's not related to the actual question. – CKing Jul 23 '15 at 11:10
  • Thanks I see. But I think it is a design issue, couldn't come up with one scenario where this would be a nice approach. Instead of catching everything and calling this method they probably should just use throws and handle at the correct place. – maraca Jul 23 '15 at 11:47
  • 2
    @maraca Most of the exceptions they are catching are subclasses of `RuntimeException`. `RuntimeException`s don't need to follow the catch or specify requirement so the framework guys can't guarantee that the users of their API will handle these exceptions. They probably might be wrapping these exceptions into checked exceptions and throwing them or they might be handling these exceptions on their behalf. Bottom line, no point declaring that a method `throws RuntimeException` in this case IMO. – CKing Jul 23 '15 at 11:52
9

Catching Throwables out of laziness is a bad idea.

This was particularly tempting before try-multi-catch was introduced.

try {
   ...
} catch (SomeException e) {
   //do something
} catch (OtherException e) {
   //do the same thing
} ...

Repeating catch blocks is tedious and verbose, so some people decided to just catch Exception or Throwable and be done with it. This is what should be avoided because:

  1. It makes it difficult to follow what you're trying to do.
  2. You may end up catching a lot of stuff you can't deal with.
  3. You deserve bonus punishment if you completely swallow the Throwable in the catch block. (And we've all seen code that does that...:))

But catching Throwables when it is absolutely necessary is fine.

When is it necessary? Very rarely. In framework-style code there are various scenarios (dynamically loading an external class is the most obvious one), in a standalone application a typical example is to attempt to display/log some kind of error message before exiting. (Bearing in mind that the attempt may fail, so you don't want to put anything critical there.)

As a rule of thumb, if there's nothing you can do about an exception/error, you shouldn't catch it at all.

biziclop
  • 46,403
  • 12
  • 73
  • 97
3

You posted a link to Jongo, which demonstrates one possible use for this technique: re-using error handling code.

Let's say you've got a large block of error handling code that naturally repeats itself in various places in your code - for example Jongo produces standard responses for some standard classes of errors. It may be a good idea to extract that error handling code into a method, so you can re-use it from all the places it's needed.

However, that's not to say that there's nothing wrong with Jongo's code.

Catching Throwable (rather than using multicatch) is still suspicious, as you're likely to catch Errors that you're not really in a position to handle (are you sure you meant to catch ThreadDeath?). In this situation, if you absolutely have to catch Throwable, it'd be better to "catch and release" (i.e, rethrow anything that you didn't mean to catch). Jongo doesn't do this.

James_pic
  • 3,051
  • 16
  • 23
3

There are exactly two valid uses for using a huge net:

  • If you will handle everything uniformly, like a top-level catch for logging/reporting, possibly followed by an immediate exit.

  • To reduce duplication, by exporting all the handling into its own method.
    Catch the most derived common ancestor there is to avoid extra-work and increase clarity.
    DRY is an important design principle.

In both cases, unless you expected that exception and handled it completely, rethrow.

Deduplicator
  • 41,806
  • 6
  • 61
  • 104
2

First of all, catching Throwable makes your application rather intransparent. You should be as explicit as possible on catching exceptions to enable good traceability in exceptional cases.

Let's have a look at the method handleException(...) and see some of the problems that occur by this approach:

  • you catch Throwable but you only handle Exceptions, what happens if an e.g. OutOfMemoryError of type Error is thrown? - I see bad things to happen...
  • Regarding good object oriented programming using instanceof breaks the Open-Closed-Principle and makes code changes (e.g. adding new exceptions) really messy.

From my point of view, catch-blocks are exactly made for the functionality that are tried to cover in handleExceptions(...), so use them.

crazzle
  • 261
  • 3
  • 12
2

Java 7 solves a bit of the tedium that is multi-catching of similar exceptions with similar handling. You definitely should not be doing what the person has done here. Just catch the appropriate exceptions as needed, it may look ugly but then that's what throws is for, pass it to the method that should catch it and you shouldn't be wasting too much code space.

Check out this link for more information.

insidesin
  • 727
  • 1
  • 7
  • 25
2

Just to provide balance - there is one place where I will always catch (Throwable):

public static void main(String args[]) {
    try {
        new Test().test();
    } catch (Throwable t) {
        t.printStackTrace(System.err);
    }
}

At least something shows somewhere that something went wrong.

Jean-François Savard
  • 19,624
  • 6
  • 42
  • 70
OldCurmudgeon
  • 60,862
  • 15
  • 108
  • 197
0

You can always catch different type of exceptions and perform some operations based on the type of the exception you got.

Here is an example

          try{

              //do something that could throw an exception

             }catch (ConnectException e) {
                //do something related to connection


            } catch (InvalidAttributeValueException e) {
                // do anything related to invalid attribute exception

            } catch (NullPointerException e) {

                // do something if a null if obtained
            }

            catch (Exception e) {
            // any other exception that is not handled can be catch here, handle it here

            }
            finally{
          //perform the final operatin like closing the connections etc.
             }
Navankur Chauhan
  • 343
  • 5
  • 19