You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by ps...@apache.org on 2009/09/12 23:21:04 UTC
svn commit: r814240 - in /commons/proper/dbcp/trunk: ./
src/java/org/apache/commons/dbcp/cpdsadapter/
src/java/org/apache/commons/dbcp/datasources/
src/test/org/apache/commons/dbcp/datasources/
Author: psteitz
Date: Sat Sep 12 21:20:59 2009
New Revision: 814240
URL: http://svn.apache.org/viewvc?rev=814240&view=rev
Log:
1) Modified fix applied in r598045:
* Removed workaround to prevent ConcurrentModificationExceptions generated by PooledConnectionImpl's notifyListeners method.
* Changed PooledConnectionImpl notifyListeners to copy listeners and iterate over the copy instead of directly iterating over failfast Vector iterator.
2) Applied the same fix to CPDSConnectionFactory
Jira: DBCP-216
Added:
commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp/datasources/TestCPDSConnectionFactory.java
Modified:
commons/proper/dbcp/trunk/pom.xml
commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/cpdsadapter/PooledConnectionImpl.java
commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/datasources/CPDSConnectionFactory.java
commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/datasources/KeyedCPDSConnectionFactory.java
commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp/datasources/PooledConnectionProxy.java
Modified: commons/proper/dbcp/trunk/pom.xml
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/pom.xml?rev=814240&r1=814239&r2=814240&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/pom.xml (original)
+++ commons/proper/dbcp/trunk/pom.xml Sat Sep 12 21:20:59 2009
@@ -272,6 +272,7 @@
<include>org/apache/commons/dbcp/TestJndi.java</include>
<include>org/apache/commons/dbcp/datasources/TestFactory.java</include>
+ <include>org/apache/commons/dbcp/datasources/TestCPDSConnectionFactory.java</include>
<include>org/apache/commons/dbcp/datasources/TestKeyedCPDSConnectionFactory.java</include>
<include>org/apache/commons/dbcp/datasources/TestPerUserPoolDataSource.java</include>
<include>org/apache/commons/dbcp/datasources/TestSharedPoolDataSource.java</include>
Modified: commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/cpdsadapter/PooledConnectionImpl.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/cpdsadapter/PooledConnectionImpl.java?rev=814240&r1=814239&r2=814240&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/cpdsadapter/PooledConnectionImpl.java (original)
+++ commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/cpdsadapter/PooledConnectionImpl.java Sat Sep 12 21:20:59 2009
@@ -215,9 +215,9 @@
*/
void notifyListeners() {
ConnectionEvent event = new ConnectionEvent(this);
- Iterator i = eventListeners.iterator();
- while (i.hasNext()) {
- ((ConnectionEventListener) i.next()).connectionClosed(event);
+ Object[] listeners = eventListeners.toArray();
+ for (int i = 0; i < listeners.length; i++) {
+ ((ConnectionEventListener) listeners[i]).connectionClosed(event);
}
}
Modified: commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/datasources/CPDSConnectionFactory.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/datasources/CPDSConnectionFactory.java?rev=814240&r1=814239&r2=814240&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/datasources/CPDSConnectionFactory.java (original)
+++ commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/datasources/CPDSConnectionFactory.java Sat Sep 12 21:20:59 2009
@@ -195,9 +195,19 @@
return obj;
}
+ /**
+ * Closes the PooledConnection and stops listening for events from it.
+ */
public void destroyObject(Object obj) throws Exception {
if (obj instanceof PooledConnectionAndInfo) {
- ((PooledConnectionAndInfo) obj).getPooledConnection().close();
+ PooledConnection pc = ((PooledConnectionAndInfo)obj).getPooledConnection();
+ try {
+ pc.removeConnectionEventListener(this);
+ } catch (Exception e) {
+ //ignore
+ }
+ pcMap.remove(pc);
+ pc.close();
}
}
@@ -295,6 +305,11 @@
System.err.println("CLOSING DOWN CONNECTION AS IT COULD "
+ "NOT BE RETURNED TO THE POOL");
try {
+ pc.removeConnectionEventListener(this);
+ } catch (Exception e2) {
+ //ignore
+ }
+ try {
destroyObject(info);
} catch (Exception e2) {
System.err.println("EXCEPTION WHILE DESTROYING OBJECT "
@@ -317,8 +332,6 @@
"CLOSING DOWN CONNECTION DUE TO INTERNAL ERROR ("
+ event.getSQLException() + ")");
}
- //remove this from the listener list because we are no more
- //interested in errors since we are about to close this connection
pc.removeConnectionEventListener(this);
} catch (Exception ignore) {
// ignore
@@ -329,7 +342,7 @@
throw new IllegalStateException(NO_KEY_MESSAGE);
}
try {
- destroyObject(info);
+ _pool.invalidateObject(info);
} catch (Exception e) {
System.err.println("EXCEPTION WHILE DESTROYING OBJECT " + info);
e.printStackTrace();
Modified: commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/datasources/KeyedCPDSConnectionFactory.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/datasources/KeyedCPDSConnectionFactory.java?rev=814240&r1=814239&r2=814240&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/datasources/KeyedCPDSConnectionFactory.java (original)
+++ commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/datasources/KeyedCPDSConnectionFactory.java Sat Sep 12 21:20:59 2009
@@ -55,18 +55,10 @@
protected KeyedObjectPool _pool = null;
/**
- * Map of "muted" PooledConnections for which close events
- * are ignored. Connections are muted when they are being validated
- * or when they have been marked for removal.
+ * Map of PooledConnections for which close events are ignored.
+ * Connections are muted when they are being validated.
*/
- private Map muteMap = new HashMap();
-
- /**
- * Map of PooledConnections marked for removal. Connections are
- * marked for removal from pcMap and muteMap in destroyObject.
- * Cleanup happens in makeObject.
- */
- private Map cleanupMap = new HashMap();
+ private Map validatingMap = new HashMap();
/**
* Map of PooledConnectionAndInfo instances
@@ -77,7 +69,8 @@
* Create a new <tt>KeyedPoolableConnectionFactory</tt>.
* @param cpds the ConnectionPoolDataSource from which to obtain PooledConnection's
* @param pool the {*link ObjectPool} in which to pool those {*link Connection}s
- * @param validationQuery a query to use to {*link #validateObject validate} {*link Connection}s. Should return at least one row. May be <tt>null</tt>
+ * @param validationQuery a query to use to {*link #validateObject validate} {*link Connection}s.
+ * Should return at least one row. May be <tt>null</tt>
*/
public KeyedCPDSConnectionFactory(ConnectionPoolDataSource cpds,
KeyedObjectPool pool,
@@ -161,7 +154,9 @@
}
/**
- * @param key
+ * Creates a new {@link PooledConnectionAndInfo} from the given {@link UserPassKey}.
+ *
+ * @param key {@link UserPassKey} containing user credentials
* @throws SQLException if the connection could not be created.
* @see org.apache.commons.pool.KeyedPoolableObjectFactory#makeObject(java.lang.Object)
*/
@@ -187,25 +182,33 @@
pc.addConnectionEventListener(this);
obj = new PooledConnectionAndInfo(pc, username, password);
pcMap.put(pc, obj);
- cleanupListeners();
return obj;
}
/**
- * Closes the PooledConnection and marks it for removal from the
- * pcMap and for listener cleanup. Adds it to muteMap so connectionClosed
- * events generated by this or other actions are ignored.
+ * Closes the PooledConnection and stops listening for events from it.
*/
public void destroyObject(Object key, Object obj) throws Exception {
if (obj instanceof PooledConnectionAndInfo) {
PooledConnection pc = ((PooledConnectionAndInfo)obj).getPooledConnection();
- cleanupMap.put(pc, null); // mark for cleanup
- muteMap.put(pc, null); // ignore events until listener is removed
+ try {
+ pc.removeConnectionEventListener(this);
+ } catch (Exception e) {
+ //ignore
+ }
+ pcMap.remove(pc);
pc.close();
}
}
+ /**
+ * Validates a pooled connection.
+ *
+ * @param key ignored
+ * @param obj {@link PooledConnectionAndInfo} containing the connection to validate
+ * @return true if validation suceeds
+ */
public boolean validateObject(Object key, Object obj) {
boolean valid = false;
if (obj instanceof PooledConnectionAndInfo) {
@@ -220,7 +223,7 @@
// before another one can be requested and closing it will
// generate an event. Keep track so we know not to return
// the PooledConnection
- muteMap.put(pconn, null);
+ validatingMap.put(pconn, null);
try {
conn = pconn.getConnection();
stmt = conn.createStatement();
@@ -257,7 +260,7 @@
// ignore
}
}
- muteMap.remove(pconn);
+ validatingMap.remove(pconn);
}
} else {
valid = true;
@@ -289,7 +292,7 @@
// if this event occurred because we were validating, or if this
// connection has been marked for removal, ignore it
// otherwise return the connection to the pool.
- if (!muteMap.containsKey(pc)) {
+ if (!validatingMap.containsKey(pc)) {
PooledConnectionAndInfo info =
(PooledConnectionAndInfo) pcMap.get(pc);
if (info == null) {
@@ -300,8 +303,11 @@
} catch (Exception e) {
System.err.println("CLOSING DOWN CONNECTION AS IT COULD " +
"NOT BE RETURNED TO THE POOL");
- cleanupMap.put(pc, null); // mark for cleanup
- muteMap.put(pc, null); // ignore events until listener is removed
+ try {
+ pc.removeConnectionEventListener(this);
+ } catch (Exception e2) {
+ //ignore
+ }
try {
_pool.invalidateObject(info.getUserPassKey(), info);
} catch (Exception e3) {
@@ -319,17 +325,13 @@
*/
public void connectionErrorOccurred(ConnectionEvent event) {
PooledConnection pc = (PooledConnection)event.getSource();
- if (cleanupMap.containsKey(pc)) {
- return; // waiting for listener cleanup - ignore event
- }
try {
if (null != event.getSQLException()) {
System.err
.println("CLOSING DOWN CONNECTION DUE TO INTERNAL ERROR (" +
event.getSQLException() + ")");
}
- cleanupMap.put(pc, null); // mark for cleanup
- muteMap.put(pc, null); // ignore events until listener is removed
+ pc.removeConnectionEventListener(this);
} catch (Exception ignore) {
// ignore
}
@@ -345,31 +347,4 @@
e.printStackTrace();
}
}
-
- /**
- * Cleans up pooled connections from cleanupMap.
- * 1) remove PoolableConnectionAndInfo wrapper from pcMap
- * 2) remove from MuteMap
- */
- private void cleanupListeners() {
- try {
- Iterator iter = cleanupMap.keySet().iterator();
- while (iter.hasNext()) {
- PooledConnection pc = (PooledConnection) iter.next();
- pcMap.remove(pc);
- muteMap.remove(pc);
- try {
- pc.removeConnectionEventListener(this);
- } catch (Exception ex) {
- System.err.println("EXCEPTION REMOVING CONNECTION LISTENER ");
- ex.printStackTrace();
- }
- }
- } catch (Exception e) {
- System.err.println("EXCEPTION CLEANING UP CONNECTION LISTENERS ");
- e.printStackTrace();
- } finally {
- cleanupMap.clear();
- }
- }
}
Modified: commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp/datasources/PooledConnectionProxy.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp/datasources/PooledConnectionProxy.java?rev=814240&r1=814239&r2=814240&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp/datasources/PooledConnectionProxy.java (original)
+++ commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp/datasources/PooledConnectionProxy.java Sat Sep 12 21:20:59 2009
@@ -100,9 +100,9 @@
*/
void notifyListeners() {
ConnectionEvent event = new ConnectionEvent(this);
- Iterator i = eventListeners.iterator();
- while (i.hasNext()) {
- ((ConnectionEventListener) i.next()).connectionClosed(event);
+ Object[] listeners = eventListeners.toArray();
+ for (int i = 0; i < listeners.length; i++) {
+ ((ConnectionEventListener) listeners[i]).connectionClosed(event);
}
}
@@ -134,9 +134,9 @@
* Pass error events on to listeners
*/
public void connectionErrorOccurred(ConnectionEvent event) {
- Iterator i = eventListeners.iterator();
- while (i.hasNext()) {
- ((ConnectionEventListener) i.next()).connectionErrorOccurred(event);
+ Object[] listeners = eventListeners.toArray();
+ for (int i = 0; i < listeners.length; i++) {
+ ((ConnectionEventListener) listeners[i]).connectionErrorOccurred(event);
}
}
Added: commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp/datasources/TestCPDSConnectionFactory.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp/datasources/TestCPDSConnectionFactory.java?rev=814240&view=auto
==============================================================================
--- commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp/datasources/TestCPDSConnectionFactory.java (added)
+++ commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp/datasources/TestCPDSConnectionFactory.java Sat Sep 12 21:20:59 2009
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.dbcp.datasources;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+
+import javax.sql.PooledConnection;
+
+import junit.framework.Test;
+import junit.framework.TestCase;
+import junit.framework.TestSuite;
+
+import org.apache.commons.dbcp.cpdsadapter.DriverAdapterCPDS;
+
+import org.apache.commons.pool.impl.GenericObjectPool;
+
+/**
+ * @version $Revision$ $Date$
+ */
+public class TestCPDSConnectionFactory extends TestCase {
+
+ protected ConnectionPoolDataSourceProxy cpds = null;
+
+ public TestCPDSConnectionFactory(String testName) {
+ super(testName);
+ }
+
+ public static Test suite() {
+ return new TestSuite(TestCPDSConnectionFactory.class);
+ }
+
+ public void setUp() throws Exception {
+ cpds = new ConnectionPoolDataSourceProxy(new DriverAdapterCPDS());
+ DriverAdapterCPDS delegate = (DriverAdapterCPDS) cpds.getDelegate();
+ delegate.setDriver("org.apache.commons.dbcp.TesterDriver");
+ delegate.setUrl("jdbc:apache:commons:testdriver");
+ delegate.setUser("username");
+ delegate.setPassword("password");
+ }
+
+ /**
+ * JIRA DBCP-216
+ *
+ * Check PoolableConnection close triggered by destroy is handled
+ * properly. PooledConnectionProxy (dubiously) fires connectionClosed
+ * when PooledConnection itself is closed.
+ */
+ public void testSharedPoolDSDestroyOnReturn() throws Exception {
+ PerUserPoolDataSource ds = new PerUserPoolDataSource();
+ ds.setConnectionPoolDataSource(cpds);
+ ds.setPerUserMaxActive("username",10);
+ ds.setPerUserMaxWait("username",50);
+ ds.setPerUserMaxIdle("username",2);
+ Connection conn1 = ds.getConnection("username", "password");
+ Connection conn2 = ds.getConnection("username", "password");
+ Connection conn3 = ds.getConnection("username", "password");
+ assertEquals(3, ds.getNumActive("username", "password"));
+ conn1.close();
+ assertEquals(1, ds.getNumIdle("username", "password"));
+ conn2.close();
+ assertEquals(2, ds.getNumIdle("username", "password"));
+ conn3.close(); // Return to pool will trigger destroy -> close sequence
+ assertEquals(2, ds.getNumIdle("username", "password"));
+ }
+
+ /**
+ * JIRA DBCP-216
+ *
+ * Verify that pool counters are maintained properly and listeners are
+ * cleaned up when a PooledConnection throws a connectionError event.
+ */
+ public void testConnectionErrorCleanup() throws Exception {
+ // Setup factory
+ GenericObjectPool pool = new GenericObjectPool(null);
+ CPDSConnectionFactory factory =
+ new CPDSConnectionFactory(cpds, pool, null, "username", "password");
+
+ // Checkout a pair of connections
+ PooledConnection pcon1 =
+ ((PooledConnectionAndInfo) pool.borrowObject())
+ .getPooledConnection();
+ Connection con1 = pcon1.getConnection();
+ PooledConnection pcon2 =
+ ((PooledConnectionAndInfo) pool.borrowObject())
+ .getPooledConnection();
+ assertEquals(2, pool.getNumActive());
+ assertEquals(0, pool.getNumIdle());
+
+ // Verify listening
+ PooledConnectionProxy pc = (PooledConnectionProxy) pcon1;
+ assertTrue(pc.getListeners().contains(factory));
+
+ // Throw connectionError event
+ pc.throwConnectionError();
+
+ // Active count should be reduced by 1 and no idle increase
+ assertEquals(1, pool.getNumActive());
+ assertEquals(0, pool.getNumIdle());
+
+ // Throw another one - should be ignored
+ pc.throwConnectionError();
+ assertEquals(1, pool.getNumActive());
+ assertEquals(0, pool.getNumIdle());
+
+ // Ask for another connection
+ PooledConnection pcon3 =
+ ((PooledConnectionAndInfo) pool.borrowObject())
+ .getPooledConnection();
+ assertTrue(!pcon3.equals(pcon1)); // better not get baddie back
+ assertTrue(!pc.getListeners().contains(factory)); // verify cleanup
+ assertEquals(2, pool.getNumActive());
+ assertEquals(0, pool.getNumIdle());
+
+ // Return good connections back to pool
+ pcon2.getConnection().close();
+ pcon3.getConnection().close();
+ assertEquals(2, pool.getNumIdle());
+ assertEquals(0, pool.getNumActive());
+
+ // Verify pc is closed
+ try {
+ pc.getConnection();
+ fail("Expecting SQLException using closed PooledConnection");
+ } catch (SQLException ex) {
+ // expected
+ }
+
+ // Back from the dead - ignore the ghost!
+ con1.close();
+ assertEquals(2, pool.getNumIdle());
+ assertEquals(0, pool.getNumActive());
+
+ // Clear pool
+ factory.getPool().clear();
+ assertEquals(0, pool.getNumIdle());
+ }
+
+}