You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by ma...@apache.org on 2013/12/03 20:48:05 UTC
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/
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());
+ }
}