CODEWITHSUNDEEP
javajdbccoding-style
jb.
jb.

Reputation: 23955

Should I catch exceptions thrown when closing java.sql.Connection

Connection.close() may throw SqlException, but I have always assumed that it is safe to ignore any such exceptions (and I have never seen code that does not ignore them).

Normally I would write:

 try{
    connection.close();
 }catch(Exception e) {}

Or 

 try{
    connection.close();
 }catch(Exception e) {
     logger.log(e.getMessage(), e); 
 }

The question is:

  1. Is it bad practice (and has anyone had problems when ignoring such exceptions).
  2. When Connection.close() does throw any exception.
  3. If it is bad how should I handle the exception.

Comment:

I know that discarding exceptions is evil, but I'm reffering only to exceptions thrown when closing a connection (and as I've seen this is fairly common in this case).

Does anyone know when Connection.close() may throw anything?

Upvotes: 18

Views: 16844

Answers (12)

Sankar
Sankar

Reputation: 172

Its a better practice to handle the exception at the time of closing the connection to the database. Because, at later some point of time in your code, if you are trying to access the statement or resultset objects then it will automatically raise an exception. So, Better to handle the exception.

Upvotes: 0

Thorbjørn Ravn Andersen
Thorbjørn Ravn Andersen

Reputation: 75376

If you can handle it, then do so (and log it if it was unexpected). If you cannot handle it, then rethrow it properly so some code above can handle it.

Silently swallowing exceptions is leaving out crucial information for the person to fix the code.

Upvotes: 0

Brian Agnew
Brian Agnew

Reputation: 272257

Note that Apache Commons DButils provides a closeQuietly() method, that you can use to avoid cluttering your code with 'redundant' catches. Note I'm not advocating swallowing exceptions, but for this close() scenario I think it's generally acceptable.

Upvotes: 1

Bill K
Bill K

Reputation: 62769

In general, I've had days wasted by people throwing away exceptions like that.

I recommend following a few basic rules with exceptions:

If you are ABSOLUTELY SURE you will NEVER cause a problem with a checked exception, catch JUST that exception and comment exactly why you don't need to handle it. (Sleep throws an InterruptedException that can always be ignored unless you actually are interested in it, but honestly this is the only case I usually ignore--even at that, if you never get it, what's the cost of logging it?)

If you are not sure, but you may get it occasionally, catch and log a stack trace just so that if it is causing a problem, it can be found. Again, catch only the exception you need to.

If you don't see any way the checked exception can be thrown, catch it and re-throw it as an unchecked exception.

