Improved retrofit timeout handling. (#1618)

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

Author: bfg

Diff for: 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: <b>{@value}</b>)
-   *
-   * @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<AsyncHttpClient> 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 for: 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<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 +115,6 @@ void shouldInvokeConsumersOnEachExecution(Consumer<AsyncCompletionHandler<?>> 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<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 +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<Response> handler = invocation.getArgument(1);