diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 7502db26f..5d2513626 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -406,6 +406,8 @@ public class WhisperServerService extends Application dynamicConfigurationManager; + @VisibleForTesting + HCaptchaClient(final String apiKey, + final FaultTolerantHttpClient faultTolerantHttpClient, + final DynamicConfigurationManager dynamicConfigurationManager) { + this.apiKey = apiKey; + this.client = faultTolerantHttpClient; + this.dynamicConfigurationManager = dynamicConfigurationManager; + } + public HCaptchaClient( final String apiKey, - final HttpClient client, + final ScheduledExecutorService retryExecutor, + final CircuitBreakerConfiguration circuitBreakerConfiguration, + final RetryConfiguration retryConfiguration, final DynamicConfigurationManager dynamicConfigurationManager) { - this.apiKey = apiKey; - this.client = client; - this.dynamicConfigurationManager = dynamicConfigurationManager; + this(apiKey, + FaultTolerantHttpClient.newBuilder() + .withName("hcaptcha") + .withCircuitBreaker(circuitBreakerConfiguration) + .withExecutor(Executors.newCachedThreadPool()) + .withRetryExecutor(retryExecutor) + .withRetry(retryConfiguration) + .withRetryOnException(ex -> ex instanceof IOException) + .withConnectTimeout(Duration.ofSeconds(10)) + .withVersion(HttpClient.Version.HTTP_2) + .build(), + dynamicConfigurationManager); } @Override @@ -82,11 +113,12 @@ public class HCaptchaClient implements CaptchaClient { .POST(HttpRequest.BodyPublishers.ofString(body)) .build(); - HttpResponse response; + final HttpResponse response; try { - response = this.client.send(request, HttpResponse.BodyHandlers.ofString()); - } catch (InterruptedException e) { - throw new IOException(e); + response = this.client.sendAsync(request, HttpResponse.BodyHandlers.ofString()).join(); + } catch (CompletionException e) { + logger.warn("failed to make http request to hCaptcha: {}", e.getMessage()); + throw new IOException(ExceptionUtils.unwrap(e)); } if (response.statusCode() != Response.Status.OK.getStatusCode()) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/HCaptchaConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/HCaptchaConfiguration.java index 6c2ea26bf..d5a334e3d 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/HCaptchaConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/HCaptchaConfiguration.java @@ -5,8 +5,35 @@ package org.whispersystems.textsecuregcm.configuration; +import com.fasterxml.jackson.annotation.JsonProperty; import javax.validation.constraints.NotNull; import org.whispersystems.textsecuregcm.configuration.secrets.SecretString; -public record HCaptchaConfiguration(@NotNull SecretString apiKey) { +public class HCaptchaConfiguration { + + @JsonProperty + @NotNull + SecretString apiKey; + + @JsonProperty + @NotNull + CircuitBreakerConfiguration circuitBreaker = new CircuitBreakerConfiguration(); + + @JsonProperty + @NotNull + RetryConfiguration retry = new RetryConfiguration(); + + + public SecretString getApiKey() { + return apiKey; + } + + public CircuitBreakerConfiguration getCircuitBreaker() { + return circuitBreaker; + } + + public RetryConfiguration getRetry() { + return retry; + } + } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClient.java b/service/src/main/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClient.java index 21b608a2b..ef729c136 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClient.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClient.java @@ -5,6 +5,7 @@ package org.whispersystems.textsecuregcm.http; +import com.google.common.annotations.VisibleForTesting; import io.github.resilience4j.circuitbreaker.CircuitBreaker; import io.github.resilience4j.retry.Retry; import io.github.resilience4j.retry.RetryConfig; @@ -18,12 +19,14 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; +import java.util.function.Predicate; import java.util.function.Supplier; import org.glassfish.jersey.SslConfigurator; import org.whispersystems.textsecuregcm.configuration.CircuitBreakerConfiguration; import org.whispersystems.textsecuregcm.configuration.RetryConfiguration; import org.whispersystems.textsecuregcm.util.CertificateUtil; import org.whispersystems.textsecuregcm.util.CircuitBreakerUtil; +import org.whispersystems.textsecuregcm.util.ExceptionUtils; public class FaultTolerantHttpClient { @@ -40,9 +43,10 @@ public class FaultTolerantHttpClient { return new Builder(); } - private FaultTolerantHttpClient(String name, HttpClient httpClient, ScheduledExecutorService retryExecutor, + @VisibleForTesting + FaultTolerantHttpClient(String name, HttpClient httpClient, ScheduledExecutorService retryExecutor, Duration defaultRequestTimeout, RetryConfiguration retryConfiguration, - CircuitBreakerConfiguration circuitBreakerConfiguration) { + final Predicate retryOnException, CircuitBreakerConfiguration circuitBreakerConfiguration) { this.httpClient = httpClient; this.retryExecutor = retryExecutor; @@ -55,9 +59,12 @@ public class FaultTolerantHttpClient { if (this.retryExecutor == null) { throw new IllegalArgumentException("retryExecutor must be specified with retryConfiguration"); } - RetryConfig retryConfig = retryConfiguration.toRetryConfigBuilder() - .retryOnResult(o -> o.statusCode() >= 500).build(); - this.retry = Retry.of(name + "-retry", retryConfig); + final RetryConfig.Builder retryConfig = retryConfiguration.toRetryConfigBuilder() + .retryOnResult(o -> o.statusCode() >= 500); + if (retryOnException != null) { + retryConfig.retryOnException(retryOnException); + } + this.retry = Retry.of(name + "-retry", retryConfig.build()); CircuitBreakerUtil.registerMetrics(retry, FaultTolerantHttpClient.class); } else { this.retry = null; @@ -101,6 +108,7 @@ public class FaultTolerantHttpClient { private KeyStore trustStore; private String securityProtocol = SECURITY_PROTOCOL_TLS_1_2; private RetryConfiguration retryConfiguration; + private Predicate retryOnException; private CircuitBreakerConfiguration circuitBreakerConfiguration; private Builder() { @@ -161,6 +169,11 @@ public class FaultTolerantHttpClient { return this; } + public Builder withRetryOnException(final Predicate predicate) { + this.retryOnException = throwable -> predicate.test(ExceptionUtils.unwrap(throwable)); + return this; + } + public FaultTolerantHttpClient build() { if (this.circuitBreakerConfiguration == null || this.name == null || this.executor == null) { throw new IllegalArgumentException("Must specify circuit breaker config, name, and executor"); @@ -181,7 +194,7 @@ public class FaultTolerantHttpClient { builder.sslContext(sslConfigurator.createSSLContext()); return new FaultTolerantHttpClient(name, builder.build(), retryExecutor, requestTimeout, retryConfiguration, - circuitBreakerConfiguration); + retryOnException, circuitBreakerConfiguration); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java index 04e22bc03..6f6d98597 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java @@ -15,6 +15,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.Map; import java.util.Set; +import java.util.concurrent.CompletableFuture; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -22,6 +23,7 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicCaptchaConfiguration; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; +import org.whispersystems.textsecuregcm.http.FaultTolerantHttpClient; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; public class HCaptchaClientTest { @@ -44,7 +46,7 @@ public class HCaptchaClientTest { public void captchaProcessed(final boolean success, final float score, final boolean expectedResult) throws IOException, InterruptedException { - final HttpClient client = mockResponder(200, String.format(""" + final FaultTolerantHttpClient client = mockResponder(200, String.format(""" { "success": %b, "score": %f, @@ -64,14 +66,14 @@ public class HCaptchaClientTest { @Test public void errorResponse() throws IOException, InterruptedException { - final HttpClient httpClient = mockResponder(503, ""); + final FaultTolerantHttpClient httpClient = mockResponder(503, ""); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); assertThrows(IOException.class, () -> client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null)); } @Test public void invalidScore() throws IOException, InterruptedException { - final HttpClient httpClient = mockResponder(200, """ + final FaultTolerantHttpClient httpClient = mockResponder(200, """ {"success" : true, "score": 1.1} """); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); @@ -80,7 +82,7 @@ public class HCaptchaClientTest { @Test public void badBody() throws IOException, InterruptedException { - final HttpClient httpClient = mockResponder(200, """ + final FaultTolerantHttpClient httpClient = mockResponder(200, """ {"success" : true, """); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); @@ -102,15 +104,14 @@ public class HCaptchaClientTest { } } - private static HttpClient mockResponder(final int statusCode, final String jsonBody) - throws IOException, InterruptedException { - HttpClient httpClient = mock(HttpClient.class); + private static FaultTolerantHttpClient mockResponder(final int statusCode, final String jsonBody) { + FaultTolerantHttpClient httpClient = mock(FaultTolerantHttpClient.class); @SuppressWarnings("unchecked") final HttpResponse httpResponse = mock(HttpResponse.class); when(httpResponse.body()).thenReturn(jsonBody); when(httpResponse.statusCode()).thenReturn(statusCode); - when(httpClient.send(any(), any())).thenReturn(httpResponse); + when(httpClient.sendAsync(any(), any())).thenReturn(CompletableFuture.completedFuture(httpResponse)); return httpClient; } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClientTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClientTest.java index 81bfb7ef7..f1380dfff 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClientTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClientTest.java @@ -11,6 +11,11 @@ import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; import static org.assertj.core.api.AssertionsForClassTypes.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import com.github.tomakehurst.wiremock.junit5.WireMockExtension; import io.github.resilience4j.circuitbreaker.CallNotPermittedException; @@ -19,6 +24,8 @@ import java.net.URI; import java.net.http.HttpClient; import java.net.http.HttpRequest; import java.net.http.HttpResponse; +import java.time.Duration; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -114,6 +121,35 @@ class FaultTolerantHttpClientTest { wireMock.verify(3, getRequestedFor(urlEqualTo("/failure"))); } + @Test + void testRetryGetOnException() { + final HttpClient mockHttpClient = mock(HttpClient.class); + final FaultTolerantHttpClient client = new FaultTolerantHttpClient( + "test", + mockHttpClient, + retryExecutor, + Duration.ofSeconds(1), + new RetryConfiguration(), + throwable -> throwable instanceof IOException, + new CircuitBreakerConfiguration()); + + when(mockHttpClient.sendAsync(any(), any())) + .thenReturn(CompletableFuture.failedFuture(new IOException("test exception"))); + + HttpRequest request = HttpRequest.newBuilder() + .uri(URI.create("http://localhost:1234/failure")) + .GET() + .build(); + + try { + client.sendAsync(request, HttpResponse.BodyHandlers.ofString()).join(); + throw new AssertionError("Should have failed!"); + } catch (CompletionException e) { + assertThat(e.getCause()).isInstanceOf(IOException.class); + } + verify(mockHttpClient, times(3)).sendAsync(any(), any()); + } + @Test void testNetworkFailureCircuitBreaker() throws InterruptedException { CircuitBreakerConfiguration circuitBreakerConfiguration = new CircuitBreakerConfiguration();