HTTPCLIENT-2141: HttpClient to not retry requests if the retry interv…

…al exceeds the response timeout
baihongliang · Mar 28, 2021 · 73c1530 · 73c1530
1 parent bde58d6
commit 73c1530
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 2 deletions.
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpRequestRetryExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpRequestRetryExec.java
@@ -34,6 +34,7 @@
 import org.apache.hc.client5.http.classic.ExecChain;
 import org.apache.hc.client5.http.classic.ExecChain.Scope;
 import org.apache.hc.client5.http.classic.ExecChainHandler;
+import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
 import org.apache.hc.core5.annotation.Contract;
 import org.apache.hc.core5.annotation.Internal;
@@ -46,6 +47,7 @@
 import org.apache.hc.core5.http.io.support.ClassicRequestBuilder;
 import org.apache.hc.core5.util.Args;
 import org.apache.hc.core5.util.TimeValue;
+import org.apache.hc.core5.util.Timeout;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -133,9 +135,16 @@ public ClassicHttpResponse execute(
                     return response;
                 }
                 if (retryStrategy.retryRequest(response, execCount, context)) {
+                    final TimeValue nextInterval = retryStrategy.getRetryInterval(response, execCount, context);
+                    // Make sure the retry interval does not exceed the response timeout
+                    if (TimeValue.isPositive(nextInterval)) {
+                        final RequestConfig requestConfig = context.getRequestConfig();
+                        final Timeout responseTimeout = requestConfig.getResponseTimeout();
+                        if (responseTimeout != null && nextInterval.compareTo(responseTimeout) > 0) {
+                            return response;
+                        }
+                    }
                     response.close();
-                    final TimeValue nextInterval =
-                            retryStrategy.getRetryInterval(response, execCount, context);
                     if (TimeValue.isPositive(nextInterval)) {
                         try {
                             if (LOG.isDebugEnabled()) {

diff --git a/...ient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestHttpRequestRetryExec.java b/...ient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestHttpRequestRetryExec.java
@@ -36,6 +36,7 @@
 import org.apache.hc.client5.http.classic.ExecRuntime;
 import org.apache.hc.client5.http.classic.methods.HttpGet;
 import org.apache.hc.client5.http.classic.methods.HttpPost;
+import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.entity.EntityBuilder;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
 import org.apache.hc.core5.http.ClassicHttpRequest;
@@ -47,6 +48,7 @@
 import org.apache.hc.core5.http.io.support.ClassicRequestBuilder;
 import org.apache.hc.core5.http.protocol.HttpContext;
 import org.apache.hc.core5.util.TimeValue;
+import org.apache.hc.core5.util.Timeout;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
@@ -106,6 +108,70 @@ public void testFundamentals1() throws Exception {
         Mockito.verify(response, Mockito.times(1)).close();
     }
 
+    @Test
+    public void testRetryIntervalGreaterResponseTimeout() throws Exception {
+        final HttpRoute route = new HttpRoute(target);
+        final HttpGet request = new HttpGet("/test");
+        final HttpClientContext context = HttpClientContext.create();
+        context.setRequestConfig(RequestConfig.custom()
+                .setResponseTimeout(Timeout.ofSeconds(3))
+                .build());
+
+        final ClassicHttpResponse response = Mockito.mock(ClassicHttpResponse.class);
+
+        Mockito.when(chain.proceed(
+                Mockito.same(request),
+                Mockito.<ExecChain.Scope>any())).thenReturn(response);
+        Mockito.when(retryStrategy.retryRequest(
+                Mockito.<HttpResponse>any(),
+                Mockito.anyInt(),
+                Mockito.<HttpContext>any())).thenReturn(Boolean.TRUE, Boolean.FALSE);
+        Mockito.when(retryStrategy.getRetryInterval(
+                Mockito.<HttpResponse>any(),
+                Mockito.anyInt(),
+                Mockito.<HttpContext>any())).thenReturn(TimeValue.ofSeconds(5));
+
+        final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context);
+        retryExec.execute(request, scope, chain);
+
+        Mockito.verify(chain, Mockito.times(1)).proceed(
+                Mockito.<ClassicHttpRequest>any(),
+                Mockito.same(scope));
+        Mockito.verify(response, Mockito.times(0)).close();
+    }
+
+    @Test
+    public void testRetryIntervalResponseTimeoutNull() throws Exception {
+        final HttpRoute route = new HttpRoute(target);
+        final HttpGet request = new HttpGet("/test");
+        final HttpClientContext context = HttpClientContext.create();
+        context.setRequestConfig(RequestConfig.custom()
+                .setResponseTimeout(null)
+                .build());
+
+        final ClassicHttpResponse response = Mockito.mock(ClassicHttpResponse.class);
+
+        Mockito.when(chain.proceed(
+                Mockito.same(request),
+                Mockito.<ExecChain.Scope>any())).thenReturn(response);
+        Mockito.when(retryStrategy.retryRequest(
+                Mockito.<HttpResponse>any(),
+                Mockito.anyInt(),
+                Mockito.<HttpContext>any())).thenReturn(Boolean.TRUE, Boolean.FALSE);
+        Mockito.when(retryStrategy.getRetryInterval(
+                Mockito.<HttpResponse>any(),
+                Mockito.anyInt(),
+                Mockito.<HttpContext>any())).thenReturn(TimeValue.ofSeconds(1));
+
+        final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context);
+        retryExec.execute(request, scope, chain);
+
+        Mockito.verify(chain, Mockito.times(2)).proceed(
+                Mockito.<ClassicHttpRequest>any(),
+                Mockito.same(scope));
+        Mockito.verify(response, Mockito.times(1)).close();
+    }
+
     @Test(expected = RuntimeException.class)
     public void testStrategyRuntimeException() throws Exception {
         final HttpRoute route = new HttpRoute(target);