From dee6af01ad29c9a9e6829f86b4e97b5abff9ab6b Mon Sep 17 00:00:00 2001 From: Sam Berlin Date: Fri, 27 Jun 2008 19:49:20 +0000 Subject: [PATCH] simplified keep-alive support. changed ConnectionKeepAliveStrategy to return time in ms, instead of time & timeunit in two different calls. reverted ConnectionReleaseTrigger.releaseConnection to take zero params & instead added a setIdleDuration method to ManagedClientConnection, along with converting direct uses of ClientConnectionManager.releaseConnection to instead go through ManagedClientConnection.releaseConnection (which now uses the idle time when giving itself to the manager). git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@672367 13f79535-47bb-0310-9956-ffa450edef68 --- .../http/conn/BasicEofSensorWatcher.java | 4 +- .../apache/http/conn/BasicManagedEntity.java | 23 ++------ .../conn/ConnectionKeepAliveStrategy.java | 9 +--- .../http/conn/ConnectionReleaseTrigger.java | 8 +-- .../http/conn/EofSensorInputStream.java | 3 +- .../http/conn/ManagedClientConnection.java | 9 ++++ .../client/DefaultClientRequestDirector.java | 44 +++++++++------ .../DefaultConnectionKeepAliveStrategy.java | 8 +-- .../impl/conn/AbstractClientConnAdapter.java | 53 +++++++++++++++++-- .../TestDefaultClientRequestDirector.java | 6 +-- .../TestDefaultConnKeepAliveStrategy.java | 8 +-- .../impl/conn/ClientConnAdapterMockup.java | 5 +- .../http/impl/conn/TestTSCCMNoServer.java | 2 +- .../http/impl/conn/TestTSCCMWithServer.java | 8 +-- 14 files changed, 111 insertions(+), 79 deletions(-) diff --git a/module-client/src/main/java/org/apache/http/conn/BasicEofSensorWatcher.java b/module-client/src/main/java/org/apache/http/conn/BasicEofSensorWatcher.java index c9c95edb5..b2e676c1c 100644 --- a/module-client/src/main/java/org/apache/http/conn/BasicEofSensorWatcher.java +++ b/module-client/src/main/java/org/apache/http/conn/BasicEofSensorWatcher.java @@ -86,7 +86,7 @@ public class BasicEofSensorWatcher implements EofSensorWatcher { managedConn.markReusable(); } } finally { - managedConn.releaseConnection(-1, null); + managedConn.releaseConnection(); } return false; } @@ -104,7 +104,7 @@ public class BasicEofSensorWatcher implements EofSensorWatcher { managedConn.markReusable(); } } finally { - managedConn.releaseConnection(-1, null); + managedConn.releaseConnection(); } return false; } diff --git a/module-client/src/main/java/org/apache/http/conn/BasicManagedEntity.java b/module-client/src/main/java/org/apache/http/conn/BasicManagedEntity.java index d1680c40b..232272f27 100644 --- a/module-client/src/main/java/org/apache/http/conn/BasicManagedEntity.java +++ b/module-client/src/main/java/org/apache/http/conn/BasicManagedEntity.java @@ -30,10 +30,9 @@ package org.apache.http.conn; -import java.io.InputStream; import java.io.IOException; +import java.io.InputStream; import java.io.OutputStream; -import java.util.concurrent.TimeUnit; import org.apache.http.HttpEntity; import org.apache.http.entity.HttpEntityWrapper; @@ -61,12 +60,6 @@ public class BasicManagedEntity extends HttpEntityWrapper /** Whether to keep the connection alive. */ protected final boolean attemptReuse; - - /** The duration this is valid for. */ - protected final long validDuration; - - /** The unit of time the duration is valid for. */ - protected final TimeUnit validUnit; /** @@ -81,9 +74,7 @@ public class BasicManagedEntity extends HttpEntityWrapper */ public BasicManagedEntity(HttpEntity entity, ManagedClientConnection conn, - boolean reuse, - long validDuration, - TimeUnit validUnit) { + boolean reuse) { super(entity); if (conn == null) @@ -92,8 +83,6 @@ public class BasicManagedEntity extends HttpEntityWrapper this.managedConn = conn; this.attemptReuse = reuse; - this.validDuration = validDuration; - this.validUnit = validUnit; } @@ -140,7 +129,7 @@ public class BasicManagedEntity extends HttpEntityWrapper // non-javadoc, see interface ConnectionReleaseTrigger - public void releaseConnection(long validDuration, TimeUnit timeUnit) + public void releaseConnection() throws IOException { this.consumeContent(); @@ -221,11 +210,7 @@ public class BasicManagedEntity extends HttpEntityWrapper if (managedConn != null) { try { - // TODO: Should this be subtracting the elapsed time from - // when the entity was created till now? - // There's no good specification for when the 'timeout' - // starts counting. - managedConn.releaseConnection(validDuration, validUnit); + managedConn.releaseConnection(); } finally { managedConn = null; } diff --git a/module-client/src/main/java/org/apache/http/conn/ConnectionKeepAliveStrategy.java b/module-client/src/main/java/org/apache/http/conn/ConnectionKeepAliveStrategy.java index 2c50b0338..27d56cb60 100644 --- a/module-client/src/main/java/org/apache/http/conn/ConnectionKeepAliveStrategy.java +++ b/module-client/src/main/java/org/apache/http/conn/ConnectionKeepAliveStrategy.java @@ -30,8 +30,6 @@ */ package org.apache.http.conn; -import java.util.concurrent.TimeUnit; - import org.apache.http.ConnectionReuseStrategy; import org.apache.http.HttpResponse; import org.apache.http.protocol.HttpContext; @@ -48,9 +46,6 @@ import org.apache.http.protocol.HttpContext; * @since 4.0 */ public interface ConnectionKeepAliveStrategy { - - /** Returns the TimeUnit that this uses for specifying duration. */ - TimeUnit getTimeUnit(); /** * Returns the duration of time which this connection can be safely kept @@ -68,8 +63,8 @@ public interface ConnectionKeepAliveStrategy { * @param context * the context in which the connection is being used. * - * @return the duration which it is safe to keep the connection idle, - * or <=0 if no suggested duration. + * @return the duration in ms for which it is safe to keep the connection + * idle, or <=0 if no suggested duration. */ long getKeepAliveDuration(HttpResponse response, HttpContext context); diff --git a/module-client/src/main/java/org/apache/http/conn/ConnectionReleaseTrigger.java b/module-client/src/main/java/org/apache/http/conn/ConnectionReleaseTrigger.java index bcac1a2c1..07dd14eea 100644 --- a/module-client/src/main/java/org/apache/http/conn/ConnectionReleaseTrigger.java +++ b/module-client/src/main/java/org/apache/http/conn/ConnectionReleaseTrigger.java @@ -31,7 +31,6 @@ package org.apache.http.conn; import java.io.IOException; -import java.util.concurrent.TimeUnit; /** @@ -63,16 +62,11 @@ public interface ConnectionReleaseTrigger { * {@link #abortConnection abortConnection} for a hard release. The * connection may be reused as specified by the duration. * - * @param validDuration - * The duration of time this connection is valid to be reused. - * @param timeUnit - * The time unit the duration is measured in. - * * @throws IOException * in case of an IO problem. The connection will be released * anyway. */ - void releaseConnection(long validDuration, TimeUnit timeUnit) + void releaseConnection() throws IOException ; diff --git a/module-client/src/main/java/org/apache/http/conn/EofSensorInputStream.java b/module-client/src/main/java/org/apache/http/conn/EofSensorInputStream.java index 91901de96..bb37510b9 100644 --- a/module-client/src/main/java/org/apache/http/conn/EofSensorInputStream.java +++ b/module-client/src/main/java/org/apache/http/conn/EofSensorInputStream.java @@ -32,7 +32,6 @@ package org.apache.http.conn; import java.io.InputStream; import java.io.IOException; -import java.util.concurrent.TimeUnit; /** @@ -306,7 +305,7 @@ public class EofSensorInputStream extends InputStream /** * Same as {@link #close close()}. */ - public void releaseConnection(long validDuration, TimeUnit timeUnit) throws IOException { + public void releaseConnection() throws IOException { this.close(); } diff --git a/module-client/src/main/java/org/apache/http/conn/ManagedClientConnection.java b/module-client/src/main/java/org/apache/http/conn/ManagedClientConnection.java index 110fe30d0..bb0106203 100644 --- a/module-client/src/main/java/org/apache/http/conn/ManagedClientConnection.java +++ b/module-client/src/main/java/org/apache/http/conn/ManagedClientConnection.java @@ -32,6 +32,8 @@ package org.apache.http.conn; import java.io.IOException; +import java.util.concurrent.TimeUnit; + import javax.net.ssl.SSLSession; import org.apache.http.HttpClientConnection; @@ -245,5 +247,12 @@ public interface ManagedClientConnection extends */ Object getState() ; + + /** + * Sets the duration that this connection can remain idle + * before it is reused. + * The connection should not be used again if this time elapses. + */ + void setIdleDuration(long duration, TimeUnit unit); } // interface ManagedClientConnection diff --git a/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java b/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java index 70960f334..868d6675d 100644 --- a/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java +++ b/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java @@ -298,6 +298,7 @@ public class DefaultClientRequestDirector int execCount = 0; + boolean reuse = false; HttpResponse response = null; boolean done = false; try { @@ -445,11 +446,19 @@ public class DefaultClientRequestDirector response.setParams(params); requestExec.postProcess(response, httpProcessor, context); + + // The connection is in or can be brought to a re-usable state. + reuse = reuseStrategy.keepAlive(response, context); + if(reuse) { + // Set the idle duration of this connection + long duration = keepAliveStrategy.getKeepAliveDuration(response, context); + managedConn.setIdleDuration(duration, TimeUnit.MILLISECONDS); + } + RoutedRequest followup = handleResponse(roureq, response, context); if (followup == null) { done = true; } else { - boolean reuse = reuseStrategy.keepAlive(response, context); if (reuse) { LOG.debug("Connection kept alive"); // Make sure the response body is fully consumed, if present @@ -465,7 +474,7 @@ public class DefaultClientRequestDirector } // check if we can use the same connection for the followup if (!followup.getRoute().equals(roureq.getRoute())) { - releaseConnection(response, context); + releaseConnection(); } roureq = followup; } @@ -477,8 +486,6 @@ public class DefaultClientRequestDirector } } // while not done - // The connection is in or can be brought to a re-usable state. - boolean reuse = reuseStrategy.keepAlive(response, context); // check for entity, release connection if possible if ((response == null) || (response.getEntity() == null) || @@ -486,13 +493,11 @@ public class DefaultClientRequestDirector // connection not needed and (assumed to be) in re-usable state if (reuse) managedConn.markReusable(); - releaseConnection(response, context); + releaseConnection(); } else { // install an auto-release entity HttpEntity entity = response.getEntity(); - long duration = keepAliveStrategy.getKeepAliveDuration(response, context); - TimeUnit unit = keepAliveStrategy.getTimeUnit(); - entity = new BasicManagedEntity(entity, managedConn, reuse, duration, unit); + entity = new BasicManagedEntity(entity, managedConn, reuse); response.setEntity(entity); } @@ -515,10 +520,15 @@ public class DefaultClientRequestDirector * and prepares for retrieving a new connection during * the next request. */ - protected void releaseConnection(HttpResponse response, HttpContext context) { - long duration = keepAliveStrategy.getKeepAliveDuration(response, context); - TimeUnit unit = keepAliveStrategy.getTimeUnit(); - connManager.releaseConnection(managedConn, duration, unit); + protected void releaseConnection() { + // Release the connection through the ManagedConnection instead of the + // ConnectionManager directly. This lets the connection control how + // it is released. + try { + managedConn.releaseConnection(); + } catch(IOException ignored) { + LOG.debug("IOException releasing connection", ignored); + } managedConn = null; } @@ -980,10 +990,8 @@ public class DefaultClientRequestDirector * Shuts down the connection. * This method is called from a catch block in * {@link #execute execute} during exception handling. - * - * @throws IOException in case of an IO problem */ - private void abortConnection() throws IOException { + private void abortConnection() { ManagedClientConnection mcc = managedConn; if (mcc != null) { // we got here as the result of an exception @@ -997,7 +1005,11 @@ public class DefaultClientRequestDirector } } // ensure the connection manager properly releases this connection - connManager.releaseConnection(mcc, -1, null); + try { + mcc.releaseConnection(); + } catch(IOException ignored) { + LOG.debug("Error releasing connection", ignored); + } } } // abortConnection diff --git a/module-client/src/main/java/org/apache/http/impl/client/DefaultConnectionKeepAliveStrategy.java b/module-client/src/main/java/org/apache/http/impl/client/DefaultConnectionKeepAliveStrategy.java index 2c2cf3841..c7641d210 100644 --- a/module-client/src/main/java/org/apache/http/impl/client/DefaultConnectionKeepAliveStrategy.java +++ b/module-client/src/main/java/org/apache/http/impl/client/DefaultConnectionKeepAliveStrategy.java @@ -30,8 +30,6 @@ */ package org.apache.http.impl.client; -import java.util.concurrent.TimeUnit; - import org.apache.http.HeaderElement; import org.apache.http.HeaderElementIterator; import org.apache.http.HttpResponse; @@ -67,16 +65,12 @@ public class DefaultConnectionKeepAliveStrategy implements ConnectionKeepAliveSt String value = he.getValue(); if (value != null && param.equalsIgnoreCase("timeout")) { try { - return Long.parseLong(value); + return Long.parseLong(value) * 1000; } catch(NumberFormatException ignore) { } } } return -1; } - - public TimeUnit getTimeUnit() { - return TimeUnit.SECONDS; - } } diff --git a/module-client/src/main/java/org/apache/http/impl/conn/AbstractClientConnAdapter.java b/module-client/src/main/java/org/apache/http/impl/conn/AbstractClientConnAdapter.java index a498135b8..ae0099c00 100644 --- a/module-client/src/main/java/org/apache/http/impl/conn/AbstractClientConnAdapter.java +++ b/module-client/src/main/java/org/apache/http/impl/conn/AbstractClientConnAdapter.java @@ -98,6 +98,12 @@ public abstract class AbstractClientConnAdapter /** True if the connection has been aborted. */ private volatile boolean aborted; + + /** The last time this processed incoming headers. */ + private volatile long lastHeadersRead; + + /** The duration this is valid for while idle (in ms). */ + private volatile long duration; /** * Creates a new connection adapter. @@ -115,7 +121,7 @@ public abstract class AbstractClientConnAdapter wrappedConnection = conn; markedReusable = false; aborted = false; - + duration = Long.MAX_VALUE; } // @@ -126,6 +132,7 @@ public abstract class AbstractClientConnAdapter protected void detach() { wrappedConnection = null; connManager = null; // base class attribute + duration = Long.MAX_VALUE; } protected OperatedClientConnection getWrappedConnection() { @@ -252,6 +259,8 @@ public abstract class AbstractClientConnAdapter assertValid(conn); unmarkReusable(); + duration = Long.MAX_VALUE; // Reset duration per request. + lastHeadersRead = System.currentTimeMillis(); return conn.receiveResponseHeader(); } @@ -347,11 +356,45 @@ public abstract class AbstractClientConnAdapter public boolean isMarkedReusable() { return markedReusable; } + + public void setIdleDuration(long duration, TimeUnit unit) { + if(duration > 0) { + this.duration = unit.toMillis(duration); + } else { + this.duration = Long.MAX_VALUE; + } + } + + /** + * Returns the amount of time remaining that this connection + * can be reused. If negative, the connection should not + * be reused. If Long.MAX_VALUE, there is no suggested + * duration. + * + * The remaining time is the elapsed time between now, + * the time the last headers were processed, and the duration + * given from {@link #setIdleDuration(long, TimeUnit)}. + */ + protected long getRemainingIdleDuration() { + if(duration == Long.MAX_VALUE) { + return Long.MAX_VALUE; + } else { + long elapsedAlready = System.currentTimeMillis() - lastHeadersRead; + return duration - elapsedAlready; + } + } // non-javadoc, see interface ConnectionReleaseTrigger - public void releaseConnection(long validDuration, TimeUnit timeUnit) { - if (connManager != null) - connManager.releaseConnection(this, validDuration, timeUnit); + public void releaseConnection() { + if (connManager != null) { + long remainingTime = getRemainingIdleDuration(); + if(remainingTime <= 0) { + unmarkReusable(); + } else if(remainingTime == Long.MAX_VALUE) { + remainingTime = -1; + } + connManager.releaseConnection(this, remainingTime, TimeUnit.MILLISECONDS); + } } // non-javadoc, see interface ConnectionReleaseTrigger @@ -379,7 +422,7 @@ public abstract class AbstractClientConnAdapter // manager if #abortConnection() is called from the main execution // thread while there is no blocking I/O operation. if (executionThread.equals(Thread.currentThread())) { - releaseConnection(-1, null); + releaseConnection(); } } diff --git a/module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java b/module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java index 32812faaa..f6f45c70f 100644 --- a/module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java +++ b/module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java @@ -379,7 +379,7 @@ public class TestDefaultClientRequestDirector extends ServerTestBase { if(!awaitLatch.await(timeout, tunit)) throw new ConnectionPoolTimeoutException(); - return new ClientConnAdapterMockup(); + return new ClientConnAdapterMockup(ConnMan4.this); } }; } else { @@ -452,7 +452,7 @@ public class TestDefaultClientRequestDirector extends ServerTestBase { long timeout, TimeUnit unit) throws InterruptedException, ConnectionPoolTimeoutException { - allocatedConnection = new ClientConnAdapterMockup() { + allocatedConnection = new ClientConnAdapterMockup(ConnMan2.this) { @Override public void open(HttpRoute route, HttpContext context, HttpParams params) throws IOException { @@ -535,7 +535,7 @@ public class TestDefaultClientRequestDirector extends ServerTestBase { if(!awaitLatch.await(timeout, tunit)) throw new ConnectionPoolTimeoutException(); - return new ClientConnAdapterMockup(); + return new ClientConnAdapterMockup(ConMan.this); } }; } diff --git a/module-client/src/test/java/org/apache/http/impl/client/TestDefaultConnKeepAliveStrategy.java b/module-client/src/test/java/org/apache/http/impl/client/TestDefaultConnKeepAliveStrategy.java index 49a74fbf0..1119fe20c 100644 --- a/module-client/src/test/java/org/apache/http/impl/client/TestDefaultConnKeepAliveStrategy.java +++ b/module-client/src/test/java/org/apache/http/impl/client/TestDefaultConnKeepAliveStrategy.java @@ -1,7 +1,7 @@ /* - * $HeadURL: $ - * $Revision: $ - * $Date: $ + * $HeadURL$ + * $Revision$ + * $Date$ * ==================================================================== * * Licensed to the Apache Software Foundation (ASF) under one or more @@ -115,7 +115,7 @@ public class TestDefaultConnKeepAliveStrategy extends TestCase { response.addHeader("Keep-Alive", "timeout=300, max=20"); ConnectionKeepAliveStrategy keepAliveStrat = new DefaultConnectionKeepAliveStrategy(); long d = keepAliveStrat.getKeepAliveDuration(response, context); - assertEquals(300, d); + assertEquals(300000, d); } } diff --git a/module-client/src/test/java/org/apache/http/impl/conn/ClientConnAdapterMockup.java b/module-client/src/test/java/org/apache/http/impl/conn/ClientConnAdapterMockup.java index 4545e120d..377e50437 100644 --- a/module-client/src/test/java/org/apache/http/impl/conn/ClientConnAdapterMockup.java +++ b/module-client/src/test/java/org/apache/http/impl/conn/ClientConnAdapterMockup.java @@ -33,6 +33,7 @@ package org.apache.http.impl.conn; import java.io.IOException; import org.apache.http.HttpHost; +import org.apache.http.conn.ClientConnectionManager; import org.apache.http.conn.routing.HttpRoute; import org.apache.http.params.HttpParams; import org.apache.http.protocol.HttpContext; @@ -43,8 +44,8 @@ import org.apache.http.protocol.HttpContext; */ public class ClientConnAdapterMockup extends AbstractClientConnAdapter { - public ClientConnAdapterMockup() { - super(null, null); + public ClientConnAdapterMockup(ClientConnectionManager mgr) { + super(mgr, null); } public void close() { diff --git a/module-client/src/test/java/org/apache/http/impl/conn/TestTSCCMNoServer.java b/module-client/src/test/java/org/apache/http/impl/conn/TestTSCCMNoServer.java index 02c24b720..3690368de 100644 --- a/module-client/src/test/java/org/apache/http/impl/conn/TestTSCCMNoServer.java +++ b/module-client/src/test/java/org/apache/http/impl/conn/TestTSCCMNoServer.java @@ -309,7 +309,7 @@ public class TestTSCCMNoServer extends TestCase { // expected } try { - mgr.releaseConnection(new ClientConnAdapterMockup(), -1, null); + mgr.releaseConnection(new ClientConnAdapterMockup(null), -1, null); fail("foreign connection adapter not detected"); } catch (IllegalArgumentException iax) { // expected diff --git a/module-client/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java b/module-client/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java index 81d6d3621..26b0030a5 100644 --- a/module-client/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java +++ b/module-client/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java @@ -654,7 +654,7 @@ public class TestTSCCMWithServer extends ServerTestBase { } // return it back to the manager - ((AbstractClientConnAdapter) conn).releaseConnection(-1, null); + ((AbstractClientConnAdapter) conn).releaseConnection(); // the connection is expected to be released back to the manager ManagedClientConnection conn2 = getConnection(mgr, route, 5L, TimeUnit.SECONDS); @@ -720,7 +720,7 @@ public class TestTSCCMWithServer extends ServerTestBase { } // return it back to the manager - ((AbstractClientConnAdapter) conn).releaseConnection(-1, null); + ((AbstractClientConnAdapter) conn).releaseConnection(); // the connection is expected to be released back to the manager ManagedClientConnection conn2 = getConnection(mgr, route, 5L, TimeUnit.SECONDS); @@ -791,7 +791,7 @@ public class TestTSCCMWithServer extends ServerTestBase { } // return it back to the manager - ((AbstractClientConnAdapter) conn).releaseConnection(-1, null); + ((AbstractClientConnAdapter) conn).releaseConnection(); // the connection is expected to be released back to the manager ManagedClientConnection conn2 = getConnection(mgr, route, 5L, TimeUnit.SECONDS); @@ -869,7 +869,7 @@ public class TestTSCCMWithServer extends ServerTestBase { } // return it back to the manager - ((AbstractClientConnAdapter) conn).releaseConnection(-1, null); + ((AbstractClientConnAdapter) conn).releaseConnection(); // the connection is expected to be released back to the manager ManagedClientConnection conn2 = getConnection(mgr, route, 5L, TimeUnit.SECONDS);