Improved retrofit timeout handling.

Retrofit `Call` now uses http client's configured timeout to compute
value for `timeout()` and blocking `execute()` exception.

Author: bfg

extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java

@@ -44,9 +44,8 @@ class AsyncHttpClientCall implements Cloneable, okhttp3.Call {
    * Default {@link #execute()} timeout in milliseconds (value: <b>{@value}</b>)
    *
    * @see #execute()
-   * @see #executeTimeoutMillis
    */
-  public static final long DEFAULT_EXECUTE_TIMEOUT_MILLIS = 30_000;
+  static final long DEFAULT_EXECUTE_TIMEOUT_MILLIS = 30_000;
 
   private static final ResponseBody EMPTY_BODY = ResponseBody.create(null, "");
 
@@ -64,12 +63,6 @@ class AsyncHttpClientCall implements Cloneable, okhttp3.Call {
   @NonNull
   Supplier<AsyncHttpClient> httpClientSupplier;
 
-  /**
-   * {@link #execute()} response timeout in milliseconds.
-   */
-  @Builder.Default
-  long executeTimeoutMillis = DEFAULT_EXECUTE_TIMEOUT_MILLIS;
-
   /**
    * Retrofit request.
    */
@@ -140,7 +133,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 +172,19 @@ 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.
+   * @see #DEFAULT_EXECUTE_TIMEOUT_MILLIS
+   */
+  protected long getRequestTimeoutMillis() {
+    val config = getHttpClient().getConfig();
+    val timeout = Math.max(config.getReadTimeout(), config.getRequestTimeout());
+    return (timeout < 1) ? DEFAULT_EXECUTE_TIMEOUT_MILLIS : timeout;
   }
 
   @Override

extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java

@@ -14,13 +14,17 @@
 
 import io.netty.handler.codec.http.DefaultHttpHeaders;
 import io.netty.handler.codec.http.EmptyHttpHeaders;
+import lombok.extern.slf4j.Slf4j;
 import lombok.val;
 import okhttp3.MediaType;
 import okhttp3.Request;
 import okhttp3.RequestBody;
 import org.asynchttpclient.AsyncCompletionHandler;
 import org.asynchttpclient.AsyncHttpClient;
+import org.asynchttpclient.AsyncHttpClientConfig;
 import org.asynchttpclient.BoundRequestBuilder;
+import org.asynchttpclient.DefaultAsyncHttpClient;
+import org.asynchttpclient.DefaultAsyncHttpClientConfig;
 import org.asynchttpclient.Response;
 import org.mockito.ArgumentCaptor;
 import org.testng.Assert;
@@ -33,11 +37,14 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
 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 +53,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<AsyncHttpClient> 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 +120,6 @@ void shouldInvokeConsumersOnEachExecution(Consumer<AsyncCompletionHandler<?>> ha
                 .onRequestFailure(t -> numFailed.incrementAndGet())
                 .onRequestSuccess(r -> numOk.incrementAndGet())
                 .requestCustomizer(rb -> numRequestCustomizer.incrementAndGet())
-                .executeTimeoutMillis(1000)
                 .build();
 
         // when
@@ -245,13 +256,12 @@ public void contentTypeHeaderIsPassedInRequest() throws Exception {
         Request request = requestWithBody();
 
         ArgumentCaptor<org.asynchttpclient.Request> 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 +273,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 +285,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 +298,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 +319,40 @@ void getHttpClientShouldThrowISEIfSupplierReturnsNull() {
       call.getHttpClient();
     }
 
+    @Test
+    void shouldReturnTimeoutSpecifiedInAHCInstanceConfig() {
+        // given:
+        val cfgBuilder = new DefaultAsyncHttpClientConfig.Builder();
+        //val clientRef = new AtomicReference<AsyncHttpClient>();
+        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
+        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:
+        assertEquals(call.getRequestTimeoutMillis(), SECONDS.toMillis(10));
+        assertEquals(call.timeout().timeoutNanos(), SECONDS.toNanos(10));
+    }
+
     private void givenResponseIsProduced(AsyncHttpClient client, Response response) {
         when(client.executeRequest(any(org.asynchttpclient.Request.class), any())).thenAnswer(invocation -> {
             AsyncCompletionHandler<Response> handler = invocation.getArgument(1);