RFC 7231: automatic retrial of idempotent methods

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

httpclient/src/main/java/org/apache/http/client/HttpRequestRetryHandler.java

@@ -29,6 +29,7 @@ package org.apache.http.client;
 
 import java.io.IOException;
 
+import org.apache.http.HttpRequest;
 import org.apache.http.protocol.HttpContext;
 
 /**
@@ -47,6 +48,7 @@ public interface HttpRequestRetryHandler {
      * Determines if a method should be retried after an IOException
      * occurs during execution.
      *
+     * @param request request failed die to an I/O exception.
      * @param exception the exception that occurred
      * @param executionCount the number of times this method has been
      * unsuccessfully executed
@@ -55,6 +57,6 @@ public interface HttpRequestRetryHandler {
      * @return {@code true} if the method should be retried, {@code false}
      * otherwise
      */
-    boolean retryRequest(IOException exception, int executionCount, HttpContext context);
+    boolean retryRequest(HttpRequest request, IOException exception, int executionCount, HttpContext context);
 
 }

httpclient/src/main/java/org/apache/http/impl/client/DefaultHttpRequestRetryHandler.java

@@ -34,16 +34,17 @@ import java.net.UnknownHostException;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashSet;
+import java.util.Locale;
+import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import javax.net.ssl.SSLException;
 
-import org.apache.http.HttpEntityEnclosingRequest;
 import org.apache.http.HttpRequest;
 import org.apache.http.annotation.Immutable;
 import org.apache.http.client.HttpRequestRetryHandler;
 import org.apache.http.client.methods.HttpUriRequest;
-import org.apache.http.client.protocol.HttpClientContext;
 import org.apache.http.protocol.HttpContext;
 import org.apache.http.util.Args;
 
@@ -60,9 +61,7 @@ public class DefaultHttpRequestRetryHandler implements HttpRequestRetryHandler {
     /** the number of times a method will be retried */
     private final int retryCount;
 
-    /** Whether or not methods that have successfully sent their request will be retried */
+    private final Map<String, Boolean> idempotentMethods;
-    private final boolean requestSentRetryEnabled;
-
     private final Set<Class<? extends IOException>> nonRetriableClasses;
 
     /**
@@ -79,7 +78,13 @@ public class DefaultHttpRequestRetryHandler implements HttpRequestRetryHandler {
             final Collection<Class<? extends IOException>> clazzes) {
         super();
         this.retryCount = retryCount;
-        this.requestSentRetryEnabled = requestSentRetryEnabled;
+        this.idempotentMethods = new ConcurrentHashMap<>();
+        this.idempotentMethods.put("GET", Boolean.TRUE);
+        this.idempotentMethods.put("HEAD", Boolean.TRUE);
+        this.idempotentMethods.put("PUT", Boolean.TRUE);
+        this.idempotentMethods.put("DELETE", Boolean.TRUE);
+        this.idempotentMethods.put("OPTIONS", Boolean.TRUE);
+        this.idempotentMethods.put("TRACE", Boolean.TRUE);
         this.nonRetriableClasses = new HashSet<>();
         for (final Class<? extends IOException> clazz: clazzes) {
             this.nonRetriableClasses.add(clazz);
@@ -120,17 +125,20 @@ public class DefaultHttpRequestRetryHandler implements HttpRequestRetryHandler {
     public DefaultHttpRequestRetryHandler() {
         this(3, false);
     }
+
     /**
      * Used {@code retryCount} and {@code requestSentRetryEnabled} to determine
      * if the given method should be retried.
      */
     @Override
     public boolean retryRequest(
+            final HttpRequest request,
             final IOException exception,
             final int executionCount,
             final HttpContext context) {
-        Args.notNull(exception, "Exception parameter");
+        Args.notNull(request, "HTTP request");
-        Args.notNull(context, "HTTP context");
+        Args.notNull(exception, "I/O exception");
+
         if (executionCount > this.retryCount) {
             // Do not retry if over max retry count
             return false;
@@ -144,35 +152,17 @@ public class DefaultHttpRequestRetryHandler implements HttpRequestRetryHandler {
                 }
             }
         }
-        final HttpClientContext clientContext = HttpClientContext.adapt(context);
-        final HttpRequest request = clientContext.getRequest();
-
         if (request instanceof HttpUriRequest && ((HttpUriRequest)request).isAborted()) {
             return false;
         }
-
         if (handleAsIdempotent(request)) {
             // Retry if the request is considered idempotent
             return true;
         }
-
-        if (!clientContext.isRequestSent() || this.requestSentRetryEnabled) {
-            // Retry if the request has not been sent fully or
-            // if it's OK to retry methods that have been sent
-            return true;
-        }
         // otherwise do not retry
         return false;
     }
 
