Database connection leak

Question

Recently, I had to measure the time taken by some of the SQL requests of our software. To do that, I decided to take the naive approach and surround queries with calls to System.nanoTime(). While doing that I found a class (SqlSuggest) containing 4 very similar queries in 4 very similar methods. I thought it would be a good idea to refactor and regroup the common parts.

This created a connection leak, the connections weren't closed anymore. I rolled back the refactor but I'd still like to understand what I did wrong.

The first version of the SqlSuggest class has 4 methods getSuggestListByItems, getDisplayValueByItems, getSuggestListByListID and getDisplayValueByListeID. Each of these methods opens and closes a Connection, Statement and ResultSet (in a finally block) through another class: DBAccess.

The second version of that class has the same 4 methods except instead of opening and closing the Connection, they call 1 of 2 methods depending on whether they need one result or a list.

Those 2 methods (executeQueryGetString and executeQueryGetListOfStringArray) each declare the Connection, Statement and ResultSet, then call a method: executeQuery where the Connection is open, the Statement is created and the ResultSet returned.

The second-tier method then extracts the data from the ResultSet and closes everything.

I'm guessing I was wrong in thinking that declaring the Connection, Statement and ResultSet in a method would then allow me to close them from the same method.

Here is the old class (simplified):

public class SqlSuggest {

    private final static Logger LOGGER = LogManager.getLogger();

    public List<String[]> getSuggestListByItems(String codeSite) {
        List<String[]> suggestList = new ArrayList<>();
        String query = "SELECT iDcode, designation FROM Mytable WHERE codeSite = " + codeSite; // Simplified query building
        DBAccess accesBD = new DBAccess();
        Connection conn = null;
        Statement stmt = null;
        ResultSet rs = null;
        try {
            conn = accesBD.getConnection();
            stmt = conn.createStatement();
            rs = stmt.executeQuery(query);
            while (rs.next()) {
                suggestList.add(new String[]{rs.getString(1), rs.getString(2)});
            }
        } catch (NamingException | SQLException ex) {
            LOGGER.error("", ex);
        } finally {
            accesBD.closeResultSet(rs);
            accesBD.closeStatement(stmt);
            accesBD.closeConnection(conn);
        }
        return suggestList;
    }

    public String getDisplayValueByItems(String table, String codeSite) {
        String displayValue = null;
        String query = "SELECT iDcode, designation FROM " + table + " WHERE codeSite = " + codeSite; // Different query building
        DBAccess accesBD = new DBAccess();
        Connection conn = null;
        Statement stmt = null;
        ResultSet rs = null;

        try {
            conn = accesBD.getConnection();
            stmt = conn.createStatement();
            rs = stmt.executeQuery(query);
            if (rs.next()) {
                displayValue = rs.getString(1);
            }
        } catch (NamingException | SQLException ex) {
            LOGGER.error("", ex);

        } finally {
            accesBD.closeResultSet(rs);
            accesBD.closeStatement(stmt);
            accesBD.closeConnection(conn);
        }

        return displayValue;
    }

    public List<String[]> getSuggestListByListID(String listeID, String table, String codeSite) {
        List<String[]> suggestList = new ArrayList<>();
        String query = "SELECT iDcode, designation FROM " + table + " WHERE codeSite = " + codeSite; // Different query building involving listID
        DBAccess accesBD = new DBAccess();
        Connection conn = null;
        Statement stmt = null;
        ResultSet rs = null;
        try {
            conn = accesBD.getConnection();

            stmt = conn.createStatement();
            rs = stmt.executeQuery(query);
            while (rs.next()) {
                suggestList.add(new String[]{rs.getString(1), rs.getString(2)});
            }
        } catch (NamingException | SQLException ex) {
            LOGGER.error("", ex);
        } finally {
            accesBD.closeResultSet(rs);
            accesBD.closeStatement(stmt);
            accesBD.closeConnection(conn);
        }
        return suggestList;
    }

    public String getDisplayValueByListeID(String listeID, String table, String codeSite) {
        String displayValue = null;        
        String query = "SELECT iDcode, designation FROM " + table + " WHERE codeSite = " + codeSite; // Different query building involving listID
        DBAccess accesBD = new DBAccess();
        Connection conn = null;
        Statement stmt = null;
        ResultSet rs = null;
        try {
            conn = accesBD.getConnection();
            stmt = conn.createStatement();
            rs = stmt.executeQuery(query);
            if (rs.next()) {
                displayValue = rs.getString(2);
            }
        } catch (NamingException | SQLException ex) {
            LOGGER.error("", ex);
        } finally {
            accesBD.closeResultSet(rs);
            accesBD.closeStatement(stmt);
            accesBD.closeConnection(conn);
        }


        return displayValue;
    }
}

Here is the new class (the one with the leak):

public class SqlSuggest {

