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