Can you find the connection leak?

I did a short code review with a developer who couldn’t understand why some code was leaking connections. As far as the developer could tell, he was closing out all of the database objects correctly.
Consider the following code and see if you can find the connection leak:

public static String getNiDNAString(String lookup, String value)
    throws SQLException {
    try
    {
        // Create the initial context. No JNDI properties are to be supplied
        InitialContext initialContext = new InitialContext();
        PreparedStatement psSelect = null;
        ResultSet rsSelect = null;
        DataSource dataSource = (DataSource) initialContext.lookup("DS-NameHere");

    // Execute the query and get the results back from the handler
        String result="";
        try
        {
            psSelect = dataSource.getConnection().prepareStatement(
                "SELECT 42 from dual where 1=?"; // Query was changed
                psSelect.setString(1, "1");
            if((rsSelect = psSelect.executeQuery()).next())
            {
                result=(rsSelect.getString(1));
            }
        }
        catch(SQLException sqlexception)
        {
            throw sqlexception;
        }
        finally
        {
            closeObjects(rsSelect, psSelect, dataSource.getConnection());
        }
        return result;
        }
    }

public static void closeObjects(ResultSet rs, Statement s, Connection c)

{
    // Assume this implementation correctly closes all objects
}

The problem, of course lies in this call:

closeObjects(rsSelect, psSelect, dataSource.getConnection());

As you can see, the developer never gets a handle to the actual connection object and then tries to close the connection by passing dataSource.getConnection() as an argument to the closeObjects method.

The net effect is that the developer passes in a new connection into the closeObjects() method. The new connection is closed, leaving the original connection opened, thus a connection leak.

The corrected code follows:

public static String getNiDNAString(String lookup, String value)
    throws SQLException {
    try
    {
        // Create the initial context. No JNDI properties are to be supplied
        InitialContext initialContext = new InitialContext();
        PreparedStatement psSelect = null;
        ResultSet rsSelect = null;
        Connection conn = null; //Create a variable to hold a reference to the connection
        DataSource dataSource = (DataSource) initialContext.lookup("DS-NameHere");

    // Execute the query and get the results back from the handler
        String result="";
        try
        {
            //Assign the connection to "conn"
            conn = dataSource.getConnection();
            psSelect = conn.prepareStatement(
                "SELECT 42 from dual where 1=?"; // Query was changed
                psSelect.setString(1, "1");
            if((rsSelect = psSelect.executeQuery()).next())
            {
                result=(rsSelect.getString(1));
            }
        }
        catch(SQLException sqlexception)
        {
            throw sqlexception;
        }
        finally
        {
            //Pass conn to closeObjects
            closeObjects(rsSelect, psSelect, conn);
        }
        return result;
        }
    }

Leaking connections is amazingly easy to do. This example should underscore the importance of peer code reviews to uncover such issues. Tools such as PMD and FindBugs will also help. The latest version of FindBugs successfully identified the bug in the original code. Unfortunately, the developer was running an older version of FindBugs that didn’t include this rule.

One thought on “Can you find the connection leak?”

Leave a Reply

Your email address will not be published. Required fields are marked *