commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1564961 - in /commons/proper/dbcp/trunk/src: main/java/org/apache/commons/dbcp2/ test/java/org/apache/commons/dbcp2/
Date Wed, 05 Feb 2014 22:30:36 GMT
Author: markt
Date: Wed Feb  5 22:30:36 2014
New Revision: 1564961

URL: http://svn.apache.org/r1564961
Log:
Fix DBCP-180
The garbage collector needs to be able to collect JDBC resources that the client is no longer
referencing. The AbandonedTrace code was stopping this. This has been fixed by using WeakReferences.
A secondary issue was that pooled statements would not get returned to the pool once the WeakReference
was used since AbandonedTrace lost the ability to trace all the child resources. Use of a
finalizer (I couldn't think of a better way) addresses that.

Added:
    commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TesterUtils.java   (with
props)
Modified:
    commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/AbandonedTrace.java
    commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/DelegatingStatement.java
    commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/PoolablePreparedStatement.java
    commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TestAbandonedBasicDataSource.java

Modified: commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/AbandonedTrace.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/AbandonedTrace.java?rev=1564961&r1=1564960&r2=1564961&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/AbandonedTrace.java (original)
+++ commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/AbandonedTrace.java Wed
Feb  5 22:30:36 2014
@@ -16,7 +16,9 @@
  */
 package org.apache.commons.dbcp2;
 
+import java.lang.ref.WeakReference;
 import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.List;
 
 import org.apache.commons.pool2.TrackedUse;
@@ -34,7 +36,7 @@ import org.apache.commons.pool2.TrackedU
 public class AbandonedTrace implements TrackedUse {
 
     /** A list of objects created by children of this object */
-    private final List<AbandonedTrace> traceList = new ArrayList<>();
+    private final List<WeakReference<AbandonedTrace>> traceList = new ArrayList<>();
     /** Last time this connection was used */
     private volatile long lastUsed = 0;
 
@@ -101,7 +103,7 @@ public class AbandonedTrace implements T
      */
     protected void addTrace(AbandonedTrace trace) {
         synchronized (this.traceList) {
-            this.traceList.add(trace);
+            this.traceList.add(new WeakReference<>(trace));
         }
         setLastUsed();
     }
@@ -122,9 +124,20 @@ public class AbandonedTrace implements T
      * @return List of objects
      */
     protected List<AbandonedTrace> getTrace() {
+        ArrayList<AbandonedTrace> result = new ArrayList<>(traceList.size());
         synchronized (this.traceList) {
-            return new ArrayList<>(traceList);
+            Iterator<WeakReference<AbandonedTrace>> iter = traceList.iterator();
+            while (iter.hasNext()) {
+                WeakReference<AbandonedTrace> ref = iter.next();
+                if (ref.get() == null) {
+                    // Clean-up since we are here anyway
+                    iter.remove();
+                } else {
+                    result.add(ref.get());
+                }
+            }
         }
+        return result;
     }
 
     /**
@@ -134,7 +147,17 @@ public class AbandonedTrace implements T
      */
     protected void removeTrace(AbandonedTrace trace) {
         synchronized(this.traceList) {
-            this.traceList.remove(trace);
+            Iterator<WeakReference<AbandonedTrace>> iter = traceList.iterator();
+            while (iter.hasNext()) {
+                WeakReference<AbandonedTrace> ref = iter.next();
+                if (trace.equals(ref.get())) {
+                    iter.remove();
+                    break;
+                } else if (ref.get() == null) {
+                    // Clean-up since we are here anyway
+                    iter.remove();
+                }
+            }
         }
     }
 }

Modified: commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/DelegatingStatement.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/DelegatingStatement.java?rev=1564961&r1=1564960&r2=1564961&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/DelegatingStatement.java
(original)
+++ commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/DelegatingStatement.java
Wed Feb  5 22:30:36 2014
@@ -536,4 +536,18 @@ public class DelegatingStatement extends
             return false;
         }
     }
+
+    @Override
+    protected void finalize() throws Throwable {
+        // This is required because of statement pooling. The poolable
+        // statements will always be strongly held by the statement pool. If the
+        // delegating statements that wrap the poolable statement are not
+        // strongly held they will be garbage collected but at that point the
+        // poolable statements need to be returned to the pool else there will
+        // be a leak of statements from the pool. Closing this statement will
+        // close all the wrapped statements and return any poolable statements
+        // to the pool.
+        close();
+        super.finalize();
+    }
 }

