hc-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ol...@apache.org
Subject svn commit: r784361 - in /httpcomponents/httpclient/trunk: ./ httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ httpclient/src/test/java/org/apache/http/impl/conn/ httpclient/src/test/java/org/apache/http/impl/conn/tsccm/
Date Sat, 13 Jun 2009 10:13:37 GMT
Author: olegk
Date: Sat Jun 13 10:13:36 2009
New Revision: 784361

URL: http://svn.apache.org/viewvc?rev=784361&view=rev
Log:
HTTPCLIENT-841: removed connection garbage collection due to a memory leak

Removed:
    httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/conn/tsccm/TestDumbHelpers.java
Modified:
    httpcomponents/httpclient/trunk/RELEASE_NOTES.txt
    httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/AbstractConnPool.java
    httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/BasicPoolEntry.java
    httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java
    httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueHandler.java
    httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueWorker.java
    httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java
    httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java

Modified: httpcomponents/httpclient/trunk/RELEASE_NOTES.txt
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/RELEASE_NOTES.txt?rev=784361&r1=784360&r2=784361&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/RELEASE_NOTES.txt (original)
+++ httpcomponents/httpclient/trunk/RELEASE_NOTES.txt Sat Jun 13 10:13:36 2009
@@ -1,6 +1,10 @@
 Changes since 4.0 beta 2
 -------------------
 
+* [HTTPCLIENT-841] Removed automatic connection release using garbage collection 
+  due to a memory leak
+  Contributed by Oleg Kalnichevski <olegk at apache.org>
+
 * [HTTPCLIENT-853] Fixed bug causing invalid cookie origin port to be selected 
   when the target is accessed on the default port and the connection is 
   established via a proxy.

Modified: httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/AbstractConnPool.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/AbstractConnPool.java?rev=784361&r1=784360&r2=784361&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/AbstractConnPool.java
(original)
+++ httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/AbstractConnPool.java
Sat Jun 13 10:13:36 2009
@@ -41,7 +41,7 @@
 import java.util.concurrent.locks.ReentrantLock;
 
 import net.jcip.annotations.GuardedBy;
-import net.jcip.annotations.NotThreadSafe;
+import net.jcip.annotations.ThreadSafe;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -50,7 +50,6 @@
 import org.apache.http.conn.routing.HttpRoute;
 import org.apache.http.impl.conn.IdleConnectionHandler;
 
-
 /**
  * An abstract connection pool.
  * It is used by the {@link ThreadSafeClientConnManager}.
@@ -60,7 +59,9 @@
  *
  * @since 4.0
  */
