commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pste...@apache.org
Subject svn commit: r1096550 - in /commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src: changes/changes.xml java/org/apache/commons/dbcp/BasicDataSource.java test/org/apache/commons/dbcp/TestBasicDataSource.java
Date Mon, 25 Apr 2011 18:25:51 GMT
Author: psteitz
Date: Mon Apr 25 18:25:50 2011
New Revision: 1096550

URL: http://svn.apache.org/viewvc?rev=1096550&view=rev
Log:
Modified createDataSource method in BasicDataSource to ensure that GenericObjectPool
Evictor tasks are not started and orphaned when BasicDataSource encounters errors on
initialization.  Prior to this fix, when minIdle and timeBetweenEvictionRunsMillis
are both positive, Evictors orphaned by failed initialization can continue to
generate database connection requests.
JIRA: DBCP-342
JIRA: DBCP-339
JIRA: DBCP-93
Reported by Byungchol Kim, Mike Bartlett 
Patched by Byungchol Kim, Dmitry Semibratov 


Modified:
    commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/changes/changes.xml
    commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/java/org/apache/commons/dbcp/BasicDataSource.java
    commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/test/org/apache/commons/dbcp/TestBasicDataSource.java

Modified: commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/changes/changes.xml
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/changes/changes.xml?rev=1096550&r1=1096549&r2=1096550&view=diff
==============================================================================
--- commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/changes/changes.xml (original)
+++ commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/changes/changes.xml Mon Apr 25 18:25:50
2011
@@ -39,6 +39,14 @@ The <action> type attribute can be add,u
   </properties>
   <body>
     <release version="1.4.1" date="TBD" description="TBD">
+      <action dev="psteitz" issue="DBCP-342" type="fix" due-to="Byungchol Kim">
+        Modified createDataSource method in BasicDataSource to ensure that GenericObjectPool
+        Evictor tasks are not started and orphaned when BasicDataSource encounters errors
on
+        initialization.  Prior to this fix, when minIdle and timeBetweenEvictionRunsMillis
+        are both positive, Evictors orphaned by failed initialization can continue to
+        generate database connection requests.  This issue is duplicated by DBCP-339
+        and DBCP-93.
+      </action>
       <action dev="psteitz" issue="DBCP-330" type="fix">
         Changed DelegatingDatabaseMetaData to no longer add itself to the AbandonedTrace
         of its parent connection.  This was causing excessive memory consumption and was

Modified: commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/java/org/apache/commons/dbcp/BasicDataSource.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/java/org/apache/commons/dbcp/BasicDataSource.java?rev=1096550&r1=1096549&r2=1096550&view=diff
==============================================================================
--- commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/java/org/apache/commons/dbcp/BasicDataSource.java
(original)
+++ commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/java/org/apache/commons/dbcp/BasicDataSource.java
Mon Apr 25 18:25:50 2011
@@ -287,7 +287,7 @@ public class BasicDataSource implements 
      *  
      */    
     public synchronized void setLifo(boolean lifo) {
-        this.lifo = lifo;
+        this.lifo = lifo;   
         if (connectionPool != null) {
             connectionPool.setLifo(lifo);
         }
@@ -1458,19 +1458,53 @@ public class BasicDataSource implements 
         }
 
         // Set up the poolable connection factory
-        createPoolableConnectionFactory(driverConnectionFactory, statementPoolFactory, abandonedConfig);
-
-        // Create and return the pooling data source to manage the connections
-        createDataSourceInstance();
+        boolean success = false;
+        try {
+            createPoolableConnectionFactory(driverConnectionFactory,
+                    statementPoolFactory, abandonedConfig);
+            success = true;
+        } catch (SQLException se) {
+            throw se;
+        } catch (RuntimeException rte) {
+            throw rte; 
+        } catch (Exception ex) {
+            throw new SQLException("Error creating connection factory", ex);
+        } finally {
+            if (!success) {
+                closeConnectionPool();
+            }
+        }
+        
+        // Create the pooling data source to manage connections
+        success = false;
+        try {
+            createDataSourceInstance();
+            success = true;
+        } catch (SQLException se) {
+            throw se;
+        } catch (RuntimeException rte) {
+            throw rte;
+        } catch (Exception ex) {
+            throw new SQLException("Error creating datasource", ex);
+        } finally {
+            if (!success) {
+                closeConnectionPool();
+            }  
+        }
         
+        // If initialSize > 0, preload the pool
         try {
             for (int i = 0 ; i < initialSize ; i++) {
                 connectionPool.addObject();
             }
         } catch (Exception e) {
+            closeConnectionPool();
             throw new SQLNestedException("Error preloading the connection pool", e);
         }
         
+        // If timeBetweenEvictionRunsMillis > 0, start the pool's evictor task
+        startPoolMaintenance();
+        
         return dataSource;
     }
 
