About good exception handling practices?

[Edwin Dalorzo]

Hi, Ranchers

I started with Java like a year and a half ago and I started to develop my first real-world applications like 6 months ago. Hence, as you can imagine, I need to gain experience...

I still get a bit confused when it comes to the right practices for exception handling. I have read a lot about the subject, but obviously not enough to take me out of doubts sometimes.

What about this case:

I am writing an small class that execute batch SQL scripts.

01. try{
02.   connection = datasource.getConnection();
03.   connection.setAutoCommit(false);
04.   for(...){
05.       //TODO: execute some statements here
06.   } 
07.   connection.commit();
08.   connection.setAutoCommit(true);
09. } catch (SQLException e) {
10.  try{
11.    connection.rollback();
12.  }catch (SQLException s) {
14.    //TODO: Question-> What's the point here? 
15.  }
16. }finally{
17.  try{
18.    connection.close();
19.  }catch (SQLException e) {
20.    //TODO: Question-> What's the point here? 
21.  }
23. }

What would you guys do in lines 14 and 21?

There is no point here, I can't think of anything to recover from the exception, it does not make much of a sense to propagate the exception either, I frankly don�t see it would be of a great help if I log it. And supposedly, just ignoring a caught exception is not exactly a good practice.
[ May 03, 2006: Message edited by: Edwin Dalorzo ]

[Paul Clapham]

It's unlikely that those exceptions will ever occur. But if they do, who should be notified? Not the user, who was just trying to change the time of their appointment or something. What do they know? It's the people in the back room -- you or the people who keep the systems running -- who want to know.

Probably if those exceptions do occur, it's because something really weird is happening. And it's likely that the operations people already know, because there are more visible errors occurring. But just in case there's a database bug that only fails to close connections in some unlikely circumstance, maybe you ought to log it.

I use log4j and I have it configured so error-level logging entries are e-mailed to somebody. This might be a place to log an error, even if it may almost never happen.

Although I have to say, mostly I leave those catch-blocks empty. Wouldn't be hard to throw a single line of logging code in there, but laziness conquers all.

[Edwin Dalorzo]

Hi, Paul.

I agree with the logging point of view the way you presented it. Altough I have to admit that I was using empty catch blocks too for these cases.

I have also seen an interesting approach used by the ThreadGroup class to handle uncaught exceptions, which is registering a class to handle the exceptions or at least a class that is notified that something happened.

I have been experimenting with a exception handler like this in a small class library I am building.

What do you think of this approach?
[ May 03, 2006: Message edited by: Edwin Dalorzo ]

[Jim Yingst]

I'm a big advocate of carefully logging any exception which you don't completely understand the cause of at the time you catch it. Which is to say, almost all exceptions - though there are a few situations where you know the cause and you don't care about the stack trace. (InterruptedException is a common example.)

But going back to the original code here, there are two significant problems. The really big one is: what about the very first catch block for SQLException e, on line 9? Something went wrong in the preceding SQL code... and then you do a rollback, and you close the connection, and probably nothing will go wrong there, and if it does there's not much you can do about it anyway. OK. But what about the original exception? Something went wrong, And after this code runs, THERE IS NO INDICATION ANYWHERE THAT A PROBLEM HAS OCCURRED. No error message. No log file. If someone eventually looks at the database, they may notice that some expected data is missing. Or not. But it's going to take some work for someone to figure out what went wrong here. With luck, that person will be you. Because if it's someone else, and they spend a long time tracking this down before they find this one bit of code here that has successfully hidden the evidence of the error, and they find out you are the author...

Well, that's the sort of thing that motivates hardworking programmers to lock their co-workers in empty rooms and force them to listen for hours to Barry Manilow recordings. Or other similar forms of torture.

Seriously, exceptions contain valuable, useful information. Typically including actual error messages in addition to the stack trace itself. This is really good stuff to have a record of when things go wrong. You never, ever want to just lose an exception just because you weren't sure what to do with it. That just makes it much harder to debug problems later. Using a logging framework is a good idea - but even if you don't have one set up yet, at the very least you can call e.printStackTrace(). That usually does a pretty good job of dumping all the info you need someplace it will probably be found. Much better than doing nothing.