-@NotThreadSafe // unsynch access to refQueue, refWorker
+
+@ThreadSafe
+@SuppressWarnings("deprecation")
 public abstract class AbstractConnPool implements RefQueueHandler {
 
     private final Log log = LogFactory.getLog(getClass());
@@ -70,17 +71,12 @@
      */
     protected final Lock poolLock;
 
-
     /**
      * References to issued connections.
-     * Objects in this set are of class
-     * {@link BasicPoolEntryRef BasicPoolEntryRef},
-     * and point to the pool entry for the issued connection.
-     * GCed connections are detected by the missing pool entries.
      * Must hold poolLock when accessing.
      */
     @GuardedBy("poolLock")
-    protected Set<BasicPoolEntryRef> issuedConnections;
+    protected Set<BasicPoolEntry> leasedConnections;
 
     /** The handler for idle connections. Must hold poolLock when accessing. */
     @GuardedBy("poolLock")
@@ -90,66 +86,32 @@
     @GuardedBy("poolLock")
     protected int numConnections;
 
-    /**
-     * A reference queue to track loss of pool entries to GC.
-     * The same queue is used to track loss of the connection manager,
-     * so we cannot specialize the type.
-     */
-    // TODO - this needs to be synchronized, e.g. on Pool Lock
-    protected ReferenceQueue<Object> refQueue;
-
-    /** A worker (thread) to track loss of pool entries to GC. */
-    // TODO - this needs to be synchronized, e.g. on Pool Lock
-    private RefQueueWorker refWorker;
-
-
     /** Indicates whether this pool is shut down. */
     protected volatile boolean isShutDown;
 
+    @Deprecated
+    protected Set<BasicPoolEntryRef> issuedConnections;
+
+    @Deprecated
+    protected ReferenceQueue<Object> refQueue;
+
     /**
      * Creates a new connection pool.
      */
     protected AbstractConnPool() {
-        issuedConnections = new HashSet<BasicPoolEntryRef>();
+        leasedConnections = new HashSet<BasicPoolEntry>();
         idleConnHandler = new IdleConnectionHandler();
 
         boolean fair = false; //@@@ check parameters to decide
         poolLock = new ReentrantLock(fair);
     }
 
-
     /**
-     * Enables connection garbage collection (GC).
-     * This method must be called immediately after creating the
-     * connection pool. It is not possible to enable connection GC
-     * after pool entries have been created. Neither is it possible
-     * to disable connection GC.
-     *
-     * @throws IllegalStateException
-     *         if connection GC is already enabled, or if it cannot be
-     *         enabled because there already are pool entries
+     * @deprecated do not sue
      */
+    @Deprecated
     public void enableConnectionGC()
         throws IllegalStateException {
-
-        if (refQueue != null) { // TODO - this access is not guaranteed protected by the
pool lock
-            throw new IllegalStateException("Connection GC already enabled.");
-        }
-        poolLock.lock();
-        try {
-            if (numConnections > 0) { //@@@ is this check sufficient?
-                throw new IllegalStateException("Pool already in use.");
-            }
-        } finally {
-            poolLock.unlock();
-        }
-
-        refQueue  = new ReferenceQueue<Object>();
-        refWorker = new RefQueueWorker(refQueue, this);
-        Thread t = new Thread(refWorker); //@@@ use a thread factory
-        t.setDaemon(true);
-        t.setName("RefQueueWorker@" + this);
-        t.start();
     }
 
 
@@ -200,45 +162,12 @@
     public abstract void freeEntry(BasicPoolEntry entry, boolean reusable, long validDuration,
TimeUnit timeUnit)
         ;
 
-
-
-    // non-javadoc, see interface RefQueueHandler
+    @Deprecated
     public void handleReference(Reference<?> ref) {
-
-        poolLock.lock();
-        try {
-
-            if (ref instanceof BasicPoolEntryRef) {
-                // check if the GCed pool entry was still in use
-                //@@@ find a way to detect this without lookup
-                //@@@ flag in the BasicPoolEntryRef, to be reset when freed?
-                final boolean lost = issuedConnections.remove(ref);
-                if (lost) {
-                    final HttpRoute route =
-                        ((BasicPoolEntryRef)ref).getRoute();
-                    if (log.isDebugEnabled()) {
-                        log.debug("Connection garbage collected. " + route);
-                    }
-                    handleLostEntry(route);
-                }
-            }
-
-        } finally {
-            poolLock.unlock();
-        }
     }
 
-
-    /**
-     * Handles cleaning up for a lost pool entry with the given route.
-     * A lost pool entry corresponds to a connection that was
-     * garbage collected instead of being properly released.
-     *
-     * @param route     the route of the pool entry that was lost
-     */
-    protected abstract void handleLostEntry(HttpRoute route)
-        ;
-
+    @Deprecated
+    protected abstract void handleLostEntry(HttpRoute route);
 
     /**
      * Closes idle connections.
@@ -291,23 +220,13 @@
             if (isShutDown)
                 return;
 
-            // no point in monitoring GC anymore
-            if (refWorker != null)
-                refWorker.shutdown();
-
             // close all connections that are issued to an application
-            Iterator<BasicPoolEntryRef> iter = issuedConnections.iterator();
+            Iterator<BasicPoolEntry> iter = leasedConnections.iterator();
             while (iter.hasNext()) {
-                BasicPoolEntryRef per = iter.next();
+                BasicPoolEntry entry = iter.next();
                 iter.remove();
-                BasicPoolEntry entry = per.get();
-                if (entry != null) {
-                    closeConnection(entry.getConnection());
-                }
+                closeConnection(entry.getConnection());
             }
-
-            // remove all references to connections
-            //@@@ use this for shutting them down instead?
             idleConnHandler.removeAll();
 
             isShutDown = true;

Modified: httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/BasicPoolEntry.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/BasicPoolEntry.java?rev=784361&r1=784360&r2=784361&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/BasicPoolEntry.java
(original)
+++ httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/BasicPoolEntry.java
Sat Jun 13 10:13:36 2009
@@ -30,7 +30,6 @@
 
 package org.apache.http.impl.conn.tsccm;
 
-
 import java.lang.ref.ReferenceQueue;
 
 import org.apache.http.conn.OperatedClientConnection;
@@ -38,8 +37,6 @@
 import org.apache.http.conn.routing.HttpRoute;
 import org.apache.http.impl.conn.AbstractPoolEntry;
 
-
-
 /**
  * Basic implementation of a connection pool entry.
  *
@@ -48,11 +45,17 @@
 public class BasicPoolEntry extends AbstractPoolEntry {
 
     /**
-     * A weak reference to <code>this</code> used to detect GC of entries.
-     * Pool entries can only be GCed when they are allocated by an application
-     * and therefore not referenced with a hard link in the manager.
+     * @deprecated do not use
      */
-    private final BasicPoolEntryRef reference;
+    @Deprecated
+    public BasicPoolEntry(ClientConnectionOperator op,
+                          HttpRoute route,
+                          ReferenceQueue<Object> queue) {
+        super(op, route);
+        if (route == null) {
+            throw new IllegalArgumentException("HTTP route may not be null");
+        }
+    }
 
     /**
      * Creates a new pool entry.
@@ -63,13 +66,11 @@
      *                or <code>null</code>
      */
     public BasicPoolEntry(ClientConnectionOperator op,
-                          HttpRoute route,
-                          ReferenceQueue<Object> queue) {
+                          HttpRoute route) {
         super(op, route);
         if (route == null) {
             throw new IllegalArgumentException("HTTP route may not be null");
         }
-        this.reference = new BasicPoolEntryRef(this, queue);
     }
 
     protected final OperatedClientConnection getConnection() {
@@ -80,8 +81,9 @@
         return super.route;
     }
 
+    @Deprecated
     protected final BasicPoolEntryRef getWeakRef() {
-        return this.reference;
+        return null;
     }
 
     @Override
@@ -89,6 +91,6 @@
         super.shutdownEntry();
     }
 