@@ -1567,6 +1601,12 @@ public class BasicDataSource implements 
     /**
      * Creates a connection pool for this datasource.  This method only exists
      * so subclasses can replace the implementation class.
+     * 
+     * This implementation configures all pool properties other than 
+     * timeBetweenEvictionRunsMillis.  Setting that property is deferred to
+     * {@link #startPoolMaintenance()}, since setting timeBetweenEvictionRunsMillis
+     * to a positive value causes {@link GenericObjectpool}'s eviction timer
+     * to be started.
      */
     protected void createConnectionPool() {
         // Create an object pool to contain our active connections
@@ -1583,17 +1623,41 @@ public class BasicDataSource implements 
         gop.setMaxWait(maxWait);
         gop.setTestOnBorrow(testOnBorrow);
         gop.setTestOnReturn(testOnReturn);
-        gop.setTimeBetweenEvictionRunsMillis(timeBetweenEvictionRunsMillis);
         gop.setNumTestsPerEvictionRun(numTestsPerEvictionRun);
         gop.setMinEvictableIdleTimeMillis(minEvictableIdleTimeMillis);
         gop.setTestWhileIdle(testWhileIdle);
         gop.setLifo(lifo);
         connectionPool = gop;
     }
+    
+    /**
+     * Closes the connection pool, silently swallowing any exception that occurs.
+     */
+    private void closeConnectionPool() {
+        GenericObjectPool oldpool = connectionPool;
+        connectionPool = null;
+        try {
+            if (oldpool != null) {
+                oldpool.close();
+            }
+        } catch(Exception e) {
+            // Do not propagate
+        }
+    }
+    
+    /**
+     * Starts the connection pool maintenance task, if configured. 
+     */
+    protected void startPoolMaintenance() {
+        if (connectionPool != null && timeBetweenEvictionRunsMillis > 0) {
+            connectionPool.setTimeBetweenEvictionRunsMillis(
+                    timeBetweenEvictionRunsMillis);
+        }
+    }
 
     /**
      * Creates the actual data source instance.  This method only exists so
-     * subclasses can replace the implementation class.
+     * that subclasses can replace the implementation class.
      * 
      * @throws SQLException if unable to create a datasource instance
      */

Modified: commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/test/org/apache/commons/dbcp/TestBasicDataSource.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/test/org/apache/commons/dbcp/TestBasicDataSource.java?rev=1096550&r1=1096549&r2=1096550&view=diff
==============================================================================
--- commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/test/org/apache/commons/dbcp/TestBasicDataSource.java
(original)
+++ commons/proper/dbcp/branches/DBCP_1_4_x_BRANCH/src/test/org/apache/commons/dbcp/TestBasicDataSource.java
Mon Apr 25 18:25:50 2011
@@ -21,6 +21,9 @@ import java.io.IOException;
 import java.sql.Connection;
 import java.sql.SQLException;
 import java.util.Arrays;
+import java.util.Properties;
+
+import javax.sql.DataSource;
 
 import junit.framework.Test;
 import junit.framework.TestSuite;
@@ -513,4 +516,77 @@ public class TestBasicDataSource extends
         assertTrue(cl instanceof TesterClassLoader);
         assertTrue(((TesterClassLoader) cl).didLoad(ds.getDriverClassName()));
     }
+    
+    /**
+     * JIRA: DBCP-342, DBCP-93
+     * Verify that when errors occur during BasicDataSource initialization, GenericObjectPool
+     * Evictors are cleaned up.
+     */
+    public void testCreateDataSourceCleanupEvictor() throws Exception {
+        ds.close();
+        ds = null;
+        ds = createDataSource();
+        ds.setDriverClassName("org.apache.commons.dbcp.TesterConnRequestCountDriver");
+        ds.setUrl("jdbc:apache:commons:testerConnRequestCountDriver");
+        ds.setValidationQuery("SELECT DUMMY FROM DUAL");
+        ds.setUsername("username");
+
+        // Make password incorrect, so createDataSource will throw
+        ds.setPassword("wrong");
+        // Set timeBetweenEvictionRuns > 0, so evictor will be created
+        ds.setTimeBetweenEvictionRunsMillis(100);
+        // Set min idle > 0, so evictor will try to make connection as many as idle count
+        ds.setMinIdle(2);
+
+        // Prevent concurrent execution of threads executing test subclasses 
+        synchronized (TesterConnRequestCountDriver.class) {
+    	    TesterConnRequestCountDriver.initConnRequestCount();
+
+    	    // user request 10 times
+    	    for (int i=0; i<10; i++) {
+    	        try {
+    	            @SuppressWarnings("unused")
+    	            DataSource ds2 = ds.createDataSource();
+    	        } catch (SQLException e) {
+    	            // Ignore
+    	        }
+    	    }
+ 
+    	    // sleep 1000ms. evictor will be invoked 10 times if running.
+    	    Thread.sleep(1000);
+
+    	    // Make sure there have been no Evictor-generated requests (count should be 10,
from requests above)
+    	    assertEquals(10, TesterConnRequestCountDriver.getConnectionRequestCount());
+        }
+    	
+        // make sure cleanup is complete
+        assertNull(ds.connectionPool);
+    }
 }
+
+/**
+ * TesterDriver that keeps a static count of connection requests.
+ */
+class TesterConnRequestCountDriver extends TesterDriver {
+    protected static final String CONNECT_STRING = "jdbc:apache:commons:testerConnRequestCountDriver";
+    private static int connectionRequestCount = 0;
+
+	@Override
+    public Connection connect(String url, Properties info) throws SQLException {
+        connectionRequestCount++;
+        return super.connect(url, info);
+    }
+
+    @Override
+    public boolean acceptsURL(String url) throws SQLException {
+        return CONNECT_STRING.startsWith(url);
+    }
+
+	public static int getConnectionRequestCount() {
+	    return connectionRequestCount;
+	}
+
+    public static void initConnRequestCount() {
+	    connectionRequestCount = 0;
+    }
+};



Mime
View raw message