Method may not clear thread or resource from checked exception - FindBugs

Question

I use Spring JDBCTemplate to access the data in the database and its health. But FindBugs points me to a minor problem 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:

The method may not clear the thread or resource from the checked exception in this line PreparedStatement ps = connection.prepareStatement(insertQuery, new String[] { "id" });

Can someone please tell me what it is? And how can we solve this?

Help will be appreciated :)

Answer 1

FindBugs is right about a potential leak in the event of an exception, because setInt and setString are declared as throwing "SQLException". If any of these lines throws a SQLException, then the PreparedStatement is leaked because there is no scope block that has access to close it.

To better understand this problem, let's analyze the illusion of code, get rid of the types of springs, and add a method to the method that approximately corresponds to how the scope of the call stack works when the method returns the 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. } } } 

Returning to the original problem, the same is true if user is null, which leads to a NullPointerException due to the absence of the specified user or due to some other user exception, for example, UserNotLoggedInException thrown from the depth of getUserId() .

Here is an example of an ugly solution to this problem:

  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 closes the statement only if something went wrong. Otherwise, return an open statement for the caller to clear. The finally block is used on the catch block, because a driver implementation with errors can throw more than just SQLException objects. Blocking and re-throwing are not used, since a check of the type of throw may fail in very rare cases.

In JDK 7 and JDK 8, you can write a patch as follows:

 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 a patch as follows:

 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; } 

As for Spring, let's say that your user.getUserId() method can throw an IllegalStateException, or the given user is null . In the contract, Spring does not indicate what happens if java.lang.RuntimeException or java.lang.Error is thrown from PreparedStatementCreator. According to the documentation:

Implementations should not care about SQLExceptions that can be thrown out of the operations they try. The JdbcTemplate class will intercept and handle SQLExceptions accordingly.

This verbal expression implies that Spring relies on connection.close () to do the work .

Let's do a concept check to see what 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 result 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 a memory leak by fixing 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 result 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 distribution was balanced with the closure of the resource.

Answer 2

Yes, it looks like a false positive that the FindBugs team would like to hear so that they can set up this warning. They have added special exceptions for third-party methods to other tests, and I expect this to be handled the same way. You can record a bug report or send an e-mail command .

However, you can now ignore this warning in this case using the SuppressFBWarnings annotation:

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

To improve readability and allow reuse of warnings, it was useful for me 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 could not determine the annotation that ignored the specific warning.

Answer 3

PreparedStatement is a Closeable resource. However, it seems that the JDBC template is responsible for closing it, so FindBugs probably came across a false positive.

Answer 4

Spring will close your PreparedStatement, this part is not a problem. Spring provided you with a way to pass the callback that the PreparedStatement creates; Spring knows how to close it after it completes. In particular, the api doc for PreparedStatementCreator promises jdbcTemplate will close it:

JdbcTemplate will close the created statement.

Spring will also handle SQLExceptions, the same javadoc says:

there is no need to catch the SQLExceptions that can be selected when implementing this method. The JdbcTemplate class will handle them.

Even through the JdbcTemplate class, it handles SQLExceptions, if PreparedStatement throws a SQLException when setting the parameter, the prepared statement will not be closed with jdbcTemplate code. But in this case you have worse problems than the uncovered PreparedStatement, you have an inappropriate parameter.

If you study the source code, the update method calls this execution method:

 @Override public <T> T [More ...] execute(PreparedStatementCreator psc, PreparedStatementCallback<T> action) throws DataAccessException { Assert.notNull(psc, "PreparedStatementCreator must not be null"); Assert.notNull(action, "Callback object must not be null"); if (logger.isDebugEnabled()) { String sql = getSql(psc); logger.debug("Executing prepared SQL statement" + (sql != null ? " [" + sql + "]" : "")); } Connection con = DataSourceUtils.getConnection(getDataSource()); PreparedStatement ps = null; try { Connection conToUse = con; if (this.nativeJdbcExtractor != null && this.nativeJdbcExtractor.isNativeConnectionNecessaryForNativePreparedStatements()) { conToUse = this.nativeJdbcExtractor.getNativeConnection(con); } ps = psc.createPreparedStatement(conToUse); applyStatementSettings(ps); PreparedStatement psToUse = ps; if (this.nativeJdbcExtractor != null) { psToUse = this.nativeJdbcExtractor.getNativePreparedStatement(ps); } T result = action.doInPreparedStatement(psToUse); handleWarnings(ps); return result; } catch (SQLException ex) { // Release Connection early, to avoid potential connection pool deadlock // in the case when the exception translator hasn't been initialized yet. if (psc instanceof ParameterDisposer) { ((ParameterDisposer) psc).cleanupParameters(); } String sql = getSql(psc); psc = null; JdbcUtils.closeStatement(ps); ps = null; DataSourceUtils.releaseConnection(con, getDataSource()); con = null; throw getExceptionTranslator().translate("PreparedStatementCallback", sql, ex); } finally { if (psc instanceof ParameterDisposer) { ((ParameterDisposer) psc).cleanupParameters(); } JdbcUtils.closeStatement(ps); DataSourceUtils.releaseConnection(con, getDataSource()); } } 

It would be unrealistic to expect that the tools for analyzing static code would be smart enough for everything to be in order, only so much they could do.

For me, the real problem with this code is where you catch and log the exception. By not allowing an exception to be thrown, it prevents the transaction from rolling back when Spring fails. Either get rid of try-catch and let the DataAccessException be thrown, or (if you have to register it here) reconstruct it after logging.