-    /**
-     * @return {@code true} if this handler will retry methods that have
-     * successfully sent their request, {@code false} otherwise
-     */
-    public boolean isRequestSentRetryEnabled() {
-        return requestSentRetryEnabled;
-    }
-
     /**
      * @return the maximum number of times a method will be retried
      */
@@ -184,7 +174,9 @@ public class DefaultHttpRequestRetryHandler implements HttpRequestRetryHandler {
      * @since 4.2
      */
     protected boolean handleAsIdempotent(final HttpRequest request) {
-        return !(request instanceof HttpEntityEnclosingRequest);
+        final String method = request.getRequestLine().getMethod().toUpperCase(Locale.ROOT);
+        final Boolean b = this.idempotentMethods.get(method);
+        return b != null && b.booleanValue();
     }
 
 }

httpclient/src/main/java/org/apache/http/impl/client/StandardHttpRequestRetryHandler.java

@@ -1,81 +0,0 @@
-/*
- * ====================================================================
- * 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.client;
-
-import java.util.Locale;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-
-import org.apache.http.HttpRequest;
-import org.apache.http.annotation.Immutable;
-
-/**
- * {@link org.apache.http.client.HttpRequestRetryHandler} which assumes
- * that all requested HTTP methods which should be idempotent according
- * to RFC-2616 are in fact idempotent and can be retried.
- * <p>
- * According to RFC-2616 section 9.1.2 the idempotent HTTP methods are:
- * GET, HEAD, PUT, DELETE, OPTIONS, and TRACE
- * </p>
- *
- * @since 4.2
- */
-@Immutable
-public class StandardHttpRequestRetryHandler extends DefaultHttpRequestRetryHandler {
-
-    private final Map<String, Boolean> idempotentMethods;
-
-    /**
-     * Default constructor
-     */
-    public StandardHttpRequestRetryHandler(final int retryCount, final boolean requestSentRetryEnabled) {
-        super(retryCount, requestSentRetryEnabled);
-        this.idempotentMethods = new ConcurrentHashMap<>();
-        this.idempotentMethods.put("GET", Boolean.TRUE);
-        this.idempotentMethods.put("HEAD", Boolean.TRUE);
-        this.idempotentMethods.put("PUT", Boolean.TRUE);
-        this.idempotentMethods.put("DELETE", Boolean.TRUE);
-        this.idempotentMethods.put("OPTIONS", Boolean.TRUE);
-        this.idempotentMethods.put("TRACE", Boolean.TRUE);
-    }
-
-    /**
-     * Default constructor
-     */
-    public StandardHttpRequestRetryHandler() {
-        this(3, false);
-    }
-
-    @Override
-    protected boolean handleAsIdempotent(final HttpRequest request) {
-        final String method = request.getRequestLine().getMethod().toUpperCase(Locale.ROOT);
-        final Boolean b = this.idempotentMethods.get(method);
-        return b != null && b.booleanValue();
-    }
-
-}

httpclient/src/main/java/org/apache/http/impl/execchain/RetryExec.java

@@ -91,7 +91,7 @@ public class RetryExec implements ClientExecChain {
                     this.log.debug("Request has been aborted");
                     throw ex;
                 }
