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

@@ -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 <olegk at apache.org>
+
 * [HTTPCLIENT-424] Preemptive authentication no longer limited to BASIC
   scheme only. HttpClient can be customized to authenticate preemptively
   with DIGEST scheme.

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");
 

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.
  *
- * <p>
- * This interface and it's implementations replace the
- * <code>HttpMethodDirector</code> in HttpClient 3.
- * </p>
- *
  * @author <a href="mailto:rolandw at apache.org">Roland Weber</a>
  *
  *

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
-     * <br/><i @@@>timeout exceptions?</i>
      */
     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
-     * <br/><i @@@>timeout exceptions?</i>
      */
     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
-     * <br/><i @@@>timeout exceptions?</i>
      */
     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
-     * <br/><i @@@>timeout exceptions?</i>
      */
     HttpResponse execute(HttpHost target, HttpRequest request,
                          HttpContext context)

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
+    public synchronized final HttpRequestExecutor getRequestExecutor() {
-    // derived classes may offer that method at their own risk
+        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);
+        return 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;
-
     } // 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,

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,15 +347,9 @@ public class DefaultClientRequestDirector
                     break;
                 }
 
-                if (HttpConnectionParams.isStaleCheckingEnabled(params)) {
+                // Clear autogenerated headers if case the request is being
-                    // validate connection
+                // retried
-                    LOG.debug("Stale connection check");
+                wrapper.clearHeaders();
-                    if (managedConn.isStale()) {
-                        LOG.debug("Stale connection detected");
-                        managedConn.close();
-                        continue;
-                    }
-                }
                 
                 // 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;
                 }
                 

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();

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);
+    }
+    
 }

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() {