From a53a5291d31094fcf7812bd5fdc1d0f2f988b0e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Brane=20F=2E=20Gra=C4=8Dnar?= Date: Thu, 28 Feb 2019 01:19:11 +0100 Subject: [PATCH] Improved retrofit timeout handling. Retrofit `Call` now uses http client's configured request timeout to compute value for `timeout()` and blocking `execute()` method execution timeout. --- .../extras/retrofit/AsyncHttpClientCall.java | 27 +++--- .../retrofit/AsyncHttpClientCallTest.java | 87 ++++++++++++++----- 2 files changed, 76 insertions(+), 38 deletions(-) diff --git a/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java b/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java index 39107ce02a..bbd7601873 100644 --- a/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java +++ b/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java @@ -40,14 +40,6 @@ @Builder(toBuilder = true) @Slf4j class AsyncHttpClientCall implements Cloneable, okhttp3.Call { - /** - * Default {@link #execute()} timeout in milliseconds (value: {@value}) - * - * @see #execute() - * @see #executeTimeoutMillis - */ - public static final long DEFAULT_EXECUTE_TIMEOUT_MILLIS = 30_000; - private static final ResponseBody EMPTY_BODY = ResponseBody.create(null, ""); /** @@ -64,12 +56,6 @@ class AsyncHttpClientCall implements Cloneable, okhttp3.Call { @NonNull Supplier httpClientSupplier; - /** - * {@link #execute()} response timeout in milliseconds. - */ - @Builder.Default - long executeTimeoutMillis = DEFAULT_EXECUTE_TIMEOUT_MILLIS; - /** * Retrofit request. */ @@ -140,7 +126,7 @@ public Request request() { @Override public Response execute() throws IOException { try { - return executeHttpRequest().get(getExecuteTimeoutMillis(), TimeUnit.MILLISECONDS); + return executeHttpRequest().get(getRequestTimeoutMillis(), TimeUnit.MILLISECONDS); } catch (ExecutionException e) { throw toIOException(e.getCause()); } catch (Exception e) { @@ -179,7 +165,16 @@ public boolean isCanceled() { @Override public Timeout timeout() { - return new Timeout().timeout(executeTimeoutMillis, TimeUnit.MILLISECONDS); + return new Timeout().timeout(getRequestTimeoutMillis(), TimeUnit.MILLISECONDS); + } + + /** + * Returns HTTP request timeout in milliseconds, retrieved from http client configuration. + * + * @return request timeout in milliseconds. + */ + protected long getRequestTimeoutMillis() { + return Math.abs(getHttpClient().getConfig().getRequestTimeout()); } @Override diff --git a/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java b/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java index ab908730af..e655ed73fc 100644 --- a/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java +++ b/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java @@ -14,13 +14,14 @@ import io.netty.handler.codec.http.DefaultHttpHeaders; import io.netty.handler.codec.http.EmptyHttpHeaders; -import lombok.val; -import okhttp3.MediaType; -import okhttp3.Request; -import okhttp3.RequestBody; +import lombok.*; +import lombok.extern.slf4j.Slf4j; +import okhttp3.*; import org.asynchttpclient.AsyncCompletionHandler; import org.asynchttpclient.AsyncHttpClient; +import org.asynchttpclient.AsyncHttpClientConfig; import org.asynchttpclient.BoundRequestBuilder; +import org.asynchttpclient.DefaultAsyncHttpClientConfig; import org.asynchttpclient.Response; import org.mockito.ArgumentCaptor; import org.testng.Assert; @@ -38,6 +39,7 @@ import java.util.function.Consumer; import java.util.function.Supplier; +import static java.util.concurrent.TimeUnit.SECONDS; import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCall.runConsumer; import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCall.runConsumers; import static org.mockito.ArgumentMatchers.any; @@ -46,15 +48,20 @@ import static org.testng.Assert.assertNotEquals; import static org.testng.Assert.assertTrue; +@Slf4j public class AsyncHttpClientCallTest { static final Request REQUEST = new Request.Builder().url("http://www.google.com/").build(); + static final DefaultAsyncHttpClientConfig DEFAULT_AHC_CONFIG = new DefaultAsyncHttpClientConfig.Builder() + .setRequestTimeout(1_000) + .build(); private AsyncHttpClient httpClient; private Supplier httpClientSupplier = () -> httpClient; @BeforeMethod void setup() { - this.httpClient = mock(AsyncHttpClient.class); + httpClient = mock(AsyncHttpClient.class); + when(httpClient.getConfig()).thenReturn(DEFAULT_AHC_CONFIG); } @Test(expectedExceptions = NullPointerException.class, dataProvider = "first") @@ -108,7 +115,6 @@ void shouldInvokeConsumersOnEachExecution(Consumer> ha .onRequestFailure(t -> numFailed.incrementAndGet()) .onRequestSuccess(r -> numOk.incrementAndGet()) .requestCustomizer(rb -> numRequestCustomizer.incrementAndGet()) - .executeTimeoutMillis(1000) .build(); // when @@ -245,13 +251,12 @@ public void contentTypeHeaderIsPassedInRequest() throws Exception { Request request = requestWithBody(); ArgumentCaptor capture = ArgumentCaptor.forClass(org.asynchttpclient.Request.class); - AsyncHttpClient client = mock(AsyncHttpClient.class); - givenResponseIsProduced(client, aResponse()); + givenResponseIsProduced(httpClient, aResponse()); - whenRequestIsMade(client, request); + whenRequestIsMade(httpClient, request); - verify(client).executeRequest(capture.capture(), any()); + verify(httpClient).executeRequest(capture.capture(), any()); org.asynchttpclient.Request ahcRequest = capture.getValue(); @@ -263,11 +268,9 @@ public void contentTypeHeaderIsPassedInRequest() throws Exception { @Test public void contenTypeIsOptionalInResponse() throws Exception { - AsyncHttpClient client = mock(AsyncHttpClient.class); + givenResponseIsProduced(httpClient, responseWithBody(null, "test")); - givenResponseIsProduced(client, responseWithBody(null, "test")); - - okhttp3.Response response = whenRequestIsMade(client, REQUEST); + okhttp3.Response response = whenRequestIsMade(httpClient, REQUEST); assertEquals(response.code(), 200); assertEquals(response.header("Server"), "nginx"); @@ -277,11 +280,9 @@ public void contenTypeIsOptionalInResponse() throws Exception { @Test public void contentTypeIsProperlyParsedIfPresent() throws Exception { - AsyncHttpClient client = mock(AsyncHttpClient.class); - - givenResponseIsProduced(client, responseWithBody("text/plain", "test")); + givenResponseIsProduced(httpClient, responseWithBody("text/plain", "test")); - okhttp3.Response response = whenRequestIsMade(client, REQUEST); + okhttp3.Response response = whenRequestIsMade(httpClient, REQUEST); assertEquals(response.code(), 200); assertEquals(response.header("Server"), "nginx"); @@ -292,11 +293,9 @@ public void contentTypeIsProperlyParsedIfPresent() throws Exception { @Test public void bodyIsNotNullInResponse() throws Exception { - AsyncHttpClient client = mock(AsyncHttpClient.class); - - givenResponseIsProduced(client, responseWithNoBody()); + givenResponseIsProduced(httpClient, responseWithNoBody()); - okhttp3.Response response = whenRequestIsMade(client, REQUEST); + okhttp3.Response response = whenRequestIsMade(httpClient, REQUEST); assertEquals(response.code(), 200); assertEquals(response.header("Server"), "nginx"); @@ -315,6 +314,50 @@ void getHttpClientShouldThrowISEIfSupplierReturnsNull() { call.getHttpClient(); } + @Test + void shouldReturnTimeoutSpecifiedInAHCInstanceConfig() { + // given: + val cfgBuilder = new DefaultAsyncHttpClientConfig.Builder(); + AsyncHttpClientConfig config = null; + + // and: setup call + val call = AsyncHttpClientCall.builder() + .httpClientSupplier(httpClientSupplier) + .request(requestWithBody()) + .build(); + + // when: set read timeout to 5s, req timeout to 6s + config = cfgBuilder.setReadTimeout((int) SECONDS.toMillis(5)) + .setRequestTimeout((int) SECONDS.toMillis(6)) + .build(); + when(httpClient.getConfig()).thenReturn(config); + + // then: expect request timeout + assertEquals(call.getRequestTimeoutMillis(), SECONDS.toMillis(6)); + assertEquals(call.timeout().timeoutNanos(), SECONDS.toNanos(6)); + + // when: set read timeout to 10 seconds, req timeout to 7s + config = cfgBuilder.setReadTimeout((int) SECONDS.toMillis(10)) + .setRequestTimeout((int) SECONDS.toMillis(7)) + .build(); + when(httpClient.getConfig()).thenReturn(config); + + // then: expect request timeout + assertEquals(call.getRequestTimeoutMillis(), SECONDS.toMillis(7)); + assertEquals(call.timeout().timeoutNanos(), SECONDS.toNanos(7)); + + // when: set request timeout to a negative value, just for fun. + config = cfgBuilder.setRequestTimeout(-1000) + .setReadTimeout(2000) + .build(); + + when(httpClient.getConfig()).thenReturn(config); + + // then: expect request timeout, but as positive value + assertEquals(call.getRequestTimeoutMillis(), SECONDS.toMillis(1)); + assertEquals(call.timeout().timeoutNanos(), SECONDS.toNanos(1)); + } + private void givenResponseIsProduced(AsyncHttpClient client, Response response) { when(client.executeRequest(any(org.asynchttpclient.Request.class), any())).thenAnswer(invocation -> { AsyncCompletionHandler handler = invocation.getArgument(1);