In the above code, you weren't doing nothing, but you let confusion over what to do about the second and third catch blocks (lines 12 and 19) prevent you from logging the most important exception, the one on line 9. That's the one that would tell you what SQL error actually started whatever problem you may be having.

I mentioned two problems. The other one is, what if there's an error on line 2? You'll go to the catch block, and then the finally block... but the connection is null. So lines 11 and then 18 will both throw NullPointerException. Oops. There are two was to avoid this. One is to put "if (conn == null)" checks before the rollback() and the close(). The other, which I prefer, is to place try/catch/finally blocks carefully such that the rollback and close do not occur at all unless getConnection() succeeds:

    static void foo(DataSource datasource) {
        try {
            Connection conn = datasource.getConnection();
            try {
                conn.setAutoCommit(false);
                executeSQL();
                conn.commit();
                conn.setAutoCommit(true);
            } catch (SQLException e) {
                rollback(conn);
            } finally {
                close(conn);
            }
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }
 
    private static void executeSQL() {
        // TODO: implement this
    }
 
    private static void rollback(Connection conn) {
        try {
            conn.rollback();
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }
 
    private static void close(Connection conn) {
        try {
            conn.close();
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }

Of course e.printStackTrace() can be replaced with proper logging - as long as you do something.
[ May 04, 2006: Message edited by: Jim Yingst ]

[Edwin Dalorzo]

Excellent, nifty advises, Jim. Certainly I am going to follow them all.

Thanks for taking your time to write such a complete answer.

[Stan James]

At first blush, I like your idea of an exception handler. You could plug in different implementations for development and production or tune the production one to different levels of logging in a running system. It might reduce duplication throughout your system.

Edwin, tell us more about what your handler does!

[Edwin Dalorzo]

Hi, Stan. Thanks for participating.

Well, I was building a very small data access layer based on transactions scripts.

I am using Jakarta Commons DBUtils and I have an implementation for an interface I call SQLTask which have an execute method that I implemented somewhat like this:

01. public Integer execute() {
02.   int result = -1;
03.   QueryRunner qr = new QueryRunner(ds);
04.   try {
05.      result = qr.update(getSQL());
06.   } catch (SQLException e) {
07.      if (handler != null) {
08.         handler.handleSQLException(this, e);
09.      }
10.   }
11.   return result;
12.}

Then I have an interface for the exception handler, somewhat like this:

01. public interface SQLExceptionHandler {
02.        void handleSQLException(SQLTask task, SQLException e);
03.    }

And, I use it somewhat like this:

01. BasicSQLTask task = new BasicSQLTask(ds, new Queryable() {
02.     public String getSQL() {
03.         return "DELETE FROM OWNER.TABLE";
04.     }
05.  });
06. task.setSQLExceptionHandler(new SQLTask.SQLExceptionHandler() {
07.     public void handleSQLException(SQLTask task, SQLException e) {
08.         System.out.println("Exception happened: " + e.getMessage());
09.     }
10. });
11. System.out.println("Rows affected: " + task.execute());

I am using the exception handler in line 06. Of course, a better idea would be sharing the execption handler with all SQLTasks, however as this is a library, I was just running some tests.

I am not sure this approach is a good idea. Altough, until now, seems to work fine.

What do you think, Stan? Any ideas about how to do it better?

Thanks in advance to you all for your participation!
[ May 04, 2006: Message edited by: Edwin Dalorzo ]

[Stan James]

I was reading some Spring code before I saw this and realized they did a neat job of moving little bits of logic into their own classes so you could easily plug together different combinations. I find it appealing to delegate as much housekeeping as possible to little classes and keep the main classes focused on their jobs. I'll have to ponder the exception handler a little longer.

I also just read a rant about "handler", "helper" and "manager" objects. Some folks don't seem to like them at all. Spoilsports.