7

I'm doing some refactoring for our application, and trying to reduce the number of issues reported in our ISO Scan (a static code analysis tool based on HP Fortify). Right now, what I'm trying to address is the "Unreleased Resource: Database" issues our application has built up. One of the most prominent reasons for this is constructs like this:

Connection conn = null;
Boolean myConn = false;
try{
   if(conn == null){
     conn = DatabaseUtil.getConnection();
     myConn = true;
   }
   result = DbClass.getObject(conn, otherParameters);
}catch(DatabaseException de){
  throw de;
}catch(SQLException sqle){
  throw new DatabaseException("Error Message");
}finally{
  if(myConn && conn != null){
    try{
      conn.rollback();
      conn.close();
    }catch(SQLException sqle){}
}

This is a fairly standard construct in our application to ensure the database connection is closed at the end of our Try block. Yet every time we use it, the tool reports it as an issue.

Is there anything incorrect about this construct, and is there anything we can do to fix it if it is an issue?

  • 5
    java7? if so go with try-with-resources – ratchet freak Jun 4 '15 at 15:01
  • 1
    What does your ISO scan complain about, other than the construct's existence? Does it come with some sort of explanation as to why it's a problem? – Robert Harvey Jun 4 '15 at 15:01
  • @RobertHarvey They haven't complained about it so far, but because it's a large application, it's a significant portion of the issues reported by our scan (Diplomatically, less than 9999) and it concerns me because I'm wondering if there's a reason beyond it not recognizing the explicit close in the Finally block, which upon reviewing the ISO report is exactly how it recommends handling this issue. – Zibbobz Jun 4 '15 at 15:04
  • Forgive me for being ignorant, but what exactly is an ISO scan? Can you post a unique portion of the error message so that I have something Googlable? – Robert Harvey Jun 4 '15 at 15:06
  • 1
    In addition to what @StevenPessall said, you're releasing resources in another try block, so execution is not guaranteed from a static code analysis perspective. – Robert Harvey Jun 4 '15 at 15:19
10

And the tools are right!

Apart from the (possible) limitations of the analysis - it may or may not understand that (conn != null) == myConn, observe that rollback throws an SqlException. If it does, then close is never called, which is precisely the definition of a resource leak. Now, it may be the case that with your current DB, drivers and pooling libraries if rollback throws, then the connection is already released, but this is not true in the general case, hence a potential problem.

As noted in comments you can simplify much of this construct by using try-with-resources if using Java 7. It also becomes much simpler to reason about and include a rollback via a custom class that does rollback in its close() method in a try block:

@Override
public void close() throws SQLException {
    try {
        rollback();
    catch (...) {
        //Do stuff
    finally {
        super.close();
    }
}

Here you are 100% guaranteed that close() will actually run, whatever rollback does.

If working with legacy software, the above code can be directly put into the try:

Connection conn = null;
try {
   conn = DatabaseUtil.getConnection();
   result = DbClass.getObject(conn, otherParameters);
} catch(SQLException sqle) {
  throw new DatabaseException("Error Message");
} finally {
  if (conn != null) {
    try{
        conn.rollback();
    } catch(SQLException sqle) {
        //try-with-resources would add this to suppressed exceptions, you can either log or rethrow
    } finally {
        conn.close();
    }
}
  • 2
    Finally an answer with try-with-resources. It has only been around since 2011 and was designed specifically for cases like this. – user22815 Jun 4 '15 at 15:52
  • Sadly working with Legacy application, so not up to Java 7 yet. But this will be useful when we're up to that version (and for anyone who has Java 7) and offers a good suggestion for those of us (me) who aren't. – Zibbobz Jun 4 '15 at 16:05
  • 3
    @Zibbobz The semantics of try-with-resources are well-known, and while not directly translatable to Java 6, you can get very close. I answered a couple of such questions on SO. Oracle put an explanation up ages ago with the full semantics: in essence you would put another try in the finally block, with rollback() in the body and close() in the inner finally – Ordous Jun 4 '15 at 16:09
  • @Snowman don't get your hopes up. The .NET people have this for 15 years and still don't always use it. It boggles the mind. – usr Jun 4 '15 at 18:53
  • 1
    @Zibbobz Ah, I forgot you're wrapping all exceptions in a DatabaseException. You'd need to handle the case of close throwing it as well then (either by wrapping too, or throws on the method). Don't forget - you might not want to wrap and rethrow it if you've already got an exception from the try body or rollback (as that would mask the original)! The oracle blog from my comment above has the "proper" structure to use to catch and deal with all exceptions, and it is a horrendous 3-layered try - one of the reasons twr was made. – Ordous Jun 4 '15 at 20:30
5

Because your release of resources is conditional on the state of a boolean variable and encased in another try block, the static analyzer must be deciding that rollback() and close() are not guaranteed to execute.

Check the documentation for the Connection object of the type returned by the getConnection() factory method, and see if the methods rollback() and close() will even throw an exception. If they won't, you don't need that additional try catch block when calling those methods.

But first, eliminate the boolean variable that you're using to signal an open connection. You shouldn't need it; you shouldn't get a connection object back from the getConnection() factory method unless it was successful at establishing a connection.

Note that Fortify's security report suggests the following code structure:

public void execCxnSql(Connection conn) {
    Statement stmt;
    try {
        stmt = conn.createStatement();
        ResultSet rs = stmt.executeQuery(CXN_SQL);
        ...
    } 
    finally {
        if (stmt != null) {
            safeClose(stmt);
        }
    }
}
public static void safeClose(Statement stmt) {
    if (stmt != null) {
        try {
            stmt.close();
        } catch (SQLException e) {
            log(e);
        }
    }
}

...which suggests that the boolean variable is the problem, not the additional try block.

  • I'll have to try this, and see if it resolves the issue - if not, it might just be an oversight in the HP Fortify Rulespack that we're using. – Zibbobz Jun 4 '15 at 15:48
  • Of course, it would be way better to just use a try(resource){ block (try-with-resource). Assuming you can run Java 7. – Benjamin Gruenbaum Jun 4 '15 at 17:45
0

Eliminate the condition, since the compiler things the code could not run at all.

Then catch Exception instead of SQLException so it catches NullPointerException in case the connections are not initialized. I usually set the object to null after closing it, because a recommendation I read in a Sybase's EAServer paper and, maybe, it has nothing to do with Java's garbage collector but with EAServer's connection pool, and maybe I'm "cargo-culting" here.

Connection conn = null;
Boolean myConn = false;
try{
   if(conn == null){
     conn = DatabaseUtil.getConnection();
     // myConn = true; no need for this
   }
   result = DbClass.getObject(conn, otherParameters);
}catch(DatabaseException de){
  throw de;
}catch(SQLException sqle){
  throw new DatabaseException("Error Message");
}finally{
    try{
      conn.rollback();
      conn.close();
      conn=null; // good practice according to people at Sybase
    }catch(Exception e){
         //fails silently when conn is not initialized.
    }
}
  • 1
    "a recommendation I read somewhere" is not really a good thing to repeat to other people :) It's to do with the fact that the JVM won't release or finalize the object until either it's dereferenced or out of scope. So the memory associated with the connection (and all data returned), as well as any (possible) native resources would linger until you exit the method - potentially a long time if you do more work in the method! – Ordous Jun 4 '15 at 16:11
  • @Ordous I added a mention to where I read that (I already had done it in the code's comment). – Tulains Córdova Jun 4 '15 at 16:32
  • I never worked at or with Sybase or their products, yet I was given the same recommendation. Some of the answers on Javas garbage collection reaffirmed this, as well as the other suggestion I've been made - don't do anything in DB access methods except DB access. Maybe EAServer has other reasons for this as well, but it does seem like solid advice anyway. – Ordous Jun 4 '15 at 16:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.