46

Consider the code:

PreparedStatement ps = null;
ResultSet rs = null;
try {
  ps = conn.createStatement(myQueryString);
  rs = ps.executeQuery();
  // process the results...
} catch (java.sql.SQLException e) {
  log.error("an error!", e);
  throw new MyAppException("I'm sorry. Your query did not work.");
} finally {
  ps.close();
  rs.close();
}

The above does not compile, because both PreparedStatement.close() and ResultSet.close() throw a java.sql.SQLException. So do I add a try/catch block to the finally clause? Or move the close statements into the try clause? Or just not bother calling close?

Tom Hawtin - tackline
  • 145,806
  • 30
  • 211
  • 305
MCS
  • 22,113
  • 20
  • 62
  • 76

13 Answers13

54

In Java 7, you should not close them explicitly, but use automatic resource management to ensure that resources are closed and exceptions are handled appropriately. Exception handling works like this:

Exception in try | Exception in close | Result
-----------------+--------------------+----------------------------------------
      No         |        No          | Continue normally
      No         |        Yes         | Throw the close() exception
      Yes        |        No          | Throw the exception from try block
      Yes        |        Yes         | Add close() exception to main exception
                 |                    |  as "suppressed", throw main exception

Hopefully that makes sense. In allows pretty code, like this:

private void doEverythingInOneSillyMethod(String key)
  throws MyAppException
{
  try (Connection db = ds.getConnection()) {
    db.setReadOnly(true);
    ...
    try (PreparedStatement ps = db.prepareStatement(...)) {
      ps.setString(1, key);
      ...
      try (ResultSet rs = ps.executeQuery()) {
        ...
      }
    }
  } catch (SQLException ex) {
    throw new MyAppException("Query failed.", ex);
  }
}

Prior to Java 7, it's best to use nested finally blocks, rather than testing references for null.

The example I'll show might look ugly with the deep nesting, but in practice, well-designed code probably isn't going to create a connection, statement, and results all in the same method; often, each level of nesting involves passing a resource to another method, which uses it as a factory for another resource. With this approach, exceptions from a close() will mask an exception from inside the try block. That can be overcome, but it results in even more messy code, and requires a custom exception class that provides the "suppressed" exception chaining present in Java 7.

