Spot the concurrency issue

Question

There is a database connection leak in one of our legacy application, and I tracked it down to this little gem. From debugging, I can see the same logical connection being returned for multiple threads (not good!). But I'm struggling to understand why that is happening.

We are using the ojdbc6 driver, setup on a WebLogic Data Source with connection pooling.

Code that produces problem

public class MyDummyDaoUtil {

    //note: this is a public field in a singleton (not a static field though...)
    public Connection conn;

    private MyDummyDaoUtil() {
    }

    public static MyDummyDaoUtil getInstance() {
        if (instance == null) {
            instance = new MyDummyDaoUtil();
        }

        return instance;
    }

    private DataSource getDataSource(final String dsName)
        throws NamingException {
        return ServiceLocator.getInstance().getDataSource(dsName);
    }

    public static Connection getConnection(final String source)
        throws NamingException {
        return MyDummyDaoUtil.getInstance().getDBConnection(source);
    }

    private Connection getDBConnection(final String source)
        throws NamingException {

        //the same logical connection is produced by the data source or something else happening?
        conn = getDataSource(source).getConnection(); 

        conn.setAutoCommit(false);

        return conn;
    }
}

Updated fix

public class MyDummyDaoUtil {

    private MyDummyDaoUtil() {
    }

    public static MyDummyDaoUtil getInstance() {
        if (instance == null) {
            instance = new MyDummyDaoUtil();
        }

        return instance;
    }

    private DataSource getDataSource(final String dsName)
        throws NamingException {
        return ServiceLocator.getInstance().getDataSource(dsName);
    }

    public static Connection getConnection(final String source)
        throws NamingException {
        return MyDummyDaoUtil.getInstance().getDBConnection(source);
    }

    private Connection getDBConnection(final String source)
        throws NamingException {

        Connection conn = getDataSource(source).getConnection();
        conn.setAutoCommit(false);

        return conn;
    }
}

Summary of fix

Incorrect lazy initialization of the instance
Connection should be a local variable in the method, not the singleton class

You say the MyDummyDaoUtil is a Singleton. If this is shared across multiple threads then there will be an opportunity for two connections to be requested mutating the reference and the same reference (the last one) returned from both calls to getDBConnection. The connection not returned can never be closed. Was this code not ending up depleting the available connections? — Yoztastic, Oct 08 '14 at 17:57
@Yoztastic The connections probably got shot down by their finalizer when they were garbage collected, hiding that problem. — Durandal, Oct 08 '14 at 19:28
@Yoztastic - Yes, that was the initial symptom. Our connections were depleted, and we had to turn on the inactive timeout to help reclaim those leaked connections while we track down the issue. From what I can tell, the code was implemented this way from day 1 (which is scary...) It's unclear how this problem didn't show on our old system. My guess is the connections were reclaimed somehow, and we didn't realize this problem existed. — user1766760, Oct 10 '14 at 16:19

score 0 · Answer 1 · edited May 23 '17 at 12:27

You are using singleton pattern and you have to handle the object instantiation in synchronized way when used in multi-threaded enviroment.

Try any one option:

use getInstance() synchronized method

public static synchronized MyDummyDaoUtil getInstance() {
    if (instance == null) {
        instance = new MyDummyDaoUtil();
    } 
    return instance;
}

instantiate it eagerly

private static MyDummyDaoUtil instance = new MyDummyDaoUtil();
public static MyDummyDaoUtil getInstance() {
    return instance;
}

use double check locking mechanism

public static MyDummyDaoUtil getInstance() {

    if (instance == null) {
        synchronized(MyDummyDaoUtil.class){
             if (instance == null) {
                 instance = new MyDummyDaoUtil();
             }
         }
    }
    return instance;
}

Note: Don't forget to close the Connection one its done.

score 0 · Answer 2 · answered Oct 08 '14 at 20:23

Assuming that what you're asking is "Why did changing this code fix the problem of getDBConnection returning the same object on multiple threads"...

You've got mutable state (MyDummyDaoUtil.conn) that you're using. Consider the following scenario - two threads (A and B) are both calling your original function at the same time:

private Connection getDBConnection(final String source)
    throws NamingException {
    conn = getDataSource(source).getConnection(); //line 1
    conn.setAutoCommit(false);                    //line 2
    return conn;                                  //line 3
}

There are a lot of possible sequences here, but here's a sample problematic one:

Thread A executes line 1. Your data source returns a new connection (we'll call it connectionA), and MyDummyDaoUtil.conn is set to connectionA.
Thread B executes line 1. Your data source returns a new connection (we'll call this connectionB), and MyDummyDaoUtil.conn is set to connectionB.
Thread A executes line 2. conn is now connectionB, so this results in setting auto commit to false on connectionB.
Thread A executes line 3, returning connectionB (which is the one created from the other thread).
Thread B executes line 2, setting auto commit to false on connectionB (which is a no-op, because Thread A already did that on accident)
Thread B executes line 3, returning connectionB.

The problem is that MyDummyDaoUtil.conn, since it's a singleton member variable, refers to the same variable in both threads. In your second example, the fact that there's a local variable means there's a separate variable for each invocation of the function, so you don't get the cross-contamination between calls.

Spot the concurrency issue

2 Answers2