HTTPCLIENT-841: removed connection garbage collection due to a memory leak

git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@784361 13f79535-47bb-0310-9956-ffa450edef68

RELEASE_NOTES.txt

@@ -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.

httpclient/src/main/java/org/apache/http/impl/conn/tsccm/AbstractConnPool.java

@@ -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 @@ public abstract class AbstractConnPool implements RefQueueHandler {
      */
     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 @@ public abstract class AbstractConnPool implements RefQueueHandler {
     @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).
+     * @deprecated do not sue
-     * 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
     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 @@ BasicPoolEntry getEntry(
     public abstract void freeEntry(BasicPoolEntry entry, boolean reusable, long validDuration, TimeUnit timeUnit)
         ;
 
-
+    @Deprecated
-
-    // non-javadoc, see interface RefQueueHandler
     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 {
+    @Deprecated
-            poolLock.unlock();
+    protected abstract void handleLostEntry(HttpRoute route);
-        }
-    }
-
-
-    /**
-     * 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)
-        ;
-
 
     /**
      * Closes idle connections.
@@ -291,23 +220,13 @@ public void shutdown() {
             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());
             }
-            }
-
-            // remove all references to connections
-            //@@@ use this for shutting them down instead?
             idleConnHandler.removeAll();
 
             isShutDown = true;

httpclient/src/main/java/org/apache/http/impl/conn/tsccm/BasicPoolEntry.java

@@ -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.
+     * @deprecated do not use
-     * Pool entries can only be GCed when they are allocated by an application
-     * and therefore not referenced with a hard link in the manager.
      */
-    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 @@ public class BasicPoolEntry extends AbstractPoolEntry {
      *                or <code>null</code>
      */
     public BasicPoolEntry(ClientConnectionOperator op,
-                          HttpRoute route,
+                          HttpRoute route) {
-                          ReferenceQueue<Object> queue) {
         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 @@ protected final HttpRoute getPlannedRoute() {
         return super.route;
     }
 
+    @Deprecated
     protected final BasicPoolEntryRef getWeakRef() {
-        return this.reference;
+        return null;
     }
 
     @Override
@@ -89,6 +91,6 @@ protected void shutdownEntry() {
         super.shutdownEntry();
     }
 
-} // class BasicPoolEntry
+}
 
 

httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java

@@ -283,7 +283,7 @@ protected BasicPoolEntry getEntryBlocking(
 
                 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 @@ public void freeEntry(BasicPoolEntry entry, boolean reusable, long validDuration
             }
 
             // 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 @@ protected BasicPoolEntry getFreeEntry(RouteSpecificPool rospl, Object state) {
                         rospl.dropEntry();
                         numConnections--;
                     } else {
-                        issuedConnections.add(entry.getWeakRef());
+                        leasedConnections.add(entry);
                         done = true;
                     }
     
@@ -486,17 +486,13 @@ protected BasicPoolEntry createEntry(RouteSpecificPool rospl,
         }
 
         // the entry will create the connection when needed
-        BasicPoolEntry entry =
+        BasicPoolEntry entry = new BasicPoolEntry(op, rospl.getRoute());
-            new BasicPoolEntry(op, rospl.getRoute(), refQueue);
 
         poolLock.lock();
         try {
-
             rospl.createdEntry(entry);
             numConnections++;
-
+            leasedConnections.add(entry);
-            issuedConnections.add(entry.getWeakRef());
-
         } finally {
             poolLock.unlock();
         }

httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueHandler.java

@@ -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 {
 
     /**

httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueWorker.java

@@ -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. */

httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java

@@ -123,13 +123,7 @@ protected void finalize() throws Throwable {
      * @return  the connection pool to use
      */
     protected AbstractConnPool createConnectionPool(final HttpParams params) {
-
+        return new ConnPoolByRoute(connOperator, params);
-        AbstractConnPool acp = new ConnPoolByRoute(connOperator, params);
-        boolean conngc = true; //@@@ check parameters to decide
-        if (conngc) {
-            acp.enableConnectionGC();
-        }
-        return acp;
     }
 
     /**

httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java

@@ -492,65 +492,6 @@ public void testReleaseConnectionOnAbort() throws Exception {
         mgr.shutdown();
     }
 
-    /**
-     * 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.
      */

httpclient/src/test/java/org/apache/http/impl/conn/tsccm/TestDumbHelpers.java

@@ -1,137 +0,0 @@
-/*
- * $HeadURL$
- * $Revision$
- * $Date$
- * ====================================================================
- * 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.
- * ====================================================================
- *
- * This software consists of voluntary contributions made by many
- * individuals on behalf of the Apache Software Foundation.  For more
- * information on the Apache Software Foundation, please see
- * <http://www.apache.org/>.
- *
- */
-
-package org.apache.http.impl.conn.tsccm;
-
-/*
-import java.util.Date;
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.Condition;
-import java.util.concurrent.locks.ReentrantLock;
-*/
-
-import junit.framework.Test;
-import junit.framework.TestCase;
-import junit.framework.TestSuite;
-
-
-import org.apache.http.HttpHost;
-import org.apache.http.conn.routing.HttpRoute;
-import org.apache.http.conn.scheme.PlainSocketFactory;
-import org.apache.http.conn.scheme.Scheme;
-import org.apache.http.conn.scheme.SchemeRegistry;
-import org.apache.http.conn.scheme.SocketFactory;
-import org.apache.http.conn.ClientConnectionOperator;
-import org.apache.http.impl.conn.DefaultClientConnectionOperator;
-
-
-
-/**
- * Tests for simple helper classes without advanced functionality.
- */
-public class TestDumbHelpers extends TestCase {
-
-    public final static
-        HttpHost TARGET = new HttpHost("target.test.invalid");
-
-
-    /** The default scheme registry. */
-    private SchemeRegistry supportedSchemes;
-
-
-
-    public TestDumbHelpers(String testName) {
-        super(testName);
-    }
-
-    public static void main(String args[]) {
-        String[] testCaseName = { TestDumbHelpers.class.getName() };
-        junit.textui.TestRunner.main(testCaseName);
-    }
-
-    public static Test suite() {
-        return new TestSuite(TestDumbHelpers.class);
-    }
-
-
-    @Override
-    protected void setUp() {
-        supportedSchemes = new SchemeRegistry();
-        SocketFactory sf = PlainSocketFactory.getSocketFactory();
-        supportedSchemes.register(new Scheme("http", sf, 80));
-    }
-
-
-
-
-    public void testBasicPoolEntry() {
-        HttpRoute route = new HttpRoute(TARGET);
-        ClientConnectionOperator ccop =
-            new DefaultClientConnectionOperator(supportedSchemes);
-
-        BasicPoolEntry bpe = null;
-        try {
-            bpe = new BasicPoolEntry(null, null, null);
-            fail("null operator not detected");
-        } catch (NullPointerException npx) {
-            // expected
-        } catch (IllegalArgumentException iax) {
-            // would be preferred
-        }
-
-        try {
-            bpe = new BasicPoolEntry(ccop, null, null);
-            fail("null route not detected");
-        } catch (IllegalArgumentException iax) {
-            // expected
-        }
-
-        bpe = new BasicPoolEntry(ccop, route, null);
-        assertEquals ("wrong route", route, bpe.getPlannedRoute());
-        assertNotNull("missing ref", bpe.getWeakRef());
-
-        assertEquals("bad weak ref", bpe, bpe.getWeakRef().get());
-        assertEquals("bad ref route", route, bpe.getWeakRef().getRoute());
-    }
-
-
-    public void testBasicPoolEntryRef() {
-        // the actual reference is tested implicitly with BasicPoolEntry
-        // but we need to cover the argument check in the constructor
-        try {
-            new BasicPoolEntryRef(null, null);
-            fail("null pool entry not detected");
-        } catch (IllegalArgumentException iax) {
-            // expected
-        }
-    }
-
-
-} // class TestDumbHelpers