Mailing-List: contact commons-dev-help@jakarta.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Jakarta Commons Developers List" <commons-dev@jakarta.apache.org>
Date: Tue, 8 Mar 2005 20:47:18 +0100
Message-Id: <200503081947.j28JlIho014948@ajax.apache.org>
From: bugzilla@apache.org
To: commons-dev@jakarta.apache.org
Subject: DO NOT REPLY [Bug 33912]  New:  -
    Evictor thread in GenericObjectPool has potential for deadlock

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=33912>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=33912

           Summary: Evictor thread in GenericObjectPool has potential for
                    deadlock
           Product: Commons
           Version: unspecified
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Dbcp
        AssignedTo: commons-dev@jakarta.apache.org
        ReportedBy: elayton@novell.com
                CC: elayton@novell.com


The GenericObjectPool Evictor thread can potentially cause a deadlock between
its connection factory and java.sql.DriverManager.  The deadlock occurs when the
Evictor thread is trying to make enough connections to bring the pool's idle
connections up to what's specified in minIdle, at the same time a connection is
being requested through DriverManager.getConnection().  See the attached stack
trace dump:

Found one Java-level deadlock:
=============================
"Thread-0":
  waiting to lock monitor 0x0809a994 (object 0x698c2b48, a java.lang.Class),
  which is held by "main"
"main":
  waiting to lock monitor 0x0809aad4 (object 0x65df7030, a
org.apache.commons.dbcp.PoolableConnectionFactory),
  which is held by "Thread-0"
 
Java stack information for the threads listed above:
===================================================
"Thread-0":
        at java.sql.DriverManager.getConnection(DriverManager.java:158)
        - waiting to lock <0x698c2b48> (a java.lang.Class)
        at
org.apache.commons.dbcp.DriverManagerConnectionFactory.createConnection(DriverManagerConnectionFactory.java:48)
        at
org.apache.commons.dbcp.PoolableConnectionFactory.makeObject(PoolableConnectionFactory.java:290)
        - locked <0x65df7030> (a org.apache.commons.dbcp.PoolableConnectionFactory)
        at
org.apache.commons.pool.impl.GenericObjectPool.addObject(GenericObjectPool.java:996)
        at
org.apache.commons.pool.impl.GenericObjectPool.ensureMinIdle(GenericObjectPool.java:978)
        at
org.apache.commons.pool.impl.GenericObjectPool.access$000(GenericObjectPool.java:124)
        at
org.apache.commons.pool.impl.GenericObjectPool$Evictor.run(GenericObjectPool.java:1090)
        at java.lang.Thread.run(Thread.java:595)
"main":
        at
org.apache.commons.dbcp.PoolableConnectionFactory.makeObject(PoolableConnectionFactory.java:290)
        - waiting to lock <0x65df7030> (a
org.apache.commons.dbcp.PoolableConnectionFactory)
        at
org.apache.commons.pool.impl.GenericObjectPool.borrowObject(GenericObjectPool.java:771)
        at org.apache.commons.dbcp.PoolingDriver.connect(PoolingDriver.java:175)
        at java.sql.DriverManager.getConnection(DriverManager.java:525)
        - locked <0x698c2b48> (a java.lang.Class)
        at java.sql.DriverManager.getConnection(DriverManager.java:193)
        - locked <0x698c2b48> (a java.lang.Class)
        at Deadlock.main(Deadlock.java:83)
 
Found 1 deadlock.


The deadlock occurs when GenericObjectPool.addObject() calls synchronized method
PoolableConnectionFactory.makeObject(), meanwhile static synchronized
DriverManager.getConnection() is called.  makeObject() waits for the lock on
DriverManager to be released, whereas DriverManager waits for the lock on the
PoolableConnectionFactory instance to be released.

The Java program below, based on ManualPoolingDriverExample.java provided with
DBCP, duplicates the deadlock for me within several seconds of being run:

import java.sql.*;
import org.apache.commons.pool.*;
import org.apache.commons.pool.impl.*;
import org.apache.commons.dbcp.*;
 
/**
 * Duplicate DBCP pool deadlocking.
 *
 * Compile with:
 * /usr/java/jdk1.5.0/bin/javac
 * -classpath commons-collections.jar:commons-dbcp-1.2.1.jar:commons-pool-1.2.jar
 * Deadlock.java
 *
 * Run with:
 * /usr/java/jdk1.5.0/bin/java
 * -classpath
commons-collections.jar:commons-dbcp-1.2.1.jar:commons-pool-1.2.jar:ojdbc14.jar:xerces.jar:.
 * Deadlock
 *
 * Locks still occur when compiled and run with J2SDK 1.4.1_03.
 */
public class Deadlock {
 
        private static final int ACTIVE = 10;
 
