From 650a04c7341d65d88982dc734b0780620cdce27f Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Thu, 29 May 2008 19:41:00 +0000 Subject: [PATCH] Fixed request re-generation logic when retrying a failed request. Auto-generated headers will no accumulate. git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@661444 13f79535-47bb-0310-9956-ffa450edef68 --- RELEASE_NOTES.txt | 4 + .../http/examples/client/ClientFormLogin.java | 4 - .../http/client/ClientRequestDirector.java | 5 -- .../org/apache/http/client/HttpClient.java | 4 - .../http/impl/client/AbstractHttpClient.java | 29 ++++--- .../client/DefaultClientRequestDirector.java | 36 +++++--- .../http/impl/client/DefaultHttpClient.java | 7 ++ .../TestDefaultClientRequestDirector.java | 85 +++++++++++++++++++ .../impl/conn/ClientConnAdapterMockup.java | 1 - 9 files changed, 135 insertions(+), 40 deletions(-) diff --git a/RELEASE_NOTES.txt b/RELEASE_NOTES.txt index ff4b7c227..f91fc5104 100644 --- a/RELEASE_NOTES.txt +++ b/RELEASE_NOTES.txt @@ -1,6 +1,10 @@ Changes since 4.0 Alpha 4 ------------------- +* Fixed request re-generation logic when retrying a failed request. + Auto-generated headers will no accumulate. + Contributed by Oleg Kalnichevski + * [HTTPCLIENT-424] Preemptive authentication no longer limited to BASIC scheme only. HttpClient can be customized to authenticate preemptively with DIGEST scheme. diff --git a/module-client/src/examples/org/apache/http/examples/client/ClientFormLogin.java b/module-client/src/examples/org/apache/http/examples/client/ClientFormLogin.java index 7a2c12431..a33200282 100644 --- a/module-client/src/examples/org/apache/http/examples/client/ClientFormLogin.java +++ b/module-client/src/examples/org/apache/http/examples/client/ClientFormLogin.java @@ -37,8 +37,6 @@ import org.apache.http.NameValuePair; import org.apache.http.client.entity.UrlEncodedFormEntity; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpPost; -import org.apache.http.client.params.CookiePolicy; -import org.apache.http.client.params.ClientPNames; import org.apache.http.cookie.Cookie; import org.apache.http.impl.client.DefaultHttpClient; import org.apache.http.message.BasicNameValuePair; @@ -53,8 +51,6 @@ public class ClientFormLogin { public static void main(String[] args) throws Exception { DefaultHttpClient httpclient = new DefaultHttpClient(); - httpclient.getParams().setParameter( - ClientPNames.COOKIE_POLICY, CookiePolicy.BROWSER_COMPATIBILITY); HttpGet httpget = new HttpGet("https://portal.sun.com/portal/dt"); diff --git a/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java b/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java index 1383a7cbc..60ec78d77 100644 --- a/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java +++ b/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java @@ -52,11 +52,6 @@ import org.apache.http.protocol.HttpContext; * connections for reading the response entity. Such connections * MUST be released, but that is out of the scope of a request director. * - *

- * This interface and it's implementations replace the - * HttpMethodDirector in HttpClient 3. - *

- * * @author Roland Weber * * diff --git a/module-client/src/main/java/org/apache/http/client/HttpClient.java b/module-client/src/main/java/org/apache/http/client/HttpClient.java index 3685aab20..0f09e5e41 100644 --- a/module-client/src/main/java/org/apache/http/client/HttpClient.java +++ b/module-client/src/main/java/org/apache/http/client/HttpClient.java @@ -91,7 +91,6 @@ public interface HttpClient { * @throws HttpException in case of a problem * @throws IOException in case of an IO problem * or the connection was aborted - *
timeout exceptions? */ HttpResponse execute(HttpUriRequest request) throws HttpException, IOException @@ -115,7 +114,6 @@ public interface HttpClient { * @throws HttpException in case of a problem * @throws IOException in case of an IO problem * or the connection was aborted - *
timeout exceptions? */ HttpResponse execute(HttpUriRequest request, HttpContext context) throws HttpException, IOException @@ -141,7 +139,6 @@ public interface HttpClient { * @throws HttpException in case of a problem * @throws IOException in case of an IO problem * or the connection was aborted - *
timeout exceptions? */ HttpResponse execute(HttpHost target, HttpRequest request) throws HttpException, IOException @@ -168,7 +165,6 @@ public interface HttpClient { * @throws HttpException in case of a problem * @throws IOException in case of an IO problem * or the connection was aborted - *
timeout exceptions? */ HttpResponse execute(HttpHost target, HttpRequest request, HttpContext context) diff --git a/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java b/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java index a7f0c0fcc..c86356bc8 100644 --- a/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java +++ b/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java @@ -61,6 +61,7 @@ import org.apache.http.protocol.DefaultedHttpContext; import org.apache.http.protocol.HttpContext; import org.apache.http.protocol.BasicHttpContext; import org.apache.http.protocol.HttpProcessor; +import org.apache.http.protocol.HttpRequestExecutor; /** * Convenience base class for HTTP client implementations. @@ -78,6 +79,9 @@ public abstract class AbstractHttpClient implements HttpClient { /** The parameters. */ private HttpParams defaultParams; + /** The request executor. */ + private HttpRequestExecutor requestExec; + /** The connection manager. */ private ClientConnectionManager connManager; @@ -137,6 +141,9 @@ public abstract class AbstractHttpClient implements HttpClient { protected abstract HttpContext createHttpContext(); + protected abstract HttpRequestExecutor createRequestExecutor(); + + protected abstract ClientConnectionManager createClientConnectionManager(); @@ -196,7 +203,6 @@ public abstract class AbstractHttpClient implements HttpClient { } - // non-javadoc, see interface HttpClient public synchronized final ClientConnectionManager getConnectionManager() { if (connManager == null) { connManager = createClientConnectionManager(); @@ -205,8 +211,12 @@ public abstract class AbstractHttpClient implements HttpClient { } - // no setConnectionManager(), too dangerous to replace while in use - // derived classes may offer that method at their own risk + public synchronized final HttpRequestExecutor getRequestExecutor() { + if (requestExec == null) { + requestExec = createRequestExecutor(); + } + return requestExec; + } public synchronized final AuthSchemeRegistry getAuthSchemes() { @@ -512,6 +522,7 @@ public abstract class AbstractHttpClient implements HttpClient { } // Create a director for this request director = createClientRequestDirector( + getRequestExecutor(), getConnectionManager(), getConnectionReuseStrategy(), getRoutePlanner(), @@ -524,19 +535,12 @@ public abstract class AbstractHttpClient implements HttpClient { determineParams(request)); } - HttpResponse response = director.execute(target, request, execContext); - // If the response depends on the connection, the director - // will have set up an auto-release input stream. - - //@@@ "finalize" response, to allow for buffering of entities? - //@@@ here or in director? - - return response; - + return director.execute(target, request, execContext); } // execute protected ClientRequestDirector createClientRequestDirector( + final HttpRequestExecutor requestExec, final ClientConnectionManager conman, final ConnectionReuseStrategy reustrat, final HttpRoutePlanner rouplan, @@ -548,6 +552,7 @@ public abstract class AbstractHttpClient implements HttpClient { final UserTokenHandler stateHandler, final HttpParams params) { return new DefaultClientRequestDirector( + requestExec, conman, reustrat, rouplan, 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 f40b50362..e859e7688 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 @@ -154,6 +154,7 @@ public class DefaultClientRequestDirector private final AuthState proxyAuthState; public DefaultClientRequestDirector( + final HttpRequestExecutor requestExec, final ClientConnectionManager conman, final ConnectionReuseStrategy reustrat, final HttpRoutePlanner rouplan, @@ -165,6 +166,10 @@ public class DefaultClientRequestDirector final UserTokenHandler userTokenHandler, final HttpParams params) { + if (requestExec == null) { + throw new IllegalArgumentException + ("Request executor may not be null."); + } if (conman == null) { throw new IllegalArgumentException ("Client connection manager may not be null."); @@ -205,6 +210,7 @@ public class DefaultClientRequestDirector throw new IllegalArgumentException ("HTTP parameters may not be null"); } + this.requestExec = requestExec; this.connManager = conman; this.reuseStrategy = reustrat; this.routePlanner = rouplan; @@ -215,7 +221,6 @@ public class DefaultClientRequestDirector this.proxyAuthHandler = proxyAuthHandler; this.userTokenHandler = userTokenHandler; this.params = params; - this.requestExec = new HttpRequestExecutor(); this.managedConn = null; @@ -312,6 +317,15 @@ public class DefaultClientRequestDirector iox.initCause(interrupted); throw iox; } + + if (HttpConnectionParams.isStaleCheckingEnabled(params)) { + // validate connection + LOG.debug("Stale connection check"); + if (managedConn.isStale()) { + LOG.debug("Stale connection detected"); + managedConn.close(); + } + } } if (orig instanceof AbortableHttpRequest) { @@ -333,16 +347,10 @@ public class DefaultClientRequestDirector break; } - if (HttpConnectionParams.isStaleCheckingEnabled(params)) { - // validate connection - LOG.debug("Stale connection check"); - if (managedConn.isStale()) { - LOG.debug("Stale connection detected"); - managedConn.close(); - continue; - } - } - + // Clear autogenerated headers if case the request is being + // retried + wrapper.clearHeaders(); + // Re-write request URI if needed rewriteRequestURI(wrapper, route); @@ -367,6 +375,8 @@ public class DefaultClientRequestDirector targetAuthState); context.setAttribute(ClientContext.PROXY_AUTH_STATE, proxyAuthState); + + // Run request protocol interceptors requestExec.preProcess(wrapper, httpProcessor, context); context.setAttribute(ExecutionContext.HTTP_REQUEST, @@ -397,6 +407,7 @@ public class DefaultClientRequestDirector throw ex; } + // Run response protocol interceptors response.setParams(params); requestExec.postProcess(response, httpProcessor, context); @@ -424,9 +435,6 @@ public class DefaultClientRequestDirector connManager.releaseConnection(managedConn); managedConn = null; } - // In case we are going to retry the same request, - // clear auto-generated headers - followup.getRequest().clearHeaders(); roureq = followup; } diff --git a/module-client/src/main/java/org/apache/http/impl/client/DefaultHttpClient.java b/module-client/src/main/java/org/apache/http/impl/client/DefaultHttpClient.java index 6c2238aad..bf793b74c 100644 --- a/module-client/src/main/java/org/apache/http/impl/client/DefaultHttpClient.java +++ b/module-client/src/main/java/org/apache/http/impl/client/DefaultHttpClient.java @@ -72,6 +72,7 @@ import org.apache.http.params.HttpProtocolParams; import org.apache.http.protocol.BasicHttpProcessor; import org.apache.http.protocol.HTTP; import org.apache.http.protocol.HttpContext; +import org.apache.http.protocol.HttpRequestExecutor; import org.apache.http.protocol.RequestConnControl; import org.apache.http.protocol.RequestContent; import org.apache.http.protocol.RequestExpectContinue; @@ -143,6 +144,12 @@ public class DefaultHttpClient extends AbstractHttpClient { } + @Override + protected HttpRequestExecutor createRequestExecutor() { + return new HttpRequestExecutor(); + } + + @Override protected ClientConnectionManager createClientConnectionManager() { SchemeRegistry registry = new SchemeRegistry(); 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 1c0f0af48..7be0b05fa 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 @@ -38,13 +38,17 @@ import java.util.concurrent.atomic.AtomicReference; import junit.framework.Test; import junit.framework.TestSuite; +import org.apache.http.Header; +import org.apache.http.HttpClientConnection; import org.apache.http.HttpEntity; import org.apache.http.HttpException; import org.apache.http.HttpHost; import org.apache.http.HttpRequest; +import org.apache.http.HttpRequestInterceptor; import org.apache.http.HttpResponse; import org.apache.http.HttpStatus; import org.apache.http.ProtocolVersion; +import org.apache.http.client.HttpRequestRetryHandler; import org.apache.http.client.methods.AbortableHttpRequest; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.params.ClientPNames; @@ -67,7 +71,9 @@ import org.apache.http.mockup.SocketFactoryMockup; import org.apache.http.params.BasicHttpParams; import org.apache.http.params.HttpParams; import org.apache.http.protocol.BasicHttpContext; +import org.apache.http.protocol.ExecutionContext; import org.apache.http.protocol.HttpContext; +import org.apache.http.protocol.HttpRequestExecutor; import org.apache.http.protocol.HttpRequestHandler; /** @@ -621,4 +627,83 @@ public class TestDefaultClientRequestDirector extends ServerTestBase { assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); } + private static class FaultyHttpRequestExecutor extends HttpRequestExecutor { + + private static final String MARKER = "marker"; + + @Override + public HttpResponse execute( + final HttpRequest request, + final HttpClientConnection conn, + final HttpContext context) throws IOException, HttpException { + + Object marker = context.getAttribute(MARKER); + if (marker == null) { + context.setAttribute(MARKER, Boolean.TRUE); + throw new IOException("Oppsie"); + } + return super.execute(request, conn, context); + } + + } + + private static class FaultyHttpClient extends DefaultHttpClient { + + @Override + protected HttpRequestExecutor createRequestExecutor() { + return new FaultyHttpRequestExecutor(); + } + + } + + + public void testAutoGeneratedHeaders() throws Exception { + int port = this.localServer.getServicePort(); + this.localServer.register("*", new SimpleService()); + + FaultyHttpClient client = new FaultyHttpClient(); + + client.addRequestInterceptor(new HttpRequestInterceptor() { + + public void process( + final HttpRequest request, + final HttpContext context) throws HttpException, IOException { + request.addHeader("my-header", "stuff"); + } + + }) ; + + client.setHttpRequestRetryHandler(new HttpRequestRetryHandler() { + + public boolean retryRequest( + final IOException exception, + int executionCount, + final HttpContext context) { + return true; + } + + }); + + HttpContext context = new BasicHttpContext(); + + String s = "http://localhost:" + port; + HttpGet httpget = new HttpGet(s); + + HttpResponse response = client.execute(getServerHttp(), httpget, context); + HttpEntity e = response.getEntity(); + if (e != null) { + e.consumeContent(); + } + + HttpRequest reqWrapper = (HttpRequest) context.getAttribute( + ExecutionContext.HTTP_REQUEST); + + assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); + + assertTrue(reqWrapper instanceof RequestWrapper); + Header[] myheaders = reqWrapper.getHeaders("my-header"); + assertNotNull(myheaders); + assertEquals(1, myheaders.length); + } + } 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 7c5628f51..4545e120d 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 @@ -48,7 +48,6 @@ public class ClientConnAdapterMockup extends AbstractClientConnAdapter { } public void close() { - throw new UnsupportedOperationException("just a mockup"); } public HttpRoute getRoute() {