[ 
https://issues.apache.org/jira/browse/DBCP-44?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Dain Sundstrom updated DBCP-44:
-------------------------------

    Attachment: DBCP-44.patch

This is an architectural problem in commons-pool.  
GenericObjectPool.borrowObject() is synchronized so when it needs to make a new 
object, that method is called within a synchronized block.  Here are the stack 
traces of the deal lock in our case:

[EMAIL PROTECTED], priority=5, in group 'main', status: 'MONITOR'
         blocks [EMAIL PROTECTED]
         waiting for [EMAIL PROTECTED]
          borrowObject():781, GenericObjectPool.java
          connect():176, PoolingDriver.java
          getConnection():525, DriverManager.java
          getConnection():140, DriverManager.java
          main():107, Deadlock.java

[EMAIL PROTECTED] daemon, priority=5, in group 'main', status: 'MONITOR'
         blocks [EMAIL PROTECTED]
         waiting for [EMAIL PROTECTED]
          getConnection():138, DriverManager.java
          createConnection():68, DriverManagerConnectionFactory.java
          makeObject():329, PoolableConnectionFactory.java
          addObject():1059, GenericObjectPool.java
          ensureMinIdle():1040, GenericObjectPool.java
          access$100():128, GenericObjectPool.java
          run():1117, GenericObjectPool.java
          mainLoop():512, Timer.java
          run():462, Timer.java

This is a classic two lock dead lock.  The main thread acquires the locks on 
DriveManager then GenericObjectPool, and at the same time the timer thread 
attempts to acquire them in the opposite order GenericObjectPool and then 
DriveManger.

There are really only two ways to fix this 1) guarantee locks are always 
acquired in the same order or 2) eliminate one of the locks.  The first one is 
not really possible as both objects are generally publicly accessible, and the 
second fix will require a complex change to the GenericObjectPool code.  

Attached is a patch that adds the Deadlock class, which has a main method that 
causes the dead lock.    The patch also fixes the synchronization in 
PoolableConnectionFactory.  I also added property to TesterDriver to cause the 
thread to sleep in getConnection so the dead lock can be reproduced.

I'm going to attempt to write a new pool using the Java5 concurrent code which 
doesn't call makeObject inside of a synchronized block.



> [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]

Reply via email to