        private static void init() {
                System.out.println("Loading drivers");
 
                try {
                    Class.forName("oracle.jdbc.driver.OracleDriver");
                        Class.forName("org.apache.commons.dbcp.PoolingDriver");
                } catch (ClassNotFoundException e) {
                    e.printStackTrace();
                }
 
                System.out.println("Setting up pool");
 
                try {
                        GenericObjectPool.Config config = new
GenericObjectPool.Config();
                        config.maxActive = ACTIVE;
                        config.minIdle = 2; // Idle limits are low to allow more
possibility of locking.
                        config.maxIdle = 4; // Locking only occurs when there
are 0 idle connections in the pool.
                        config.maxWait = 5000L;
                        config.testOnBorrow = true;
                        config.testOnReturn = false;
                        config.testWhileIdle = true;
                                // Locking still occurs whether these tests are
performed or not.
                        config.whenExhaustedAction =
GenericObjectPool.WHEN_EXHAUSTED_BLOCK;
                                // Locking still occurs regardless of the
exhausted action.
                        config.timeBetweenEvictionRunsMillis = 3000L; // The
Evictor thread is involved in the lock, so run it quite often.
                        config.minEvictableIdleTimeMillis = 6000L;
                        config.numTestsPerEvictionRun = 3;
 
                        ObjectPool op = new GenericObjectPool(null, config);
 
                        ConnectionFactory cf = new
DriverManagerConnectionFactory("jdbc:oracle:thin:@oracle8server:1521:sid",
"username", "password");
                        PoolableConnectionFactory pcf = new
PoolableConnectionFactory(cf, op, null, "SELECT 1 FROM DUAL", false, true);
                                // Locking still occurs whether there is a
validation query or not.
 
                        PoolingDriver pd =
(PoolingDriver)DriverManager.getDriver("jdbc:apache:commons:dbcp:");
                        pd.registerPool("PoolName", op);
 
                } catch (SQLException e) {
                    e.printStackTrace();
                }
 
                System.out.println("Done");
        }
 
 
        public static void main(String[] args) {
                init();
 
                Connection[] c = new Connection[ACTIVE];
 
                try {
                        printPoolStatus();
 
                        // Loop forever to create a high load.
                        while (true) {
                                // Create a number of connections.
                                for (int i = 0; i < ACTIVE; ++i) {
                                        c[i] =
DriverManager.getConnection("jdbc:apache:commons:dbcp:PoolName");
                                        printPoolStatus();
                                }
                                // Then immmediately drop them.
                                for (int i = 0; i < ACTIVE; ++i) {
                                        try {
                                                if (c[i] != null) {
                                                        c[i].close();
                                                        printPoolStatus();
                                                        c[i] = null;
                                                }
                                        } catch (SQLException e) {
                                                e.printStackTrace();
                                        }
                                }
                        }
 
                } catch (SQLException e) {
                        e.printStackTrace();
 
                } finally {
                        // Close down any open connetions.
                        for (int i = 0; i < ACTIVE; ++i) {
                                try {
                                        if (c[i] != null) c[i].close();
                                } catch (SQLException e) { }
                        }
 
                        System.out.println("Closing pool");
                        try {
                                PoolingDriver pd =
(PoolingDriver)DriverManager.getDriver("jdbc:apache:commons:dbcp:");
                                pd.closePool("PoolName");
                        } catch (SQLException e) {
                                e.printStackTrace();
                        }
                        System.out.println("Pool closed");
                }
 
        }
 
 
        /**
         * Display pool status.  Locks still occur even if this method is never
called.
         */
        private static void printPoolStatus() throws SQLException {
                PoolingDriver pd =
(PoolingDriver)DriverManager.getDriver("jdbc:apache:commons:dbcp:");
                ObjectPool op = pd.getConnectionPool("PoolName");
 
                System.out.println("Active / idle: " + op.getNumActive() + " / "
+ op.getNumIdle());
        }
 
}


The patch I initially suggest is as follows (sorry for not providing a diff):

In org.apache.commons.dbcp.PoolableConnectionFactory.java we have:

    synchronized public Object makeObject() throws Exception {
        Connection conn = _connFactory.createConnection();
        if(null != _stmtPoolFactory) {
            KeyedObjectPool stmtpool = _stmtPoolFactory.createPool();
            conn = new PoolingConnection(conn,stmtpool);
            stmtpool.setFactory((PoolingConnection)conn);
        }
        return new PoolableConnection(conn,_pool,_config);
    }

I suggest changing that to this:
    public Object makeObject() throws Exception {
        Connection conn = _connFactory.createConnection();
        synchronized (this) {
            if(null != _stmtPoolFactory) {
                KeyedObjectPool stmtpool = _stmtPoolFactory.createPool();
                conn = new PoolingConnection(conn,stmtpool);
                stmtpool.setFactory((PoolingConnection)conn);
            }
            return new PoolableConnection(conn,_pool,_config);
        }
    }


Note the move of the synchronized block from the entire method to within the
method after the connection (obtained ultimately by DriverManager) is retrieved.

I'm afraid I don't know enough about DBCP and pooling to know the full
ramification of this change and other calls to makeObject().

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org