commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1547553 - in /commons/proper/dbcp/trunk/src: java/org/apache/commons/dbcp2/ java/org/apache/commons/dbcp2/cpdsadapter/ java/org/apache/commons/dbcp2/managed/ test/org/apache/commons/dbcp2/
Date Tue, 03 Dec 2013 19:48:05 GMT
Author: markt
Date: Tue Dec  3 19:48:05 2013
New Revision: 1547553

URL: http://svn.apache.org/r1547553
Log:
Refactor the closing of DelegatingConnection and sub-classes to correctly handle closing and
passivation of connections when one DelegatingConnection wraps another.
The problem was discovered while exploring some refactoring options and is now tested using
the additional unit test.

Modified:
    commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/DelegatingConnection.java
    commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/PoolingConnection.java
    commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/cpdsadapter/ConnectionImpl.java
    commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/managed/ManagedConnection.java
    commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp2/TestPoolableConnection.java

Modified: commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/DelegatingConnection.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/DelegatingConnection.java?rev=1547553&r1=1547552&r2=1547553&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/DelegatingConnection.java
(original)
+++ commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/DelegatingConnection.java
Tue Dec  3 19:48:05 2013
@@ -142,10 +142,7 @@ public class DelegatingConnection extend
         return getDelegateInternal();
     }
     
