diff --git a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java index f7660519b..df3547db3 100644 --- a/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java +++ b/cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java @@ -14,6 +14,7 @@ import org.apache.hc.client5.http.classic.HttpClient; import org.apache.hc.client5.http.config.ConnectionConfig; import org.apache.hc.client5.http.config.RequestConfig; +import org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy; import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; import org.apache.hc.client5.http.impl.classic.HttpClientBuilder; import org.apache.hc.client5.http.impl.classic.HttpClients; @@ -24,8 +25,12 @@ import org.apache.hc.client5.http.ssl.NoopHostnameVerifier; import org.apache.hc.client5.http.ssl.TlsSocketStrategy; import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.HttpRequestInterceptor; +import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.io.SocketConfig; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.util.TimeValue; import org.apache.hc.core5.util.Timeout; import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationAccessException; @@ -92,6 +97,7 @@ private CloseableHttpClient buildHttpClient( .custom() .setConnectionManager(getConnectionManager(destination)) .setDefaultRequestConfig(requestConfig) + .setRetryStrategy(new LoggingHttpRequestRetryStrategy(destination)) .setProxy(getProxy(destination)); if( requestInterceptor != null ) { @@ -233,4 +239,54 @@ private boolean isValidProxyConfigurationUriInDestination( @Nullable final HttpD } return true; } + + private static class LoggingHttpRequestRetryStrategy extends DefaultHttpRequestRetryStrategy + { + private static final int MAX_RETRIES = 1; // default + private static final Duration RETRY_INTERVAL = Duration.ofSeconds(1L); // default + + private final @Nonnull String destinationRef; + + public LoggingHttpRequestRetryStrategy( final @Nullable HttpDestinationProperties destination ) + { + super(MAX_RETRIES, TimeValue.of(RETRY_INTERVAL)); + this.destinationRef = destination == null ? "" : " for destination " + destination; + } + + @Override + public boolean retryRequest( final HttpResponse response, final int execCount, final HttpContext context ) + { + final boolean retry = super.retryRequest(response, execCount, context); + if( retry ) { + final String msg = "Retrying request{} due to response {}. Retry attempt {}/{} after {}s."; + log.warn(msg, destinationRef, response.getCode(), execCount, MAX_RETRIES, RETRY_INTERVAL.getSeconds()); + } + return retry; + } + + @Override + public boolean retryRequest( + final HttpRequest req, + final IOException exception, + final int execCount, + final HttpContext context ) + { + final boolean retry = super.retryRequest(req, exception, execCount, context); + if( retry ) { + final String msg = + "Retrying {} request{} to {} due to exception \"{}\". Retry attempt {}/{} after {}s."; + log + .warn( + msg, + req.getMethod(), + destinationRef, + req.getRequestUri(), + exception.getMessage(), + execCount, + MAX_RETRIES, + RETRY_INTERVAL.getSeconds()); + } + return retry; + } + } } diff --git a/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/LoggingHttpRequestRetryStrategyTest.java b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/LoggingHttpRequestRetryStrategyTest.java new file mode 100644 index 000000000..3c72ae0e0 --- /dev/null +++ b/cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/LoggingHttpRequestRetryStrategyTest.java @@ -0,0 +1,143 @@ +package com.sap.cloud.sdk.cloudplatform.connectivity; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +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 com.github.tomakehurst.wiremock.stubbing.Scenario.STARTED; +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.IOException; + +import org.apache.hc.client5.http.classic.methods.HttpGet; +import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import org.apache.hc.core5.http.HttpResponse; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; + +class LoggingHttpRequestRetryStrategyTest +{ + @RegisterExtension + static final WireMockExtension WIRE_MOCK_SERVER = + WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build(); + + @Test + void testHttpClientRetriesOn503() + throws IOException + { + // Use WireMock Scenarios to simulate: first request returns 503, second returns 200 + WIRE_MOCK_SERVER + .stubFor( + get(urlEqualTo("/retry-test")) + .inScenario("Retry 503") + .whenScenarioStateIs(STARTED) + .willReturn(aResponse().withStatus(503)) + .willSetStateTo("Recovered")); + + WIRE_MOCK_SERVER + .stubFor( + get(urlEqualTo("/retry-test")) + .inScenario("Retry 503") + .whenScenarioStateIs("Recovered") + .willReturn(aResponse().withStatus(200).withBody("OK"))); + + final ApacheHttpClient5Factory factory = new ApacheHttpClient5FactoryBuilder().build(); + + try( CloseableHttpClient client = (CloseableHttpClient) factory.createHttpClient() ) { + final HttpGet request = new HttpGet(WIRE_MOCK_SERVER.url("/retry-test")); + final int statusCode = client.execute(request, HttpResponse::getCode); + + assertThat(statusCode).isEqualTo(200); + WIRE_MOCK_SERVER.verify(2, getRequestedFor(urlEqualTo("/retry-test"))); + } + } + + @Test + void testHttpClientRetriesOn429() + throws IOException + { + // Use WireMock Scenarios to simulate: first request returns 429, second returns 200 + WIRE_MOCK_SERVER + .stubFor( + get(urlEqualTo("/rate-limit-test")) + .inScenario("Retry 429") + .whenScenarioStateIs(STARTED) + .willReturn(aResponse().withStatus(429)) + .willSetStateTo("Recovered")); + + WIRE_MOCK_SERVER + .stubFor( + get(urlEqualTo("/rate-limit-test")) + .inScenario("Retry 429") + .whenScenarioStateIs("Recovered") + .willReturn(aResponse().withStatus(200).withBody("OK"))); + + final ApacheHttpClient5Factory factory = new ApacheHttpClient5FactoryBuilder().build(); + + try( CloseableHttpClient client = (CloseableHttpClient) factory.createHttpClient() ) { + final HttpGet request = new HttpGet(WIRE_MOCK_SERVER.url("/rate-limit-test")); + final int statusCode = client.execute(request, HttpResponse::getCode); + + assertThat(statusCode).isEqualTo(200); + WIRE_MOCK_SERVER.verify(2, getRequestedFor(urlEqualTo("/rate-limit-test"))); + } + } + + @Test + void testHttpClientDoesNotRetryOn500() + throws IOException + { + // 500 Internal Server Error should not trigger retry + WIRE_MOCK_SERVER.stubFor(get(urlEqualTo("/no-retry-test")).willReturn(aResponse().withStatus(500))); + + final ApacheHttpClient5Factory factory = new ApacheHttpClient5FactoryBuilder().build(); + + try( CloseableHttpClient client = (CloseableHttpClient) factory.createHttpClient() ) { + final HttpGet request = new HttpGet(WIRE_MOCK_SERVER.url("/no-retry-test")); + final int statusCode = client.execute(request, HttpResponse::getCode); + + assertThat(statusCode).isEqualTo(500); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(urlEqualTo("/no-retry-test"))); + } + } + + @Test + void testHttpClientDoesNotRetryOn400() + throws IOException + { + // 400 Bad Request should not trigger retry + WIRE_MOCK_SERVER.stubFor(get(urlEqualTo("/bad-request-test")).willReturn(aResponse().withStatus(400))); + + final ApacheHttpClient5Factory factory = new ApacheHttpClient5FactoryBuilder().build(); + + try( CloseableHttpClient client = (CloseableHttpClient) factory.createHttpClient() ) { + final HttpGet request = new HttpGet(WIRE_MOCK_SERVER.url("/bad-request-test")); + final int statusCode = client.execute(request, HttpResponse::getCode); + + assertThat(statusCode).isEqualTo(400); + WIRE_MOCK_SERVER.verify(1, getRequestedFor(urlEqualTo("/bad-request-test"))); + } + } + + @Test + void testHttpClientStopsRetryingAfterMaxAttempts() + throws IOException + { + // Server always returns 503 - should stop after max retries (1 retry = 2 total requests) + WIRE_MOCK_SERVER.stubFor(get(urlEqualTo("/always-503")).willReturn(aResponse().withStatus(503))); + + final ApacheHttpClient5Factory factory = new ApacheHttpClient5FactoryBuilder().build(); + + try( CloseableHttpClient client = (CloseableHttpClient) factory.createHttpClient() ) { + final HttpGet request = new HttpGet(WIRE_MOCK_SERVER.url("/always-503")); + final int statusCode = client.execute(request, HttpResponse::getCode); + + // DefaultHttpRequestRetryStrategy allows 1 retry, so 2 total requests + assertThat(statusCode).isEqualTo(503); + WIRE_MOCK_SERVER.verify(2, getRequestedFor(urlEqualTo("/always-503"))); + } + } +} diff --git a/release_notes.md b/release_notes.md index 3112514a9..6e2985406 100644 --- a/release_notes.md +++ b/release_notes.md @@ -16,7 +16,7 @@ ### 📈 Improvements -- +- Enable Retry behavior for HttpClient5 instances, to mirror legacy behvior for HttpClient4: `1` retry with `1s` delay for response codes `429` (Too Many Requests) and `503` (Service Unavailable). ### 🐛 Fixed Issues