-} // class BasicPoolEntry
+}
 
 

Modified: httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java?rev=784361&r1=784360&r2=784361&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java
(original)
+++ httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java
Sat Jun 13 10:13:36 2009
@@ -283,7 +283,7 @@
 
                 if (log.isDebugEnabled()) {
                     log.debug("Total connections kept alive: " + freeConnections.size());

-                    log.debug("Total issued connections: " + issuedConnections.size()); 
+                    log.debug("Total issued connections: " + leasedConnections.size()); 
                     log.debug("Total allocated connection: " + numConnections + " out of
" + maxTotalConnections);
                 }
                 
@@ -381,7 +381,7 @@
             }
 
             // no longer issued, we keep a hard reference now
-            issuedConnections.remove(entry.getWeakRef());
+            leasedConnections.remove(entry);
 
             RouteSpecificPool rospl = getRoutePool(route, true);
 
@@ -448,7 +448,7 @@
                         rospl.dropEntry();
                         numConnections--;
                     } else {
-                        issuedConnections.add(entry.getWeakRef());
+                        leasedConnections.add(entry);
                         done = true;
                     }
     
@@ -486,17 +486,13 @@
         }
 
         // the entry will create the connection when needed
-        BasicPoolEntry entry =
-            new BasicPoolEntry(op, rospl.getRoute(), refQueue);
+        BasicPoolEntry entry = new BasicPoolEntry(op, rospl.getRoute());
 
         poolLock.lock();
         try {
-
             rospl.createdEntry(entry);
             numConnections++;
-
-            issuedConnections.add(entry.getWeakRef());
-
+            leasedConnections.add(entry);
         } finally {
             poolLock.unlock();
         }

Modified: httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueHandler.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueHandler.java?rev=784361&r1=784360&r2=784361&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueHandler.java
(original)
+++ httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueHandler.java
Sat Jun 13 10:13:36 2009
@@ -32,12 +32,12 @@
 
 import java.lang.ref.Reference;
 