Connection db = ds.getConnection();
try {
  PreparedStatement ps = ...;
  try {
    ResultSet rs = ...
    try {
      ...
    }
    finally {
      rs.close();
    }
  } 
  finally {
    ps.close();
  }
} 
finally {
  db.close();
}
erickson
  • 265,237
  • 58
  • 395
  • 493
  • 6
    It'd be less ugly if you put the finally on the same line as the closing brace. ;) – Tom Hawtin - tackline Nov 26 '08 at 20:08
  • 9
    Ctrl-Shift-F to your heart's content! ;) – erickson Nov 26 '08 at 21:24
  • It is a bad idea if you do not close statements. If you use connection pools your code will end up with `ORA-01000: maximum open cursors exceeded`. Closing statements helps, because Oracles Universal Connection Pool (UCP) does also statement pooling. – ceving Sep 23 '13 at 13:09
  • 3
    @ceving No one has suggested that closing statements may be avoided. What are you responding to? – erickson Sep 23 '13 at 17:04
  • Closing the PreparedStatement should suffice: the (underlying ResultSet gets closed upon closing the PreparedStatement. See: http://docs.oracle.com/javase/7/docs/api/java/sql/ResultSet.html#close() – roomsg Jan 29 '15 at 15:28
  • 2
    @roomsg That's true, but if you are using a `PreparedStatement`, the `ResultSet` objects are very likely to be created inside a loop, so ensuring that they are closed like this is still good code hygiene. If you create a lot of result sets before closing the prepared statement, you can still have problems, even though they will be closed eventually. – erickson Jan 29 '15 at 15:37
  • @erickson, True, you're right, ico loops it's better to handle/close each resultset separately, but that does not really apply to the given example. Just wanted to notify to anybody reading your (great !) answer. – roomsg Jan 29 '15 at 15:54
31

If you're really hand-rolling your own jdbc it definitely gets messy. The close() in the finally needs to get wrapped with its own try catch, which, at the very least, is ugly. You can't skip the close, although the resources will get cleared when the connection is closed (which might not be right away, if you're using a pool). Actually, one of the main selling points of using a framework (e.g. hibernate) to manage your db access is to manage the connection and result set handling so you don't forget to close.

You can do something simple like this, which at least hides the mess, and guarantees that you don't forget something.

public static void close(ResultSet rs, Statement ps, Connection conn)
{
    if (rs!=null)
    {
        try
        {
            rs.close();

        }
        catch(SQLException e)
        {
            logger.error("The result set cannot be closed.", e);
        }
    }
    if (ps != null)
    {
        try
        {
            ps.close();
        } catch (SQLException e)
        {
            logger.error("The statement cannot be closed.", e);
        }
    }
    if (conn != null)
    {
        try
        {
            conn.close();
        } catch (SQLException e)
        {
            logger.error("The data source connection cannot be closed.", e);
        }
    }

}

and then,

finally {
    close(rs, ps, null); 
}
Keith Pinson
  • 7,835
  • 7
  • 61
  • 104
Steve B.
  • 55,454
  • 12
  • 93
  • 132
  • This solution works very well if you're rolling your own, because the SQLExceptions thrown by the close methods are unrecoverable anyway. Another neat way to go about it, is to throw a RuntimeException subclass from this method which bubbles up to a code layer that can manage the DB failure. – Spencer Kormos Nov 26 '08 at 20:09
  • 10
    Note that according to the JavaDoc for Statement, the ResultSet is closed as a side-effect of closing the statement so it's not strictly necessary in this example (but doesn't hurt). http://java.sun.com/javase/6/docs/api/java/sql/Statement.html#close() – hallidave Apr 30 '09 at 15:16
  • I wrote it this way because it's useful to be able to flexibly close with some of the objects as null. So you can use the same function to close any subset of (statement, resultset, connection) objects. – Steve B. Apr 30 '09 at 16:18
  • Always close statements, leaving them open after connection is closed, might produce problems: see more at http://stackoverflow.com/questions/321418/where-to-close-java-preparedstatements-and-resultsets. – jb. May 05 '09 at 20:43
  • You shouldn't ignore exceptions from closing a resource. In the case of a JDBC operation, for example, it means that whatever you did inside your `try` block probably didn't really happen. The approach taken by the ARM implementation is exemplary: throw `close()` exception if it's the only one, otherwise preserve it by chaining to the main exception. – erickson Oct 30 '12 at 19:39
  • 1
    Just looked at this again - this is a very old answer, and was written in Java6 days. You could probably write this a bit more neatly in Java7 using AutoCloseables, if the relevant objects support it. – Steve B. Nov 09 '13 at 05:49
  • @SteveB. Thanks, I was wondering about that, erickson below has an answer using AutoCloseables. Cleaned my legacy code up nicely. Since yours is the accepted answer, you might edit it to point viewers to his :) – Nobbynob Littlun May 08 '14 at 22:51
  • Using 7, but I like this, thanks @SteveB. Straightforward and simple. – k_rollo Dec 08 '14 at 05:46
12

For file I/O, I generally add a try/catch to the finally block. However, you must be careful not to throw any exceptions from the finally block, since they will cause the original exception (if any) to be lost.

See this article for a more specific example of database connection closing.

Michael Myers
  • 188,989
  • 46
  • 291
  • 292
8

Don't waste your time coding low-level exception management, use an higher-level API like Spring-JDBC, or a custom wrapper around connection/statement/rs objects, to hide the messy try-catch ridden code.

BraveSirFoobar
  • 794
  • 1
  • 6
  • 12
  • I also agree with this. Spring will wrap checked exception into unchecked, because in most cases application cannot recover from DB exceptions. – dma_k May 06 '10 at 16:09
8

Also note:

"When a Statement object is closed, its current ResultSet object, if one exists, is also closed. "

http://java.sun.com/j2se/1.5.0/docs/api/java/sql/Statement.html#close()