If you know exactly what is causing the exception, catch it and log exactly why, you don't really need a stack trace in this case if you are very clear as to what's causing it (and you might mention the class that's logging it if you're not already using log4j or something.

It sounds like your problem would fall into the last category, and for this kind of a catch, never do what you wrote (Exception e), always do the specific exception just in case some unchecked exception is thrown (bad parameters, null pointer, ...)

Update: The main problem here is that Checked Exceptions are ungood. The only highly used language they exist in is Java. They are neat in theory, but in action they cause this behavior of catch and hide that you don't get with unchecked exceptions.

A lot of people have commented on the fact that I said that hiding them is okay sometimes. To be specific, the one case I can think of is:

try {
    Thread.sleep(1000);
catch (InterruptedException e) {
    // I really don't care if this sleep is interrupted!
}

I suppose the main reason I feel this use is okay is because this use of InterruptedException is an abuse of the checked exception pattern in the first place, it's communicating the result of a sleep more than indicating an exception condition.

It would have made much more sense to have:

boolean interrupted=Thread.sleep(1000);

But they were very proud of their new checked exception pattern when they first created Java (understandably so, it's really neat in concept--only fails in practice)

I can't imagine another case where this is acceptable, so perhaps I should have listed this as the single case where it might be valid to ignore an exception.

Upvotes: 13

anjanb
anjanb

Reputation: 13867

Actually, what you're doing is (almost) best practice :-) here's what I saw in Spring's JdbcUtils.java. So, you might want to add another Catch block.

/**
 * Close the given  ResultSet and ignore any thrown exception.
 * This is useful for typical finally blocks in manual  code.
 * @param resultSet the  ResultSet to close
 * @see javax.resource.cci.ResultSet#close()
 */
private void closeResultSet(ResultSet resultSet) {
  if (resultSet != null) {
    try {
      resultSet.close();
    }
    catch (SQLException ex) {
      logger.debug("Could not close  ResultSet", ex);
    }
    catch (Throwable ex) {
      // We don't trust the  driver: It might throw RuntimeException or Error.
      logger.debug("Unexpected exception on closing  ResultSet", ex);
    }
  }
}

Upvotes: 17

Fábio
Fábio

Reputation: 3480

You could also throw a RuntimeException:

try {
    connection.close();
 } catch(Exception e) {
     throw new RuntimeException(e); 
 }

You won't have to change your method signature and will be able to use the Exception.getCause method later on to find the cause of the problem.

Upvotes: 1

SWD
SWD

Reputation: 289

if this is an "error that never happens" case then I will just rethrow an Exception and hope no one catches it.
if this is any other case I will probably log it

Upvotes: 0

matt b
matt b

Reputation: 139921

At the very minimum, always always always log exceptions that you are catching and not acting on.

Silently caught exceptions that are swallowed without the tiniest peep are the worst.

Upvotes: 7

Robin
Robin

Reputation: 24262

In an ideal world, you should never do nothing on an exception, of course, in an ideal world, you would never get an exception either 8-)

So, you have to examine the impacts of the various options.

Log only: Database operations are all finished, nothing left to do but clean up the resources. If an exception occurs at this point, it most likely has no impact on the work performed, so logging the error should suffice. Of course, if an error occurs during logging, then you basically have to handle failed database operation that didn't actually fail.

Empty handler: Database operations are all finished, nothing left to do but clean up the resources. If an exception occurs at this point, it most likely has no impact on the work performed, so the method returns successfully. The next database access may run into the same problem, but it should occur at the start of a transaction, where it will rightly fail and then get handled appropriately. If the problem has fixed itself, then there will be no indication that anything ever went wrong.

It is a pretty typical scenario to put a close() operation(s) in a finally block to ensure that cleanup occurs, since we don't want any other failures to inhibit the resource cleanup. If no error has occurred, then your method should not fail when its operation has successfully completed. In this case, empty exception handling is quite normal.

Of course opinions will vary.

Upvotes: 1

a-sak
a-sak

Reputation: 1340

From my experience ignoring an exception is never a good idea. Believe me, the production support engineers and analysts will thank you a tonne if you logged the exception.

Also, if you are using the right Logging framework, there would be zero or minimal performance impact of the exception.

Upvotes: 0

Guido
Guido

Reputation: 47665

You have to handle the exception. It is not a bad practice. Imagine you lost the network just before closing the dabatase connection. It will probably throw the exception.

Is it rare ? Yes. I suppose that's what they are called exceptions and that is not a reason to ignore it. Remember that if it could fail, it will fail.

You should also think about whether it is possible to have a null connection at this point (it would cause a NullPointerException) or not.

if (connection != null) {
   try { 
      connection.close(); 
   } catch (SQLException sqle) { 
      logger.log(e.getMessage(), e); 
   }
}

Upvotes: 1

Jeremy
Jeremy

Reputation: 46350

I personally like your second idea of at least logging the error. Because you're catching Exception, it's theoretically possible to catch something other than a SQL Exception. I'm not sure what could happen or how rare (like out of memory exceptions, etc) but supressing all errors doesn't seem right to me.

If you want to suppress errors, I would do it only to very specific ones you know should be handled that way.

Hypothecial situation: what if your sql had an open transaction, and closing the connection caused an exceptino because of that, would you want to suppress that error? Even suppressing SQLExceptions might be a little dangerous.

Upvotes: 3

Related Questions