    private static final Logger LOGGER = LogManager.getLogger();

    public List<String[]> getSuggestListByItems(String codeSite) {
        List<String[]> suggestList;
        String query = "SELECT iDcode, designation FROM Mytable WHERE codeSite = " + codeSite; // Simplified query building
        suggestList = executeQueryGetListOfStringArray(query);
        return suggestList;
    }

    public String getDisplayValueByItems(String table, String codeSite) {
        String displayValue = null;
        String query = "SELECT iDcode, designation FROM " + table + " WHERE codeSite = " + codeSite; // Different query building
        int columnToFetch = 1;
        displayValue = executeQueryGetString(query, columnToFetch);
        return displayValue;
    }

    public List<String[]> getSuggestListByListID(String listeID, String table, String codeSite) {
        List<String[]> suggestList = new ArrayList<>();
        String query = "SELECT iDcode, designation FROM " + table + " WHERE codeSite = " + codeSite; // Different query building involving listID
        suggestList = executeQueryGetListOfStringArray(query);
        return suggestList;
    }

    public String getDisplayValueByListeID(String listeID, String table, String codeSite) {
        String displayValue = null;
        String query = "SELECT iDcode, designation FROM " + table + " WHERE codeSite = " + codeSite; // Different query building involving listID
        int columnToFetch = 2;
        displayValue = executeQueryGetString(query, columnToFetch);
        return displayValue;
    }

    private String executeQueryGetString(String query, int columnToFetch){
        String result = null;
        Statement stmt = null;
        Connection conn = null;
        ResultSet rs = executeQuery(query, stmt, conn);
        try{
            if (rs.next()) {
                result = rs.getString(columnToFetch);
            }
        } catch (SQLException ex) {
            LOGGER.error("", ex);
        } finally {
            DBAccess accesBD = new DBAccess();
            accesBD.closeResultSet(rs);
            accesBD.closeStatement(stmt);
            accesBD.closeConnection(conn);
        }
        return result;
    }

    private List<String[]> executeQueryGetListOfStringArray(String query){
        List<String[]> result = new ArrayList<>();
        Statement stmt = null;
        Connection conn = null;
        ResultSet rs = executeQuery(query, stmt, conn);
        try{
            while (rs.next()) {
                result.add(new String[]{rs.getString(1), rs.getString(2)});
            }
        } catch (SQLException ex) {
            LOGGER.error("", ex);
        } finally {
            DBAccess accesBD = new DBAccess();
            accesBD.closeResultSet(rs);
            accesBD.closeStatement(stmt);
            accesBD.closeConnection(conn);
        }
        return result;
    }

    private ResultSet executeQuery(String query, Statement stmt, Connection conn){
        DBAccess accesBD = new DBAccess();
        ResultSet rs = null;
        try {
            conn = accesBD.getConnection();
            stmt = conn.createStatement();
            rs = stmt.executeQuery(query);   
        } catch (NamingException | SQLException ex) {
            LOGGER.error("", ex);
        }
        return rs;
    }
}

And here is the class that actually opens and closes the connections (which wasn't changed in any way):

    public class DBAccess {

    private final static Logger LOGGER = LogManager.getLogger();

    public void closeResultSet(ResultSet rs) {
        if (rs != null) {
            try {
                rs.close();
            } catch (Exception e) {
                LOGGER.error("", e);
            }
        }
    }

    public void closeStatement(Statement stmt) {
        if (stmt != null) {
            try {
                stmt.close();
            } catch (Exception e) {
                LOGGER.error("", e);
            }
        }
    }

    public void closeConnection(Connection conn) {
        if (conn != null) {
            try {
                conn.close();
            } catch (Exception e) {
                LOGGER.error("", e);
            }
        }
    }

    public Connection getConnection() throws SQLException, NamingException {
        Connection cnx = null;
        PropertiesManager manager = PropertiesDelegate.getPropertiesManager();
        String jndi = manager.getProperty("datasource.name", "QuartisWeb-PU");
        Context ctx = null;
        DataSource dataSource = null;
        try {
            ctx = new InitialContext();
            dataSource = (DataSource) ctx.lookup(jndi);
        } catch (NamingException ex) {
            try {
                dataSource = (DataSource) ctx.lookup("java:comp/env/" + jndi);
            } catch (NamingException ex1) {
                LOGGER.error("", ex1);
            }
        }
        if (dataSource != null) {
            cnx = dataSource.getConnection();
        }
        return cnx;
    }

}

Please disregard the fact that those parameters aren't correctly set in the query, I know it should be done.

Answer 1

When you make a call from executeQueryGetString() to executeQuery() the value of conn and stmt will change inside executeQuery() but will remain null inside executeQueryGetString() upon return. Therefore, at the finally of executeQueryGetString() the call to accesBD.closeConnection(conn) is actually doing accesBD.closeConnection(null)