Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit fea53b0

Browse files
bfgslandelle
authored andcommitted
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.
1 parent bbaa4c3 commit fea53b0

File tree

2 files changed

+76
-38
lines changed

2 files changed

+76
-38
lines changed

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

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,6 @@
4040
@Builder(toBuilder = true)
4141
@Slf4j
4242
class AsyncHttpClientCall implements Cloneable, okhttp3.Call {
43-
/**
44-
* Default {@link #execute()} timeout in milliseconds (value: <b>{@value}</b>)
45-
*
46-
* @see #execute()
47-
* @see #executeTimeoutMillis
48-
*/
49-
public static final long DEFAULT_EXECUTE_TIMEOUT_MILLIS = 30_000;
50-
5143
private static final ResponseBody EMPTY_BODY = ResponseBody.create(null, "");
5244

5345
/**
@@ -64,12 +56,6 @@ class AsyncHttpClientCall implements Cloneable, okhttp3.Call {
6456
@NonNull
6557
Supplier<AsyncHttpClient> httpClientSupplier;
6658

67-
/**
68-
* {@link #execute()} response timeout in milliseconds.
69-
*/
70-
@Builder.Default
71-
long executeTimeoutMillis = DEFAULT_EXECUTE_TIMEOUT_MILLIS;
72-
7359
/**
7460
* Retrofit request.
7561
*/
@@ -140,7 +126,7 @@ public Request request() {
140126
@Override
141127
public Response execute() throws IOException {
142128
try {
143-
return executeHttpRequest().get(getExecuteTimeoutMillis(), TimeUnit.MILLISECONDS);
129+
return executeHttpRequest().get(getRequestTimeoutMillis(), TimeUnit.MILLISECONDS);
144130
} catch (ExecutionException e) {
145131
throw toIOException(e.getCause());
146132
} catch (Exception e) {
@@ -179,7 +165,16 @@ public boolean isCanceled() {
179165

180166
@Override
181167
public Timeout timeout() {
182-
return new Timeout().timeout(executeTimeoutMillis, TimeUnit.MILLISECONDS);
168+
return new Timeout().timeout(getRequestTimeoutMillis(), TimeUnit.MILLISECONDS);
169+
}
170+
171+
/**
172+
* Returns HTTP request timeout in milliseconds, retrieved from http client configuration.
173+
*
174+
* @return request timeout in milliseconds.
175+
*/
176+
protected long getRequestTimeoutMillis() {
177+
return Math.abs(getHttpClient().getConfig().getRequestTimeout());
183178
}
184179

185180
@Override

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

Lines changed: 65 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,14 @@
1414

1515
import io.netty.handler.codec.http.DefaultHttpHeaders;
1616
import io.netty.handler.codec.http.EmptyHttpHeaders;
17-
import lombok.val;
18-
import okhttp3.MediaType;
19-
import okhttp3.Request;
20-
import okhttp3.RequestBody;
17+
import lombok.*;
18+
import lombok.extern.slf4j.Slf4j;
19+
import okhttp3.*;
2120
import org.asynchttpclient.AsyncCompletionHandler;
2221
import org.asynchttpclient.AsyncHttpClient;
22+
import org.asynchttpclient.AsyncHttpClientConfig;
2323
import org.asynchttpclient.BoundRequestBuilder;
24+
import org.asynchttpclient.DefaultAsyncHttpClientConfig;
2425
import org.asynchttpclient.Response;
2526
import org.mockito.ArgumentCaptor;
2627
import org.testng.Assert;
@@ -38,6 +39,7 @@
3839
import java.util.function.Consumer;
3940
import java.util.function.Supplier;
4041

42+
import static java.util.concurrent.TimeUnit.SECONDS;
4143
import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCall.runConsumer;
4244
import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCall.runConsumers;
4345
import static org.mockito.ArgumentMatchers.any;
@@ -46,15 +48,20 @@
4648
import static org.testng.Assert.assertNotEquals;
4749
import static org.testng.Assert.assertTrue;
4850

51+
@Slf4j
4952
public class AsyncHttpClientCallTest {
5053
static final Request REQUEST = new Request.Builder().url("http://www.google.com/").build();
54+
static final DefaultAsyncHttpClientConfig DEFAULT_AHC_CONFIG = new DefaultAsyncHttpClientConfig.Builder()
55+
.setRequestTimeout(1_000)
56+
.build();
5157

5258
private AsyncHttpClient httpClient;
5359
private Supplier<AsyncHttpClient> httpClientSupplier = () -> httpClient;
5460

5561
@BeforeMethod
5662
void setup() {
57-
this.httpClient = mock(AsyncHttpClient.class);
63+
httpClient = mock(AsyncHttpClient.class);
64+
when(httpClient.getConfig()).thenReturn(DEFAULT_AHC_CONFIG);
5865
}
5966

6067
@Test(expectedExceptions = NullPointerException.class, dataProvider = "first")
@@ -108,7 +115,6 @@ void shouldInvokeConsumersOnEachExecution(Consumer<AsyncCompletionHandler<?>> ha
108115
.onRequestFailure(t -> numFailed.incrementAndGet())
109116
.onRequestSuccess(r -> numOk.incrementAndGet())
110117
.requestCustomizer(rb -> numRequestCustomizer.incrementAndGet())
111-
.executeTimeoutMillis(1000)
112118
.build();
113119

114120
// when
@@ -245,13 +251,12 @@ public void contentTypeHeaderIsPassedInRequest() throws Exception {
245251
Request request = requestWithBody();
246252

247253
ArgumentCaptor<org.asynchttpclient.Request> capture = ArgumentCaptor.forClass(org.asynchttpclient.Request.class);
248-
AsyncHttpClient client = mock(AsyncHttpClient.class);
249254

250-
givenResponseIsProduced(client, aResponse());
255+
givenResponseIsProduced(httpClient, aResponse());
251256

252-
whenRequestIsMade(client, request);
257+
whenRequestIsMade(httpClient, request);
253258

254-
verify(client).executeRequest(capture.capture(), any());
259+
verify(httpClient).executeRequest(capture.capture(), any());
255260

256261
org.asynchttpclient.Request ahcRequest = capture.getValue();
257262

@@ -263,11 +268,9 @@ public void contentTypeHeaderIsPassedInRequest() throws Exception {
263268

264269
@Test
265270
public void contenTypeIsOptionalInResponse() throws Exception {
266-
AsyncHttpClientclient = mock(AsyncHttpClient.class);
271+
givenResponseIsProduced(httpClient, responseWithBody(null, "test"));
267272

268-
givenResponseIsProduced(client, responseWithBody(null, "test"));
269-
270-
okhttp3.Response response = whenRequestIsMade(client, REQUEST);
273+
okhttp3.Response response = whenRequestIsMade(httpClient, REQUEST);
271274

272275
assertEquals(response.code(), 200);
273276
assertEquals(response.header("Server"), "nginx");
@@ -277,11 +280,9 @@ public void contenTypeIsOptionalInResponse() throws Exception {
277280

278281
@Test
279282
public void contentTypeIsProperlyParsedIfPresent() throws Exception {
280-
AsyncHttpClient client = mock(AsyncHttpClient.class);
281-
282-
givenResponseIsProduced(client, responseWithBody("text/plain", "test"));
283+
givenResponseIsProduced(httpClient, responseWithBody("text/plain", "test"));
283284

284-
okhttp3.Response response = whenRequestIsMade(client, REQUEST);
285+
okhttp3.Response response = whenRequestIsMade(httpClient, REQUEST);
285286

286287
assertEquals(response.code(), 200);
287288
assertEquals(response.header("Server"), "nginx");
@@ -292,11 +293,9 @@ public void contentTypeIsProperlyParsedIfPresent() throws Exception {
292293

293294
@Test
294295
public void bodyIsNotNullInResponse() throws Exception {
295-
AsyncHttpClient client = mock(AsyncHttpClient.class);
296-
297-
givenResponseIsProduced(client, responseWithNoBody());
296+
givenResponseIsProduced(httpClient, responseWithNoBody());
298297

299-
okhttp3.Response response = whenRequestIsMade(client, REQUEST);
298+
okhttp3.Response response = whenRequestIsMade(httpClient, REQUEST);
300299

301300
assertEquals(response.code(), 200);
302301
assertEquals(response.header("Server"), "nginx");
@@ -315,6 +314,50 @@ void getHttpClientShouldThrowISEIfSupplierReturnsNull() {
315314
call.getHttpClient();
316315
}
317316

317+
@Test
318+
void shouldReturnTimeoutSpecifiedInAHCInstanceConfig() {
319+
// given:
320+
val cfgBuilder = new DefaultAsyncHttpClientConfig.Builder();
321+
AsyncHttpClientConfig config = null;
322+
323+
// and: setup call
324+
val call = AsyncHttpClientCall.builder()
325+
.httpClientSupplier(httpClientSupplier)
326+
.request(requestWithBody())
327+
.build();
328+
329+
// when: set read timeout to 5s, req timeout to 6s
330+
config = cfgBuilder.setReadTimeout((int) SECONDS.toMillis(5))
331+
.setRequestTimeout((int) SECONDS.toMillis(6))
332+
.build();
333+
when(httpClient.getConfig()).thenReturn(config);
334+
335+
// then: expect request timeout
336+
assertEquals(call.getRequestTimeoutMillis(), SECONDS.toMillis(6));
337+
assertEquals(call.timeout().timeoutNanos(), SECONDS.toNanos(6));
338+
339+
// when: set read timeout to 10 seconds, req timeout to 7s
340+
config = cfgBuilder.setReadTimeout((int) SECONDS.toMillis(10))
341+
.setRequestTimeout((int) SECONDS.toMillis(7))
342+
.build();
343+
when(httpClient.getConfig()).thenReturn(config);
344+
345+
// then: expect request timeout
346+
assertEquals(call.getRequestTimeoutMillis(), SECONDS.toMillis(7));
347+
assertEquals(call.timeout().timeoutNanos(), SECONDS.toNanos(7));
348+
349+
// when: set request timeout to a negative value, just for fun.
350+
config = cfgBuilder.setRequestTimeout(-1000)
351+
.setReadTimeout(2000)
352+
.build();
353+
354+
when(httpClient.getConfig()).thenReturn(config);
355+
356+
// then: expect request timeout, but as positive value
357+
assertEquals(call.getRequestTimeoutMillis(), SECONDS.toMillis(1));
358+
assertEquals(call.timeout().timeoutNanos(), SECONDS.toNanos(1));
359+
}
360+
318361
private void givenResponseIsProduced(AsyncHttpClient client, Response response) {
319362
when(client.executeRequest(any(org.asynchttpclient.Request.class), any())).thenAnswer(invocation -> {
320363
AsyncCompletionHandler<Response> handler = invocation.getArgument(1);

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /