Method may fail to clean up stream or resource on checked exception -- FindBugs

Question

I am using Spring JDBCTemplate to access data in database and its working fine. But FindBugs is pointing me a Minor issue in my code snippet.

CODE:

public String createUser(final User user) {
        try { 
            final String insertQuery = "insert into user (id, username, firstname, lastname) values (?, ?, ?, ?)";
            KeyHolder keyHolder = new GeneratedKeyHolder();
            jdbcTemplate.update(new PreparedStatementCreator() {
                public PreparedStatement createPreparedStatement(Connection connection) throws SQLException {
                    PreparedStatement ps = connection.prepareStatement(insertQuery, new String[] { "id" });
                    ps.setInt(1, user.getUserId());
                    ps.setString(2, user.getUserName());
                    ps.setString(3, user.getFirstName());
                    ps.setInt(4, user.getLastName());
                    return ps;
                }
            }, keyHolder);
            int userId = keyHolder.getKey().intValue();
            return "user created successfully with user id: " + userId;
        } catch (DataAccessException e) {
            log.error(e, e);
        }
    }

FindBugs Issue:

Method may fail to clean up stream or resource on checked exception in this line PreparedStatement ps = connection.prepareStatement(insertQuery, new String[] { "id" });

Could some one please brief me what is this exactly? And how can we solve this?

Help would be appreciated :)

Solution 1

FindBugs is right about the potential leak on exception case because setInt and setString are declared to throw 'SQLException'. If any of those lines were to throw a SQLException then the PreparedStatement is leaked because there is no scope block that has access to close it.

To better understand this issue let's break down the code illusion by getting rid of the spring types and inline the method in way that is an approximation of how the callstack scoping would work when calling a method that returns a resource.

public void leakyMethod(Connection con) throws SQLException {
    PreparedStatement notAssignedOnThrow = null; //Simulate calling method storing the returned value.
    try { //Start of what would be createPreparedStatement method
        PreparedStatement inMethod = con.prepareStatement("select * from foo where key = ?");
        //If we made it here a resource was allocated.
        inMethod.setString(1, "foo"); //<--- This can throw which will skip next line.
        notAssignedOnThrow = inMethod; //return from createPreparedStatement method call.
    } finally {
        if (notAssignedOnThrow != null) { //No way to close because it never 
            notAssignedOnThrow.close();   //made it out of the try block statement.
        }
    }
}

Going back to the original issue, the same is true if user is null resulting in a NullPointerException due to no user given or some other custom exception say UserNotLoggedInException is thrown from deep inside of getUserId().

Here is an example of an ugly fix for this issue:

    public PreparedStatement createPreparedStatement(Connection connection) throws SQLException {
        boolean fail = true;
        PreparedStatement ps = connection.prepareStatement(insertQuery, new String[] { "id" });
        try {
            ps.setInt(1, user.getUserId());
            ps.setString(2, user.getUserName());
            ps.setString(3, user.getFirstName());
            ps.setInt(4, user.getLastName());
            fail = false;
        } finally {
            if (fail) {
                try {
                   ps.close();
                } catch(SQLException warn) {
                }
            }
        }
        return ps;
    }

So in this example it only closes the statement if things have gone wrong. Otherwise return an open statement for the caller to clean up. A finally block is used over a catch block as a buggy driver implementation can throw more than just SQLException objects. Catch block and rethrow isn't used because inspecting type of a throwable can fail in super rare cases.

In JDK 7 and JDK 8 you can write the patch like this:

public PreparedStatement createPreparedStatement(Connection connection) throws SQLException {
        PreparedStatement ps = connection.prepareStatement(insertQuery, new String[] { "id" });
        try {
            ps.setInt(1, user.getUserId());
            ps.setString(2, user.getUserName());
            ps.setString(3, user.getFirstName());
            ps.setInt(4, user.getLastName());
        } catch (Throwable t) {    
            try {
               ps.close();
            } catch (SQLException warn) {
                if (t != warn) {
                    t.addSuppressed(warn);
                }
            }
            throw t;
        }
        return ps;
    }

In JDK 9 and later you can write the patch like this:

public PreparedStatement createPreparedStatement(Connection connection) throws SQLException {
        PreparedStatement ps = connection.prepareStatement(insertQuery, new String[] { "id" });
        try {
            ps.setInt(1, user.getUserId());
            ps.setString(2, user.getUserName());
            ps.setString(3, user.getFirstName());
            ps.setInt(4, user.getLastName());
        } catch (Throwable t) {    
            try (ps) { // closes statement on error
               throw t;
            }
        }
        return ps;
    }

With regard to Spring, say your user.getUserId() method could throw IllegalStateException or the given user is null. Contractually, Spring does not specify what happens if java.lang.RuntimeException or java.lang.Error is thrown from a PreparedStatementCreator. Per the docs:

Implementations do not need to concern themselves with SQLExceptions that may be thrown from operations they attempt. The JdbcTemplate class will catch and handle SQLExceptions appropriately.

That verbiage implies that Spring is relying on connection.close() doing the work.

Let's make proof of concept to verify what the Spring documentation promises.

public class LeakByStackPop {
    public static void main(String[] args) throws Exception {
        Connection con = new Connection();
        try {
            PreparedStatement ps = createPreparedStatement(con);
            try {

            } finally {
                ps.close();
            }
        } finally {
            con.close();
        }
    }

    static PreparedStatement createPreparedStatement(Connection connection) throws Exception {
        PreparedStatement ps = connection.prepareStatement();
        ps.setXXX(1, ""); //<---- Leak.
        return ps;
    }

    private static class Connection {

        private final PreparedStatement hidden = new PreparedStatement();

        Connection() {
        }