-                if (retryHandler.retryRequest(ex, execCount, context)) {
+                if (retryHandler.retryRequest(request, ex, execCount, context)) {
                     if (this.log.isInfoEnabled()) {
                         this.log.info("I/O exception ("+ ex.getClass().getName() +
                                 ") caught when processing request to "

httpclient/src/test/java/org/apache/http/impl/client/TestDefaultHttpRequestRetryHandler.java

@@ -26,16 +26,12 @@
  */
 package org.apache.http.impl.client;
 
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
-
 import java.io.IOException;
 import java.net.UnknownHostException;
 
+import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpUriRequest;
 import org.apache.http.conn.ConnectTimeoutException;
-import org.apache.http.protocol.HttpContext;
-import org.apache.http.protocol.HttpCoreContext;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -45,70 +41,49 @@ public class TestDefaultHttpRequestRetryHandler {
 
     @Test
     public void noRetryOnConnectTimeout() throws Exception {
-        final HttpContext context = mock(HttpContext.class);
+        final HttpUriRequest request = new HttpGet("/");
-        final HttpUriRequest request = mock(HttpUriRequest.class);
 
         final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler();
         Assert.assertEquals(3, retryHandler.getRetryCount());
 
-        when(request.isAborted()).thenReturn(Boolean.FALSE);
+        Assert.assertFalse(retryHandler.retryRequest(request, new ConnectTimeoutException(), 1, null));
-        when(context.getAttribute(HttpCoreContext.HTTP_REQUEST)).thenReturn(request);
-
-        Assert.assertFalse(retryHandler.retryRequest(new ConnectTimeoutException(), 1, context));
     }
 
     @Test
     public void noRetryOnUnknownHost() throws Exception {
-        final HttpContext context = mock(HttpContext.class);
+        final HttpUriRequest request = new HttpGet("/");
-        final HttpUriRequest request = mock(HttpUriRequest.class);
 
         final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler();
 
-        when(request.isAborted()).thenReturn(Boolean.FALSE);
+        Assert.assertFalse(retryHandler.retryRequest(request, new UnknownHostException(), 1, null));
-        when(context.getAttribute(HttpCoreContext.HTTP_REQUEST)).thenReturn(request);
-
-        Assert.assertFalse(retryHandler.retryRequest(new UnknownHostException(), 1, context));
     }
 
     @Test
     public void noRetryOnAbortedRequests() throws Exception{
-        final HttpContext context = mock(HttpContext.class);
+        final HttpUriRequest request = new HttpGet("/");
-        final HttpUriRequest request = mock(HttpUriRequest.class);
+        request.abort();
 
         final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler();
 
-        when(request.isAborted()).thenReturn(Boolean.TRUE);
+        Assert.assertFalse(retryHandler.retryRequest(request, new IOException(), 3, null));
-        when(context.getAttribute(HttpCoreContext.HTTP_REQUEST)).thenReturn(request);
-
-        Assert.assertFalse(retryHandler.retryRequest(new IOException(),3,context));
     }
 
     @Test
     public void retryOnNonAbortedRequests() throws Exception{
-
+        final HttpUriRequest request = new HttpGet("/");
-        final HttpContext context = mock(HttpContext.class);
-        final HttpUriRequest request = mock(HttpUriRequest.class);
 
         final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler();
 
-        when(request.isAborted()).thenReturn(Boolean.FALSE);
+        Assert.assertTrue(retryHandler.retryRequest(request, new IOException(), 3, null));
-        when(context.getAttribute(HttpCoreContext.HTTP_REQUEST)).thenReturn(request);
-
-        Assert.assertTrue(retryHandler.retryRequest(new IOException(),3,context));
     }
 
     @Test
     public void noRetryOnConnectionTimeout() throws Exception{
-
+        final HttpUriRequest request = new HttpGet("/");
-        final HttpContext context = mock(HttpContext.class);
-        final HttpUriRequest request = mock(HttpUriRequest.class);
 
         final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler();
 
-        when(request.isAborted()).thenReturn(false);
+        Assert.assertFalse(retryHandler.retryRequest(request, new ConnectTimeoutException(), 3, null));
-        when(context.getAttribute(HttpCoreContext.HTTP_REQUEST)).thenReturn(request);
-
-        Assert.assertFalse(retryHandler.retryRequest(new ConnectTimeoutException(),3,context));
     }
 
 }

httpclient/src/test/java/org/apache/http/impl/client/integration/TestClientRequestExecution.java

@@ -126,6 +126,7 @@ public class TestClientRequestExecution extends LocalServerTestBase {
 
             @Override
             public boolean retryRequest(
+                    final HttpRequest request,
                     final IOException exception,
                     final int executionCount,
                     final HttpContext context) {
@@ -166,6 +167,7 @@ public class TestClientRequestExecution extends LocalServerTestBase {
 
             @Override
             public boolean retryRequest(
+                    final HttpRequest request,
                     final IOException exception,
                     final int executionCount,
                     final HttpContext context) {

httpclient/src/test/java/org/apache/http/impl/execchain/TestRetryExec.java

@@ -29,6 +29,7 @@ package org.apache.http.impl.execchain;
 import org.apache.http.Header;
 import org.apache.http.HttpEntityEnclosingRequest;
 import org.apache.http.HttpHost;
+import org.apache.http.HttpRequest;
 import org.apache.http.client.HttpRequestRetryHandler;
 import org.apache.http.client.NonRepeatableRequestException;
 import org.apache.http.client.entity.EntityBuilder;
@@ -101,6 +102,7 @@ public class TestRetryExec {
 
         });
         Mockito.when(retryHandler.retryRequest(
+                Mockito.<HttpRequest>any(),
                 Mockito.<IOException>any(),
                 Mockito.eq(1),
                 Mockito.<HttpContext>any())).thenReturn(Boolean.TRUE);
@@ -139,6 +141,7 @@ public class TestRetryExec {
                     Mockito.same(context),
                     Mockito.same(execAware));
             Mockito.verify(retryHandler, Mockito.never()).retryRequest(
+                    Mockito.<HttpRequest>any(),
                     Mockito.<IOException>any(),
                     Mockito.anyInt(),
                     Mockito.<HttpContext>any());
@@ -173,6 +176,7 @@ public class TestRetryExec {
 
         });
         Mockito.when(retryHandler.retryRequest(
+                Mockito.<HttpRequest>any(),
                 Mockito.<IOException>any(),
                 Mockito.eq(1),
                 Mockito.<HttpContext>any())).thenReturn(Boolean.TRUE);