It should be sufficient to close only the PreparedStatement in a finally, and only if it is not already closed. If you want to be really particular though, close the ResultSet FIRST, not after closing the PreparedStatement (closing it after, like some of the examples here, should actually guarantee an exception, since it is already closed).

  • 4
    According to the documentation you reference, calling Statement.close() on an already-closed Statement has no effect. Closing a Statement multiple times doesn't throw an exception, so it stands to reason that doing the same for a ResultSet should be equally innocuous. The ResultSet documentation doesn't explicitly say this, but it also doesn't say that you shouldn't close it more than once...The whole point of this pedantic rant is that it doesn't GUARANTEE an exception. Although it would be good to close the ResultSet first just in case some implementation behaves that way. – A. Levy Oct 12 '09 at 14:52
  • 3
    I can't help myself. I have to point out that this is a really useful answer, and the one above that says "use a framework" just makes me cringe. – stu Jun 07 '12 at 19:44
5

I usually have a utility method which can close things like this, including taking care not to try to do anything with a null reference.

Usually if close() throws an exception I don't actually care, so I just log the exception and swallow it - but another alternative would be to convert it into a RuntimeException. Either way, I recommend doing it in a utility method which is easy to call, as you may well need to do this in many places.

Note that your current solution won't close the ResultSet if closing the PreparedStatement fails - it's better to use nested finally blocks.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
5

Building on @erickson's answer, why not just do it in one try block like this?

private void doEverythingInOneSillyMethod(String key) throws MyAppException
{
  try (Connection db = ds.getConnection();
       PreparedStatement ps = db.prepareStatement(...)) {

    db.setReadOnly(true);
    ps.setString(1, key);
    ResultSet rs = ps.executeQuery()
    ...
  } catch (SQLException ex) {
    throw new MyAppException("Query failed.", ex);
  }
}

Note that you don't need to create the ResultSet object inside the try block as ResultSet's are automatically closed when the PreparedStatement object is closed.

A ResultSet object is automatically closed when the Statement object that generated it is closed, re-executed, or used to retrieve the next result from a sequence of multiple results.

Reference: https://docs.oracle.com/javase/7/docs/api/java/sql/ResultSet.html

Catfish
  • 18,876
  • 54
  • 209
  • 353
3

If your are using Java 7 you can use the improvements in the exception handling mechanisms in those classes that implement AutoCloseable (i.e. PreparedStatement, Resultset)

You might also find this question interesting: Closing ResultSet in Java 7

Community
  • 1
  • 1
Guido
  • 46,642
  • 28
  • 120
  • 174
1

I know this is an old question, but just in case someone is looking for the answer, java now has the try-with-resouce solution.

static String readFirstLineFromFile(String path) throws IOException {
      try (BufferedReader br =
                   new BufferedReader(new FileReader(path))) {
        return br.readLine();
    }
}
Xin
  • 737
  • 3
  • 10
  • 27
  • This is the same as Guido Garcia´s answer. Try with resource requires the resource to be AutoCloseable. – ceving Sep 23 '13 at 13:24
0

focus finally clause,

finally {
   try {
      rs.close();
      ps.close();
   } catch (Exception e) {
      // Do something
   }
}

I think you have to modify 2 points.

First, use try & catch again in fainlly clause.

Second, do rs.close() before doing ps.close().

fly1997@naver.com

Kris
  • 11
0

Probably an old (though simple) way to do things, but it still works:

public class DatabaseTest {

    private Connection conn;    
    private Statement st;   
    private ResultSet rs;
    private PreparedStatement ps;

    public DatabaseTest() {
        // if needed
    }

    public String getSomethingFromDatabase(...) {
        String something = null;

        // code here

        try {
            // code here

        } catch(SQLException se) {
            se.printStackTrace();

        } finally { // will always execute even after a return statement
            closeDatabaseResources();
        }

        return something;
    }

    private void closeDatabaseResources() {
        try {
            if(conn != null) {
                System.out.println("conn closed");
                conn.close();
            }

            if(st != null) {
                System.out.println("st closed");
                st.close();
            }

            if(rs != null) {
                System.out.println("rs closed");
                rs.close();
            }

            if(ps != null) {
                System.out.println("ps closed");
                ps.close();
            }

        } catch(SQLException se) {
            se.printStackTrace();
        }               
    }
}
k_rollo
  • 5,304
  • 16
  • 63
  • 95
0

Do no omit calling close. It may cause problems.

I prefer adding try/catch block to the finally.

asalamon74
  • 6,120
  • 9
  • 46
  • 60
-2

I use this..

finally
{
    if (ps != null) ps.close();
    if (rs != null) rs.close();
}
Chris Nava
  • 6,614
  • 3
  • 25
  • 31