[
https://issues.apache.org/jira/browse/DBCP-44?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12514366
]
Dain Sundstrom commented on DBCP-44:
------------------------------------
Ok one more comment before I get lunch.... My sample code above is bad. It is
a hard loop (duh). Instead of hard looping, the second check on the
idleCollection could wait for a item to become available (easy with a
concurrent queue). Which leave the possibility that someone sneaks in and
steals the new connection we were waiting for :( As, I said it will take some
experimentation to get this right, but that is the fun part.
> [dbcp] Evictor thread in GenericObjectPool has potential for deadlock
> ---------------------------------------------------------------------
>
> Key: DBCP-44
> URL: https://issues.apache.org/jira/browse/DBCP-44
> Project: Commons Dbcp
> Issue Type: Bug
> Affects Versions: 1.2.1
> Environment: Operating System: All
> Platform: All
> Reporter: Eric Layton
> Fix For: 1.3
>
> Attachments: DBCP-44.patch, deadlock.txt, deadlock_post_patch.txt,
> testConcurrency.java
>
>
> 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().
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
