433689 - Evict old HttpDestinations from HttpClient.

5 files changed, 91 insertions, 4 deletions

diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/ConnectionPool.java b/jetty-client/src/main/java/org/eclipse/jetty/client/ConnectionPool.java
index 42eae39129..627228b3e3 100644
--- a/jetty-client/src/main/java/org/eclipse/jetty/client/ConnectionPool.java
+++ b/jetty-client/src/main/java/org/eclipse/jetty/client/ConnectionPool.java
@@ -192,6 +192,11 @@ public class ConnectionPool implements Closeable, Dumpable
         return idleConnections.contains(connection);
     }
 
+    public boolean isEmpty()
+    {
+        return connectionCount.get() == 0;
+    }
+
     public void close()
     {
         for (Connection connection : idleConnections)
diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java
index 3640ee058e..54b82f9ba0 100644
--- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java
+++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java
@@ -133,6 +133,7 @@ public class HttpClient extends ContainerLifeCycle
     private volatile boolean dispatchIO = true;
     private volatile boolean strictEventOrdering = false;
     private volatile HttpField encodingField;
+    private volatile boolean removeIdleDestinations = false;
 
     /**
      * Creates a {@link HttpClient} instance that can perform requests to non-TLS destinations only
@@ -464,6 +465,11 @@ public class HttpClient extends ContainerLifeCycle
         return destination;
     }
 
+    protected boolean removeDestination(HttpDestination destination)
+    {
+        return destinations.remove(destination.getOrigin()) != null;
+    }
+
     /**
      * @return the list of destinations known to this {@link HttpClient}.
      */
@@ -864,6 +870,32 @@ public class HttpClient extends ContainerLifeCycle
     }
 
     /**
+     * @return whether destinations that have no connections should be removed
+     * @see #setRemoveIdleDestinations(boolean)
+     */
+    public boolean isRemoveIdleDestinations()
+    {
+        return removeIdleDestinations;
+    }
+
+    /**
+     * Whether destinations that have no connections (nor active nor idle) should be removed.
+     * <p />
+     * Applications typically make request to a limited number of destinations so keeping
+     * destinations around is not a problem for the memory or the GC.
+     * However, for applications that hit millions of different destinations (e.g. a spider
+     * bot) it would be useful to be able to remove the old destinations that won't be visited
+     * anymore and leave space for new destinations.
+     *
+     * @param removeIdleDestinations whether destinations that have no connections should be removed
+     * @see org.eclipse.jetty.client.ConnectionPool
+     */
+    public void setRemoveIdleDestinations(boolean removeIdleDestinations)
+    {
+        this.removeIdleDestinations = removeIdleDestinations;
+    }
+
+    /**
      * @return the forward proxy configuration
      */
     public ProxyConfiguration getProxyConfiguration()
diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java
index 563c92dc90..475007523f 100644
--- a/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java
+++ b/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java
@@ -146,7 +146,11 @@ public abstract class MultiplexHttpDestination<C extends Connection> extends Htt
         {
             ConnectState current = connect.get();
             if (connect.compareAndSet(current, ConnectState.DISCONNECTED))
+            {
+                if (getHttpClient().isRemoveIdleDestinations())
+                    getHttpClient().removeDestination(this);
                 break;
+            }
         }
     }
 
diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java
index 73c13ad45d..24c77e84d8 100644
--- a/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java
+++ b/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java
@@ -168,11 +168,24 @@ public abstract class PoolingHttpDestination<C extends Connection> extends HttpD
         super.close(oldConnection);
         connectionPool.remove(oldConnection);
 
-        // We need to execute queued requests even if this connection failed.
-        // We may create a connection that is not needed, but it will eventually
-        // idle timeout, so no worries
-        if (!getHttpExchanges().isEmpty())
+        if (getHttpExchanges().isEmpty())
         {
+            if (getHttpClient().isRemoveIdleDestinations() && connectionPool.isEmpty())
+            {
+                // There is a race condition between this thread removing the destination
+                // and another thread queueing a request to this same destination.
+                // If this destination is removed, but the request queued, a new connection
+                // will be opened, the exchange will be executed and eventually the connection
+                // will idle timeout and be closed. Meanwhile a new destination will be created
+                // in HttpClient and will be used for other requests.
+                getHttpClient().removeDestination(this);
+            }
+        }
+        else
+        {
+            // We need to execute queued requests even if this connection failed.
+            // We may create a connection that is not needed, but it will eventually
+            // idle timeout, so no worries.
             C newConnection = acquire();
             if (newConnection != null)
                 process(newConnection, false);
diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java
index ad21f24012..b9723efc64 100644
--- a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java
+++ b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java
@@ -27,9 +27,12 @@ import org.eclipse.jetty.client.EmptyServerHandler;
 import org.eclipse.jetty.client.Origin;
 import org.eclipse.jetty.client.api.Connection;
 import org.eclipse.jetty.client.api.ContentResponse;
+import org.eclipse.jetty.client.api.Destination;
 import org.eclipse.jetty.client.api.Request;
 import org.eclipse.jetty.client.api.Response;
 import org.eclipse.jetty.client.api.Result;
+import org.eclipse.jetty.http.HttpHeader;
+import org.eclipse.jetty.http.HttpHeaderValue;
 import org.eclipse.jetty.toolchain.test.annotation.Slow;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.hamcrest.Matchers;
@@ -226,4 +229,34 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest
         Assert.assertTrue(failureLatch.await(5, TimeUnit.SECONDS));
         Assert.assertTrue(successLatch.await(5, TimeUnit.SECONDS));
     }
+
+    @Test
+    public void testDestinationIsRemoved() throws Exception
+    {
+        String host = "localhost";
+        int port = connector.getLocalPort();
+        Destination destinationBefore = client.getDestination(scheme, host, port);
+
+        ContentResponse response = client.newRequest(host, port)
+                .scheme(scheme)
+                .header(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString())
+                .send();
+
+        Assert.assertEquals(200, response.getStatus());
+
+        Destination destinationAfter = client.getDestination(scheme, host, port);
+        Assert.assertSame(destinationBefore, destinationAfter);
+
+        client.setRemoveIdleDestinations(true);
+
+        response = client.newRequest(host, port)
+                .scheme(scheme)
+                .header(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString())
+                .send();
+
+        Assert.assertEquals(200, response.getStatus());
+
+        destinationAfter = client.getDestination(scheme, host, port);
+        Assert.assertNotSame(destinationBefore, destinationAfter);
+    }
 }