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 :)

Answer 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.

Answer 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.