Modified: commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/PoolablePreparedStatement.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/PoolablePreparedStatement.java?rev=1564961&r1=1564960&r2=1564961&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/PoolablePreparedStatement.java
(original)
+++ commons/proper/dbcp/trunk/src/main/java/org/apache/commons/dbcp2/PoolablePreparedStatement.java
Wed Feb  5 22:30:36 2014
@@ -146,5 +146,4 @@ public class PoolablePreparedStatement<K
 
         super.passivate();
     }
-
 }

Modified: commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TestAbandonedBasicDataSource.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TestAbandonedBasicDataSource.java?rev=1564961&r1=1564960&r2=1564961&view=diff
==============================================================================
--- commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TestAbandonedBasicDataSource.java
(original)
+++ commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TestAbandonedBasicDataSource.java
Wed Feb  5 22:30:36 2014
@@ -24,6 +24,9 @@ import java.sql.PreparedStatement;
 import java.sql.SQLException;
 import java.sql.Statement;
 
+import org.apache.commons.pool2.impl.GenericKeyedObjectPool;
+import org.junit.Assert;
+
 import junit.framework.Test;
 import junit.framework.TestSuite;
 
@@ -207,6 +210,55 @@ public class TestAbandonedBasicDataSourc
     }
 
     /**
+     * DBCP-180 - verify that a GC can clean up an unused Statement when it is
+     * no longer referenced even when it is tracked via the AbandonedTrace
+     * mechanism.
+     */
+    public void testGarbageCollectorCleanUp01() throws Exception {
+        DelegatingConnection<?> conn = (DelegatingConnection<?>) ds.getConnection();
+        Assert.assertEquals(0, conn.getTrace().size());
+        createStatement(conn);
+        Assert.assertEquals(1, conn.getTrace().size());
+        System.gc();
+        Assert.assertEquals(0, conn.getTrace().size());
+    }
+
+    /**
+     * DBCP-180 - things get more interesting with statement pooling.
+     */
+    public void testGarbageCollectorCleanUp02() throws Exception {
+        ds.setPoolPreparedStatements(true);
+        ds.setAccessToUnderlyingConnectionAllowed(true);
+        DelegatingConnection<?> conn = (DelegatingConnection<?>) ds.getConnection();
+        PoolableConnection poolableConn = (PoolableConnection) conn.getDelegate();
+        PoolingConnection poolingConn = (PoolingConnection) poolableConn.getDelegate();
+        @SuppressWarnings("unchecked")
+        GenericKeyedObjectPool<PStmtKey,DelegatingPreparedStatement>  gkop =
+                (GenericKeyedObjectPool<PStmtKey,DelegatingPreparedStatement>) TesterUtils.getField(poolingConn,
"_pstmtPool");
+        Assert.assertEquals(0, conn.getTrace().size());
+        Assert.assertEquals(0, gkop.getNumActive());
+        createStatement(conn);
+        Assert.assertEquals(1, conn.getTrace().size());
+        Assert.assertEquals(1, gkop.getNumActive());
+        System.gc();
+        // Finalization happens in a separate thread. Give the test time for
+        // that to complete.
+        int count = 0;
+        while (count < 50 && gkop.getNumActive() > 0) {
+            Thread.sleep(100);
+            count++;
+        }
+        Assert.assertEquals(0, gkop.getNumActive());
+        Assert.assertEquals(0, conn.getTrace().size());
+    }
+
+    private void createStatement(Connection conn) throws Exception{
+        @SuppressWarnings("unused") // In a real use case it would be used then abandoned
+        PreparedStatement ps = conn.prepareStatement("");
+    }
+
+
+    /**
      * Verifies that Statement executeXxx methods update lastUsed on the parent connection
      */
     private void checkLastUsedStatement(Statement st, DelegatingConnection<?> conn)
throws Exception {

Added: commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TesterUtils.java
URL: http://svn.apache.org/viewvc/commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TesterUtils.java?rev=1564961&view=auto
==============================================================================
--- commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TesterUtils.java (added)
+++ commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TesterUtils.java Wed
Feb  5 22:30:36 2014
@@ -0,0 +1,38 @@
+/*
+ * 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.dbcp2;
+
+import java.lang.reflect.Field;
+
+public class TesterUtils {
+
+    private TesterUtils() {
+        // Utility class - hide default constructor
+    }
+
+    /**
+     * Access a private field. Do it this way rather than increasing the
+     * visibility of the field in the public API.
+     */
+    public static Object getField(Object target, String fieldName)
+            throws Exception {
+        Class<?> clazz = target.getClass();
+        Field f = clazz.getDeclaredField(fieldName);
+        f.setAccessible(true);
+        return f.get(target);
+    }
+}

Propchange: commons/proper/dbcp/trunk/src/test/java/org/apache/commons/dbcp2/TesterUtils.java
------------------------------------------------------------------------------
    svn:eol-style = native



Mime
View raw message