-    /**
-     * Should be final but can't be for compatibility with previous releases.
-     */
-    protected Connection getDelegateInternal() {
+    protected final Connection getDelegateInternal() {
         return _conn;
     }
     
@@ -234,13 +231,26 @@ public class DelegatingConnection extend
     }
 
     /**
-     * Closes the underlying connection, and close
-     * any Statements that were not explicitly closed.
+     * Closes the underlying connection, and close any Statements that were not
+     * explicitly closed. Sub-classes that override this method must:
+     * <ol>
+     * <li>Call passivate()</li>
+     * <li>Call close (or the equivalent appropriate action) on the wrapped
+     *     connection</li>
+     * <li>Set _closed to <code>false</code></li>
+     * </ol>
      */
     @Override
     public void close() throws SQLException {
-        passivate();
-        _conn.close();
+        try {
+            passivate();
+        } finally {
+            try {
+                _conn.close();
+            } finally {
+                _closed = true;
+            }
+        }
     }
 
     protected void handleException(SQLException e) throws SQLException {
@@ -584,33 +594,25 @@ public class DelegatingConnection extend
     }
 
     protected void passivate() throws SQLException {
-        try {
-            // The JDBC spec requires that a Connection close any open
-            // Statement's when it is closed.
-            // DBCP-288. Not all the traced objects will be statements
-            List<AbandonedTrace> traces = getTrace();
-            if(traces != null) {
-                Iterator<AbandonedTrace> traceIter = traces.iterator();
-                while (traceIter.hasNext()) {
-                    Object trace = traceIter.next();
-                    if (trace instanceof Statement) {
-                        ((Statement) trace).close();
-                    } else if (trace instanceof ResultSet) {
-                        // DBCP-265: Need to close the result sets that are
-                        // generated via DatabaseMetaData
-                        ((ResultSet) trace).close();
-                    }
+        // The JDBC spec requires that a Connection close any open
+        // Statement's when it is closed.
+        // DBCP-288. Not all the traced objects will be statements
+        List<AbandonedTrace> traces = getTrace();
+        if(traces != null) {
+            Iterator<AbandonedTrace> traceIter = traces.iterator();
+            while (traceIter.hasNext()) {
+                Object trace = traceIter.next();
+                if (trace instanceof Statement) {
+                    ((Statement) trace).close();
+                } else if (trace instanceof ResultSet) {
+                    // DBCP-265: Need to close the result sets that are
+                    // generated via DatabaseMetaData
+                    ((ResultSet) trace).close();
                 }
-                clearTrace();
             }
-            setLastUsed(0);
-            if(_conn instanceof DelegatingConnection) {
-                ((DelegatingConnection)_conn).passivate();
-            }
-        }
-        finally {
-            _closed = true;
+            clearTrace();
         }
+        setLastUsed(0);
     }
 
 

Modified: commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/PoolingConnection.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/PoolingConnection.java?rev=1547553&r1=1547552&r2=1547553&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/PoolingConnection.java (original)
+++ commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/PoolingConnection.java Tue
Dec  3 19:48:05 2013
@@ -32,8 +32,9 @@ import org.apache.commons.pool2.impl.Def
 /**
  * A {@link DelegatingConnection} that pools {@link PreparedStatement}s.
  * <p>
- * The {@link #prepareStatement} and {@link #prepareCall} methods, rather than creating a
new PreparedStatement
- * each time, may actually pull the statement from a pool of unused statements.
+ * The {@link #prepareStatement} and {@link #prepareCall} methods, rather than
+ * creating a new PreparedStatement each time, may actually pull the statement
+ * from a pool of unused statements.
  * The {@link PreparedStatement#close} method of the returned statement doesn't
  * actually close the statement, but rather returns it to the pool.
  * (See {@link PoolablePreparedStatement}, {@link PoolableCallableStatement}.)
@@ -73,23 +74,31 @@ public class PoolingConnection extends D
 
 
     /**
-     * Close and free all {@link PreparedStatement}s or {@link CallableStatement} from the
pool, and
-     * close the underlying connection.
+     * Close and free all {@link PreparedStatement}s or
+     * {@link CallableStatement}s from the pool, and close the underlying
+     * connection.
      */
     @Override
     public synchronized void close() throws SQLException {
-        if(null != _pstmtPool) {
-            KeyedObjectPool<PStmtKey,DelegatingPreparedStatement> oldpool = _pstmtPool;
-            _pstmtPool = null;
+        try {
+            if (null != _pstmtPool) {
+                KeyedObjectPool<PStmtKey,DelegatingPreparedStatement> oldpool = _pstmtPool;
+                _pstmtPool = null;
+                try {
+                    oldpool.close();
+                } catch(RuntimeException e) {
+                    throw e;
+                } catch(Exception e) {
+                    throw (SQLException) new SQLException("Cannot close connection").initCause(e);
+                }
+            }
+        } finally {
             try {
-                oldpool.close();
-            } catch(RuntimeException e) {
-                throw e;
-            } catch(Exception e) {
-                throw (SQLException) new SQLException("Cannot close connection").initCause(e);
+                getDelegateInternal().close();
+            } finally {
+                _closed = true;
             }
         }
-        getInnermostDelegate().close();
     }
 
     /**

Modified: commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/cpdsadapter/ConnectionImpl.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/cpdsadapter/ConnectionImpl.java?rev=1547553&r1=1547552&r2=1547553&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/cpdsadapter/ConnectionImpl.java
(original)
+++ commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/cpdsadapter/ConnectionImpl.java
Tue Dec  3 19:48:05 2013
@@ -74,9 +74,12 @@ class ConnectionImpl extends DelegatingC
     @Override
     public void close() throws SQLException {
         if (!_closed) {
-            _closed = true;
-            passivate();
-            pooledConnection.notifyListeners();
+            try {
+                passivate();
+            } finally {
+                _closed = true;
+                pooledConnection.notifyListeners();
+            }
         }
     }
 

Modified: commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/managed/ManagedConnection.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/managed/ManagedConnection.java?rev=1547553&r1=1547552&r2=1547553&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/managed/ManagedConnection.java
(original)
+++ commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp2/managed/ManagedConnection.java
Tue Dec  3 19:48:05 2013
@@ -148,10 +148,10 @@ public class ManagedConnection extends D
     public void close() throws SQLException {
         if (!_closed) {
             try {
-                // don't actually close the connection if in a transaction
-                // the connection will be closed by the transactionComplete method
+                // Don't actually close the connection if in a transaction. The
+                // connection will be closed by the transactionComplete method.
                 if (transactionContext == null) {
-                    getDelegateInternal().close();
+                    super.close();
                 }
             } finally {
                 _closed = true;
@@ -190,14 +190,12 @@ public class ManagedConnection extends D
                 setDelegate(null);
 
                 if (!delegate.isClosed()) {
-                    // don't use super.close() because it calls passivate() which marks the
-                    // the connection as closed without returning it to the pool
-                    delegate.close();
+                    super.close();
                 }
             } catch (SQLException ignored) {
                 // Not a whole lot we can do here as connection is closed
                 // and this is a transaction callback so there is no
-                // way to report the error
+                // way to report the error.
             }
         }
     }

Modified: commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp2/TestPoolableConnection.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp2/TestPoolableConnection.java?rev=1547553&r1=1547552&r2=1547553&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp2/TestPoolableConnection.java
(original)
+++ commons/proper/dbcp/trunk/src/test/org/apache/commons/dbcp2/TestPoolableConnection.java
Tue Dec  3 19:48:05 2013
@@ -14,7 +14,6 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-
 package org.apache.commons.dbcp2;
 
 import java.sql.Connection;
@@ -26,6 +25,7 @@ import junit.framework.TestSuite;
 
 import org.apache.commons.pool2.ObjectPool;
 import org.apache.commons.pool2.impl.GenericObjectPool;
+import org.junit.Assert;
 
 /**
  * @author James Ring
@@ -92,4 +92,21 @@ public class TestPoolableConnection exte
         assertEquals("The pool should have no active connections", 
             0, pool.getNumActive());
     }
+    
+    public void testClosingWrappedInDelegate() throws Exception {
+        Assert.assertEquals(0, pool.getNumActive());
+        
+        Connection conn = pool.borrowObject();
+        DelegatingConnection outer = new DelegatingConnection(conn);
+        
+        Assert.assertFalse(outer.isClosed());
+        Assert.assertFalse(conn.isClosed());
+        Assert.assertEquals(1, pool.getNumActive());
+
+        outer.close();
+        
+        Assert.assertTrue(outer.isClosed());
+        Assert.assertFalse(conn.isClosed());
+        Assert.assertEquals(0, pool.getNumActive());
+    }
 }



Mime
View raw message