Skip to content

Improved retrofit timeout handling. #1618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 28, 2019
Merged

Improved retrofit timeout handling. #1618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 11 additions & 16 deletions extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, "");

/**
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
87 changes: 65 additions & 22 deletions .../retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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")
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();

Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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);
Expand Down