From 73c1530b3f99f8c06d73adb122bc9dd24279e27b Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Wed, 24 Mar 2021 19:35:55 +0100 Subject: [PATCH] HTTPCLIENT-2141: HttpClient to not retry requests if the retry interval exceeds the response timeout --- .../impl/classic/HttpRequestRetryExec.java | 13 +++- .../classic/TestHttpRequestRetryExec.java | 66 +++++++++++++++++++ 2 files changed, 77 insertions(+), 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 index ffed45070..71e222135 100644 --- 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.HttpRoute; 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.NoHttpResponseException; 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 class HttpRequestRetryExec implements ExecChainHandler { 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/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestHttpRequestRetryExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestHttpRequestRetryExec.java index 83a1b8347..2c3e91905 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestHttpRequestRetryExec.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestHttpRequestRetryExec.java @@ -36,6 +36,7 @@ import org.apache.hc.client5.http.classic.ExecChain; 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.HttpResponse; 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 class TestHttpRequestRetryExec { 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.any())).thenReturn(response); + Mockito.when(retryStrategy.retryRequest( + Mockito.any(), + Mockito.anyInt(), + Mockito.any())).thenReturn(Boolean.TRUE, Boolean.FALSE); + Mockito.when(retryStrategy.getRetryInterval( + Mockito.any(), + Mockito.anyInt(), + Mockito.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.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.any())).thenReturn(response); + Mockito.when(retryStrategy.retryRequest( + Mockito.any(), + Mockito.anyInt(), + Mockito.any())).thenReturn(Boolean.TRUE, Boolean.FALSE); + Mockito.when(retryStrategy.getRetryInterval( + Mockito.any(), + Mockito.anyInt(), + Mockito.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.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);