-
 /**
- * Callback handler for {@link RefQueueWorker RefQueueWorker}.
+ * @deprecated do not use
  *
  * @since 4.0
  */
+@Deprecated
 public interface RefQueueHandler {
 
     /**

Modified: httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueWorker.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueWorker.java?rev=784361&r1=784360&r2=784361&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueWorker.java
(original)
+++ httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueWorker.java
Sat Jun 13 10:13:36 2009
@@ -42,8 +42,9 @@
  * this worker. It will pick up the queued references and pass them
  * on to a handler for appropriate processing.
  *
- * @since 4.0
+ * @deprecated do not use
  */
+@Deprecated
 public class RefQueueWorker implements Runnable {
 
     /** The reference queue to monitor. */

Modified: httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java?rev=784361&r1=784360&r2=784361&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java
(original)
+++ httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java
Sat Jun 13 10:13:36 2009
@@ -123,13 +123,7 @@
      * @return  the connection pool to use
      */
     protected AbstractConnPool createConnectionPool(final HttpParams params) {
-
-        AbstractConnPool acp = new ConnPoolByRoute(connOperator, params);
-        boolean conngc = true; //@@@ check parameters to decide
-        if (conngc) {
-            acp.enableConnectionGC();
-        }
-        return acp;
+        return new ConnPoolByRoute(connOperator, params);
     }
 
     /**

Modified: httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java?rev=784361&r1=784360&r2=784361&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java
(original)
+++ httpcomponents/httpclient/trunk/httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java
Sat Jun 13 10:13:36 2009
@@ -493,65 +493,6 @@
     }
 
     /**
-     * Tests GC of an unreferenced connection.
-     */
-    public void testConnectionGC() throws Exception {
-        // 3.x: TestHttpConnectionManager.testReclaimUnusedConnection
-
-        HttpParams mgrpar = defaultParams.copy();
-        ConnManagerParams.setMaxTotalConnections(mgrpar, 1);
-
-        ThreadSafeClientConnManager mgr = createTSCCM(mgrpar, null);
-
-        final HttpHost target = getServerHttp();
-        final HttpRoute route = new HttpRoute(target, null, false);
-        final int      rsplen = 8;
-        final String      uri = "/random/" + rsplen;
-
-        HttpRequest request =
-            new BasicHttpRequest("GET", uri, HttpVersion.HTTP_1_1);
-
-        ManagedClientConnection conn = getConnection(mgr, route);
-        conn.open(route, httpContext, defaultParams);
-
-        // a new context is created for each testcase, no need to reset
-        Helper.execute(request, conn, target, 
-                httpExecutor, httpProcessor, defaultParams, httpContext);
-
-        // we leave the connection in mid-use
-        // it's not really re-usable, but it must be closed anyway
-        conn.markReusable();
-
-        // first check that we can't get another connection
-        try {
-            // this should fail quickly, connection has not been released
-            getConnection(mgr, route, 10L, TimeUnit.MILLISECONDS);
-            fail("ConnectionPoolTimeoutException should have been thrown");
-        } catch (ConnectionPoolTimeoutException e) {
-            // expected
-        }
-
-        // We now drop the hard references to the connection and trigger GC.
-        WeakReference<ManagedClientConnection> wref = 
-            new WeakReference<ManagedClientConnection>(conn);
-        conn = null;
-        httpContext = null; // holds a reference to the connection
-
-        // Java does not guarantee that this will trigger the GC, but
-        // it does in the test environment. GC is asynchronous, so we
-        // need to give the garbage collector some time afterwards.
-        System.gc();
-        Thread.sleep(1000);
-
-        assertNull("connection not garbage collected", wref.get());
-        conn = getConnection(mgr, route, 10L, TimeUnit.MILLISECONDS);
-        assertFalse("GCed connection not closed", conn.isOpen());
-
-        mgr.shutdown();
-    }
-
-
-    /**
      * Tests GC of an unreferenced connection manager.
      */
     public void testConnectionManagerGC() throws Exception {



Mime
View raw message