Skip to content

Commit c587d9f

Browse files
committed
fix: missing catch clauses in the httpRequest
Documentation about exception thrown by Apache HTTP client: https://hc.apache.org/httpcomponents-core-4.4.x/tutorial/html/blocking-io.html#d5e375 (Legacy doc but some exceptions are still up-to-date) https://hc.apache.org/httpclient-3.x/exception-handling.html [changelog] The requester was not catching correctly all exceptions, making the retry strategy to fail instead of retrying for network related issues.
1 parent 9e783b2 commit c587d9f

6 files changed

Lines changed: 47 additions & 16 deletions

File tree

algoliasearch-apache/src/main/java/com/algolia/search/ApacheHttpRequester.java

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,14 @@
1212
import java.util.concurrent.TimeoutException;
1313
import java.util.function.Consumer;
1414
import javax.annotation.Nonnull;
15-
import org.apache.http.Header;
16-
import org.apache.http.HeaderElement;
17-
import org.apache.http.HttpEntity;
15+
import org.apache.http.*;
1816
import org.apache.http.client.config.RequestConfig;
1917
import org.apache.http.client.entity.DeflateDecompressingEntity;
2018
import org.apache.http.client.entity.GzipDecompressingEntity;
2119
import org.apache.http.client.methods.*;
2220
import org.apache.http.concurrent.FutureCallback;
2321
import org.apache.http.conn.ConnectTimeoutException;
22+
import org.apache.http.conn.ConnectionPoolTimeoutException;
2423
import org.apache.http.entity.ContentType;
2524
import org.apache.http.entity.InputStreamEntity;
2625
import org.apache.http.impl.nio.client.CloseableHttpAsyncClient;
@@ -68,8 +67,12 @@ public CompletableFuture<HttpResponse> performRequestAsync(HttpRequest request)
6867
if (t.getCause() instanceof ConnectTimeoutException
6968
|| t.getCause() instanceof SocketTimeoutException
7069
|| t.getCause() instanceof ConnectException
71-
|| t.getCause() instanceof TimeoutException) {
70+
|| t.getCause() instanceof TimeoutException
71+
|| t.getCause() instanceof ConnectionPoolTimeoutException
72+
|| t.getCause() instanceof NoHttpResponseException) {
7273
return new HttpResponse(true);
74+
} else if (t.getCause() instanceof HttpException) {
75+
return new HttpResponse().setNetworkError(true);
7376
}
7477
throw new AlgoliaRuntimeException(t);
7578
});

