commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pste...@apache.org
Subject 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/
Date Sat, 12 Sep 2009 21:21:04 GMT
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());
+    }
+
+}



Mime
View raw message