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;
+ }
+};