algoliasearch-core/src/main/java/com/algolia/search/HttpTransport.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,7 @@ private <TResult> CompletableFuture<TResult> executeWithRetry(
163163
.performRequestAsync(request)
164164
.thenComposeAsync(
165165
resp -> {
166-
switch (retryStrategy.decide(
167-
currentHost, resp.getHttpStatusCode(), resp.isTimedOut())) {
166+
switch (retryStrategy.decide(currentHost, resp)) {
168167
case SUCCESS:
169168
try (InputStream dataStream = resp.getBody()) {
170169
TResult result = Defaults.getObjectMapper().readValue(dataStream, type);

algoliasearch-core/src/main/java/com/algolia/search/RetryStrategy.java

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.algolia.search;
22

3+
import com.algolia.search.models.HttpResponse;
34
import com.algolia.search.models.common.CallType;
45
import com.algolia.search.models.common.RetryOutcome;
56
import com.algolia.search.util.HttpStatusCodeUtils;
@@ -49,18 +50,18 @@ List<StatefulHost> getTryableHosts(CallType callType) {
4950
}
5051

5152
/** Retry logic. Decide if an host is retryable or not regarding the following parameters. */
52-
RetryOutcome decide(StatefulHost tryableHost, int httpResponseCode, boolean isTimedOut) {
53+
RetryOutcome decide(StatefulHost tryableHost, HttpResponse response) {
5354

5455
synchronized (this) {
55-
if (!isTimedOut && HttpStatusCodeUtils.isSuccess(httpResponseCode)) {
56+
if (!response.isTimedOut() && HttpStatusCodeUtils.isSuccess(response)) {
5657
tryableHost.setUp(true);
5758
tryableHost.setLastUse(OffsetDateTime.now(ZoneOffset.UTC));
5859
return RetryOutcome.SUCCESS;
59-
} else if (!isTimedOut && isRetryable(httpResponseCode)) {
60+
} else if (!response.isTimedOut() && isRetryable(response)) {
6061
tryableHost.setUp(false);
6162
tryableHost.setLastUse(OffsetDateTime.now(ZoneOffset.UTC));
6263
return RetryOutcome.RETRY;
63-
} else if (isTimedOut) {
64+
} else if (response.isTimedOut()) {
6465
tryableHost.setUp(true);
6566
tryableHost.setLastUse(OffsetDateTime.now(ZoneOffset.UTC));
6667
tryableHost.incrementRetryCount();
@@ -74,10 +75,13 @@ RetryOutcome decide(StatefulHost tryableHost, int httpResponseCode, boolean isTi
7475
/**
7576
* Tells if the response is retryable or not depending on the http status code
7677
*
77-
* @param httpStatusCode The http status code
78+
* @param response Algolia's API response
7879
*/
79-
private boolean isRetryable(int httpStatusCode) {
80-
return httpStatusCode / 100 != 2 && httpStatusCode / 100 != 4;
80+
private boolean isRetryable(HttpResponse response) {
81+
boolean isRetryableHttpCode =
82+
response.getHttpStatusCode() / 100 != 2 && response.getHttpStatusCode() / 100 != 4;
83+
84+
return isRetryableHttpCode || response.isNetworkError();
8185
}
8286

8387
/**

algoliasearch-core/src/main/java/com/algolia/search/models/HttpResponse.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
public class HttpResponse {
66

7+
public HttpResponse() {}
8+
79
public HttpResponse(int httpStatusCode, InputStream body) {
810
this.httpStatusCode = httpStatusCode;
911
this.body = body;
@@ -54,8 +56,18 @@ public HttpResponse setTimedOut(boolean timedOut) {
5456
return this;
5557
}
5658

59+
public boolean isNetworkError() {
60+
return isNetworkError;
61+
}
62+
63+
public HttpResponse setNetworkError(boolean networkError) {
64+
isNetworkError = networkError;
65+
return this;
66+
}
67+
5768
private int httpStatusCode;
5869
private InputStream body;
5970
private String error;
6071
private boolean isTimedOut;
72+
private boolean isNetworkError;
6173
}

algoliasearch-core/src/main/java/com/algolia/search/util/HttpStatusCodeUtils.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
11
package com.algolia.search.util;
22

3+
import com.algolia.search.models.HttpResponse;
4+
35
public class HttpStatusCodeUtils {
46

7+
public static boolean isSuccess(HttpResponse response) {
8+
return isSuccess(response.getHttpStatusCode());
9+
}
10+
511
public static boolean isSuccess(int httpStatusCode) {
612
return httpStatusCode / 100 == 2;
713
}

algoliasearch-core/src/test/java/com/algolia/search/RetryStrategyTest.java

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import static org.assertj.core.api.Assertions.assertThat;
44

5+
import com.algolia.search.models.HttpResponse;
56
import com.algolia.search.models.common.CallType;
67
import com.algolia.search.models.common.RetryOutcome;
78
import java.util.Collections;
@@ -21,11 +22,16 @@ void testRetryStrategyRetryableFailure(int httpCode, CallType callType) {
2122
List<StatefulHost> hosts = retryStrategy.getTryableHosts(callType);
2223
assertThat(hosts).filteredOn(StatefulHost::isUp).hasSize(4);
2324

24-
RetryOutcome decision = retryStrategy.decide(hosts.get(0), httpCode, false);
25+
RetryOutcome decision =
26+
retryStrategy.decide(hosts.get(0), new HttpResponse(false).setHttpStatusCode(httpCode));
2527
assertThat(decision).isEqualTo(RetryOutcome.RETRY);
2628

2729
List<StatefulHost> updatedHosts = retryStrategy.getTryableHosts(callType);
2830
assertThat(updatedHosts).filteredOn(StatefulHost::isUp).hasSize(3);
31+
32+
RetryOutcome decisionAfterNetworkError =
33+
retryStrategy.decide(hosts.get(0), new HttpResponse().setNetworkError(true));
34+
assertThat(decisionAfterNetworkError).isEqualTo(RetryOutcome.RETRY);
2935
}
3036

3137
@ParameterizedTest
@@ -37,7 +43,8 @@ void testRetryStrategyFailureDecision(int httpCode, CallType callType) {
3743
List<StatefulHost> hosts = retryStrategy.getTryableHosts(callType);
3844
assertThat(hosts).filteredOn(StatefulHost::isUp).hasSize(4);
3945

40-
RetryOutcome decision = retryStrategy.decide(hosts.get(0), httpCode, false);
46+
RetryOutcome decision =
47+
retryStrategy.decide(hosts.get(0), new HttpResponse(false).setHttpStatusCode(httpCode));
4148
assertThat(decision).isEqualTo(RetryOutcome.FAILURE);
4249
}
4350

@@ -50,7 +57,7 @@ void testTimeOutCome(CallType callType) {
5057
List<StatefulHost> hosts = retryStrategy.getTryableHosts(callType);
5158
assertThat(hosts).filteredOn(StatefulHost::isUp).hasSize(4);
5259

53-
RetryOutcome decision = retryStrategy.decide(hosts.get(0), 0, true);
60+
RetryOutcome decision = retryStrategy.decide(hosts.get(0), new HttpResponse(true));
5461
assertThat(decision).isEqualTo(RetryOutcome.RETRY);
5562
}
5663

0 commit comments

Comments
 (0)