        public PreparedStatement prepareStatement() {
            return hidden;
        }

        public void close() throws Exception {
            hidden.closeFromConnection();
        }
    }

    private static class PreparedStatement {


        public void setXXX(int i, String value) throws Exception {
            throw new Exception();
        }

        public void close() {
            System.out.println("Closed the statement.");
        }

        public void closeFromConnection() {
            System.out.println("Connection closed the statement.");
        }
    }
}

The resulting output is:

Connection closed the statement.
Exception in thread "main" java.lang.Exception
    at LeakByStackPop$PreparedStatement.setXXX(LeakByStackPop.java:52)
    at LeakByStackPop.createPreparedStatement(LeakByStackPop.java:28)
    at LeakByStackPop.main(LeakByStackPop.java:15)

As you can see the connection is the only reference to the prepared statement.

Let's update the example to fix the memory leak by patching our fake 'PreparedStatementCreator' method.

public class LeakByStackPop {
    public static void main(String[] args) throws Exception {
        Connection con = new Connection();
        try {
            PreparedStatement ps = createPreparedStatement(con);
            try {

            } finally {
                ps.close();
            }
        } finally {
            con.close();
        }
    }

    static PreparedStatement createPreparedStatement(Connection connection) throws Exception {
        PreparedStatement ps = connection.prepareStatement();
        try {
            //If user.getUserId() could throw IllegalStateException
            //when the user is not logged in then the same leak can occur.
            ps.setXXX(1, "");
        } catch (Throwable t) {
            try {
                ps.close();
            } catch (Exception suppressed) {
                if (suppressed != t) {
                   t.addSuppressed(suppressed);
                }
            }
            throw t;
        }
        return ps;
    }

    private static class Connection {

        private final PreparedStatement hidden = new PreparedStatement();

        Connection() {
        }

        public PreparedStatement prepareStatement() {
            return hidden;
        }

        public void close() throws Exception {
            hidden.closeFromConnection();
        }
    }

    private static class PreparedStatement {


        public void setXXX(int i, String value) throws Exception {
            throw new Exception();
        }

        public void close() {
            System.out.println("Closed the statement.");
        }

        public void closeFromConnection() {
            System.out.println("Connection closed the statement.");
        }
    }
}

The resulting output is:

Closed the statement.
Exception in thread "main" java.lang.Exception
Connection closed the statement.
    at LeakByStackPop$PreparedStatement.setXXX(LeakByStackPop.java:63)
    at LeakByStackPop.createPreparedStatement(LeakByStackPop.java:29)
    at LeakByStackPop.main(LeakByStackPop.java:15)

As you can see each allocation was balanced with a close to release the resource.

Solution 2

Yes, this looks like a false positive which the FindBugs team would like to hear about so they can tune this warning. They've added specific exceptions for third-party methods in other tests, and I expect this would be handled the same way. You can file a bug report or email the team.

For now, however, you can ignore this warning in this one case using the SuppressFBWarnings annotation:

@SuppressFBWarnings("OBL_UNSATISFIED_OBLIGATION_EXCEPTION_EDGE")
public PreparedStatement createPreparedStatement...

To improve readability and allow reusing warnings I found it helpful to define constants in a helper class:

public final class FindBugs {
    final String UNCLOSED_RESOURCE = "OBL_UNSATISFIED_OBLIGATION_EXCEPTION_EDGE";

    private FindBugs() {
        // static only
    }
}

...

@SuppressFBWarnings(FindBugs.UNCLOSED_RESOURCE)

Unfortunately, I was not able to define an annotation that ignored a specific warning.

Comments

[Kiran]

Yes Bro. Even I am thinking same. As new to Spring and FindBugs curious to know how to solve it. If we can :)

[fyelci]

It worked thanks. I user below dependency for this. <dependency> <groupId>com.google.code.findbugs</groupId> <artifactId>annotations</artifactId> <version>2.0.1</version> </dependency>

[David Harkness]

Ah yes, I suppose they could throw an exception "if a database access error occurs." There's no need for the fail boolean since you'll always enter the finally block in that case.

[jmehrens]

@DavidHarkness Wrong index would trigger a leak too and another FB warning. The fail boolen is needed since we want to return an open statement in the normal case.

[David Harkness]

You're returning a closed statement which is not good. Remove fail, return the statement from the try block, and throw an exception from the catch block to simplify the logic. Or does the template throw one if the statement isn't open.

[jmehrens]

The example either returns an open statement or closes the statement and allows the pending exception thrown from setXXX to escape the method. You could simplify this logic but those versions have edge case that still allow the leak to occur.

[David Harkness]

Oh sorry, missed that it was a finally block.

[Nathan Hughes]

There's no reason to close the preparedStatement here, Spring commits to closing it.

[jmehrens]

@NathanHughes The reason to close the statement is because it can be allocated and not escape. In which case, Spring can't commit to directly closing the statement. It is relying on the JDBC implementation that is working correctly. Which is why I agree with FindBugs warning, especially after running scans on my JDBC driver. In the general case, I don't disagree with your comment.

[Nathan Hughes]

Spring is not relying on connection.close (which would be uncertain), see the jdbctemplate code posted in my answer.

[jmehrens]

@NathanHughes In your code example, the assignment is impossible for ps = psc.createPreparedStatement(conToUse); in the case of setXXX throwing an exception. Since the assignment never happens there is no way to call ps.close from that code. The code implementation of createPreparedStatement has to do it before the reference is popped of the stack. JDBC is doing clean up in this specific but unlikely case.

[Nathan Hughes]

oh ok. While it seems like if your statement throws an exception when setting parameters you have bigger problems than the statement not closing, so in practice I wouldn't add the finally block here, I can see the point.