Is this JDBC code prone to leaking?

Question

I have this JDBC code implemented, and over the weekend we had an issue where connections were potentially leaked, and database functionality ceased working. Although I am not certain that is the problem, I wanted to pass it by the Stack Overflow community to see if any of this code is indeed prone to leaking. I believe that I have it all narrowed down with Try with resources, but perhaps I have missed something. All help appreciated.

 public ArrayList<LinkedHashMap> runReportQuery(final DataSource dataSource, final String asms, final String country)  {    

      final ArrayList<LinkedHashMap> reportList = new ArrayList<>();

      this.executeQuery(dataSource, "select * from table where id =5", "customerReportType", true,  reportList);


            return reportList;
        }
        private void executeQuery(final DataSource queryDataSource, final String sql, final String reportType,
                                  final Boolean isMarketSpecific,  final ArrayList<LinkedHashMap> reportList ){

            try(Connection conn = queryDataSource.getConnection();
                PreparedStatement ps = this.createPreparedStatement(conn, isMarketSpecific, sql);
                ResultSet rs = ps.executeQuery();
            ) {
                while(rs.next()){
                    final LinkedHashMap<String, String> reportMap = new LinkedHashMap<>();
                    //Creating report string that adds columns and respective values to report - This way we do not need to deal with DTO object creation logic
                    String reportString=  "";
                    //Iterate through each column, add column and respective data.
                    for (int i = 1; i <= rs.getMetaData().getColumnCount(); i++)
                        reportString = reportString + rs.getMetaData().getColumnLabel(i) + ": " + rs.getString(i) +"!$";
                    reportMap.put("reportItem", reportType + "!$"+  reportString);
                    reportList.add(reportMap);
                }
            }
            catch (final SQLException e){
                LOG.info("SqlException Occured", e);
                e.printStackTrace();
                throw new RuntimeException(e);
            }
        }

        private PreparedStatement createPreparedStatement(final Connection con, final boolean isMarketSpecific, final String sql) throws SQLException {
            final PreparedStatement statement = con.prepareStatement(sql);
            if(isMarketSpecific)
            {
                statement.setString(1, this.asms);
                statement.setString(2, this.country);
            }
            return statement;
        }

You close the connection, statement and resultset. Looks good to me... — f1sh, Sep 05 '17 at 13:49
Are we talking about a MySQL/MariaDB instance? If yes, open the DB-client of your choice and execute `show full processlist` (do it right now ;-). That should show you all open connections (at least if you use a user with that privilege), where they are coming from and what they are currently doing. That should act as starting point of research, where the connections are coming from — Lothar, Sep 05 '17 at 13:52
It's better to avoid concat strings in a loop `for (int i = 1; i <= rs.getMetaData().getColumnCount(); i++)`. Use StringBuilder instead. — StanislavL, Sep 05 '17 at 13:53
You also probably want to change the String concatenation for StringBuffer for performance [Relevant question](https://stackoverflow.com/questions/65668/why-to-use-stringbuffer-in-java-instead-of-the-string-concatenation-operator) — jrtapsell, Sep 05 '17 at 13:57
I'm voting to close this question as off-topic because it is asking for a code review, please post your question on https://codereview.stackexchange.com/ (but check their rules and expectations first). — Mark Rotteveel, Sep 05 '17 at 14:53

score 3 · Accepted Answer · answered Sep 05 '17 at 13:55

I think there is a small possibility that some exception in this code can generate a connection leak:

if(isMarketSpecific)
{
       statement.setString(1, this.asms);
       statement.setString(2, this.country);
}

As the PreparedStatement has already been obtained, but not assigned to ps, it will not be closed by the try-with-resources. Some drivers close every dependent resource on connection.close(), but others require you to explicitly close() each resource. These drivers might leak.

score 1 · Answer 2 · answered Sep 05 '17 at 14:29

I agree with @gpeche's diagnosis. You could fix it like this:

private PreparedStatement createPreparedStatement(final Connection con, 
           final boolean isMarketSpecific, final String sql) 
           throws SQLException {
    final PreparedStatement statement = con.prepareStatement(sql);
    try {
        if (isMarketSpecific) {
            statement.setString(1, this.asms);
            statement.setString(2, this.country);
        }
        return statement;
    } catch (SQLException | RuntimeException | Error e) {
        try {
            statement.close();
        catch (SQLException e) {
            // squash
        }
        throw e;
    }
}

But as you can see, this is pretty verbose. So I think a better solution is to restructure executeQuery to use nested try-with-resource statements; e.g.

try (Connection conn = queryDataSource.getConnection();
    PreparedStatement ps = con.prepareStatement(sql)) {
    if (isMarketSpecific) { // This could be a separate method ...
        statement.setString(1, this.asms);
        statement.setString(2, this.country);
    }
    try (ResultSet rs = ps.executeQuery()) {
        while(rs.next()){
            // etc
        }
    }
}
catch (final SQLException e){
    // etc
}

Is this JDBC code prone to leaking?

2 Answers2