diff --git a/httpclient/src/main/java/org/apache/http/client/HttpRequestRetryHandler.java b/httpclient/src/main/java/org/apache/http/client/HttpRequestRetryHandler.java index bf0403eeb..5c91c8fba 100644 --- a/httpclient/src/main/java/org/apache/http/client/HttpRequestRetryHandler.java +++ b/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); } diff --git a/httpclient/src/main/java/org/apache/http/impl/client/DefaultHttpRequestRetryHandler.java b/httpclient/src/main/java/org/apache/http/impl/client/DefaultHttpRequestRetryHandler.java index aecbbf436..799df2efc 100644 --- a/httpclient/src/main/java/org/apache/http/impl/client/DefaultHttpRequestRetryHandler.java +++ b/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 boolean requestSentRetryEnabled; - + private final Map idempotentMethods; private final Set> nonRetriableClasses; /** @@ -79,7 +78,13 @@ public class DefaultHttpRequestRetryHandler implements HttpRequestRetryHandler { final Collection> 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 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(context, "HTTP context"); + Args.notNull(request, "HTTP request"); + 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(); } } diff --git a/httpclient/src/main/java/org/apache/http/impl/client/StandardHttpRequestRetryHandler.java b/httpclient/src/main/java/org/apache/http/impl/client/StandardHttpRequestRetryHandler.java deleted file mode 100644 index de5fcad07..000000000 --- a/httpclient/src/main/java/org/apache/http/impl/client/StandardHttpRequestRetryHandler.java +++ /dev/null @@ -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 - * . - * - */ - -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. - *

- * According to RFC-2616 section 9.1.2 the idempotent HTTP methods are: - * GET, HEAD, PUT, DELETE, OPTIONS, and TRACE - *

- * - * @since 4.2 - */ -@Immutable -public class StandardHttpRequestRetryHandler extends DefaultHttpRequestRetryHandler { - - private final Map 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(); - } - -} diff --git a/httpclient/src/main/java/org/apache/http/impl/execchain/RetryExec.java b/httpclient/src/main/java/org/apache/http/impl/execchain/RetryExec.java index cade16405..01cb1ce0c 100644 --- a/httpclient/src/main/java/org/apache/http/impl/execchain/RetryExec.java +++ b/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 " diff --git a/httpclient/src/test/java/org/apache/http/impl/client/TestDefaultHttpRequestRetryHandler.java b/httpclient/src/test/java/org/apache/http/impl/client/TestDefaultHttpRequestRetryHandler.java index baec3be13..777a610cf 100644 --- a/httpclient/src/test/java/org/apache/http/impl/client/TestDefaultHttpRequestRetryHandler.java +++ b/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 = mock(HttpUriRequest.class); + final HttpUriRequest request = new HttpGet("/"); final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler(); Assert.assertEquals(3, retryHandler.getRetryCount()); - when(request.isAborted()).thenReturn(Boolean.FALSE); - when(context.getAttribute(HttpCoreContext.HTTP_REQUEST)).thenReturn(request); - - Assert.assertFalse(retryHandler.retryRequest(new ConnectTimeoutException(), 1, context)); + Assert.assertFalse(retryHandler.retryRequest(request, new ConnectTimeoutException(), 1, null)); } @Test public void noRetryOnUnknownHost() throws Exception { - final HttpContext context = mock(HttpContext.class); - final HttpUriRequest request = mock(HttpUriRequest.class); + final HttpUriRequest request = new HttpGet("/"); final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler(); - when(request.isAborted()).thenReturn(Boolean.FALSE); - when(context.getAttribute(HttpCoreContext.HTTP_REQUEST)).thenReturn(request); - - Assert.assertFalse(retryHandler.retryRequest(new UnknownHostException(), 1, context)); + Assert.assertFalse(retryHandler.retryRequest(request, new UnknownHostException(), 1, null)); } @Test public void noRetryOnAbortedRequests() throws Exception{ - final HttpContext context = mock(HttpContext.class); - final HttpUriRequest request = mock(HttpUriRequest.class); + final HttpUriRequest request = new HttpGet("/"); + request.abort(); final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler(); - when(request.isAborted()).thenReturn(Boolean.TRUE); - when(context.getAttribute(HttpCoreContext.HTTP_REQUEST)).thenReturn(request); - - Assert.assertFalse(retryHandler.retryRequest(new IOException(),3,context)); + Assert.assertFalse(retryHandler.retryRequest(request, new IOException(), 3, null)); } @Test public void retryOnNonAbortedRequests() throws Exception{ - - final HttpContext context = mock(HttpContext.class); - final HttpUriRequest request = mock(HttpUriRequest.class); + final HttpUriRequest request = new HttpGet("/"); final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler(); - when(request.isAborted()).thenReturn(Boolean.FALSE); - when(context.getAttribute(HttpCoreContext.HTTP_REQUEST)).thenReturn(request); - - Assert.assertTrue(retryHandler.retryRequest(new IOException(),3,context)); + Assert.assertTrue(retryHandler.retryRequest(request, new IOException(), 3, null)); } @Test public void noRetryOnConnectionTimeout() throws Exception{ - - final HttpContext context = mock(HttpContext.class); - final HttpUriRequest request = mock(HttpUriRequest.class); + final HttpUriRequest request = new HttpGet("/"); final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler(); - when(request.isAborted()).thenReturn(false); - when(context.getAttribute(HttpCoreContext.HTTP_REQUEST)).thenReturn(request); - - Assert.assertFalse(retryHandler.retryRequest(new ConnectTimeoutException(),3,context)); + Assert.assertFalse(retryHandler.retryRequest(request, new ConnectTimeoutException(), 3, null)); } } diff --git a/httpclient/src/test/java/org/apache/http/impl/client/integration/TestClientRequestExecution.java b/httpclient/src/test/java/org/apache/http/impl/client/integration/TestClientRequestExecution.java index 640b015d7..a41a89d36 100644 --- a/httpclient/src/test/java/org/apache/http/impl/client/integration/TestClientRequestExecution.java +++ b/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) { diff --git a/httpclient/src/test/java/org/apache/http/impl/execchain/TestRetryExec.java b/httpclient/src/test/java/org/apache/http/impl/execchain/TestRetryExec.java index e0cb01659..cfa8833dd 100644 --- a/httpclient/src/test/java/org/apache/http/impl/execchain/TestRetryExec.java +++ b/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.any(), Mockito.any(), Mockito.eq(1), Mockito.any())).thenReturn(Boolean.TRUE); @@ -139,6 +141,7 @@ public class TestRetryExec { Mockito.same(context), Mockito.same(execAware)); Mockito.verify(retryHandler, Mockito.never()).retryRequest( + Mockito.any(), Mockito.any(), Mockito.anyInt(), Mockito.any()); @@ -173,6 +176,7 @@ public class TestRetryExec { }); Mockito.when(retryHandler.retryRequest( + Mockito.any(), Mockito.any(), Mockito.eq(1), Mockito.any())).thenReturn(Boolean.TRUE);