diff --git a/core-httpclient-impl/README.md b/core-httpclient-impl/README.md index 8e70b2dd..55eac460 100644 --- a/core-httpclient-impl/README.md +++ b/core-httpclient-impl/README.md @@ -106,24 +106,24 @@ The number of workers determines the number of threads the thread pool uses. ### Builder Methods The following builder methods can be used to custom configure the `AsyncEventHandler`. -|Method Name|Default Value|Description| -|---|---|---| -|`withQueueCapacity(int)`|10000|Queue size for pending logEvents| -|`withNumWorkers(int)`|2|Number of worker threads| -|`withMaxTotalConnections(int)`|200|Maximum number of connections| -|`withMaxPerRoute(int)`|20|Maximum number of connections per route| -|`withValidateAfterInactivity(int)`|5000|Time to maintain idol connections (in milliseconds)| +|Method Name|Default Value| Description | +|---|---|----------------------------------------------------| +|`withQueueCapacity(int)`|10000| Queue size for pending logEvents | +|`withNumWorkers(int)`|2| Number of worker threads | +|`withMaxTotalConnections(int)`|200| Maximum number of connections | +|`withMaxPerRoute(int)`|20| Maximum number of connections per route | +|`withValidateAfterInactivity(int)`|5000| Time to maintain idle connections (in milliseconds) | ### Advanced configuration The following properties can be set to override the default configuration. -|Property Name|Default Value|Description| -|---|---|---| -|**async.event.handler.queue.capacity**|10000|Queue size for pending logEvents| -|**async.event.handler.num.workers**|2|Number of worker threads| -|**async.event.handler.max.connections**|200|Maximum number of connections| -|**async.event.handler.event.max.per.route**|20|Maximum number of connections per route| -|**async.event.handler.validate.after**|5000|Time to maintain idol connections (in milliseconds)| +|Property Name|Default Value| Description | +|---|---|----------------------------------------------------| +|**async.event.handler.queue.capacity**|10000| Queue size for pending logEvents | +|**async.event.handler.num.workers**|2| Number of worker threads | +|**async.event.handler.max.connections**|200| Maximum number of connections | +|**async.event.handler.event.max.per.route**|20| Maximum number of connections per route | +|**async.event.handler.validate.after**|5000| Time to maintain idle connections (in milliseconds) | ## HttpProjectConfigManager diff --git a/core-httpclient-impl/build.gradle b/core-httpclient-impl/build.gradle index b43c7026..b44733ff 100644 --- a/core-httpclient-impl/build.gradle +++ b/core-httpclient-impl/build.gradle @@ -4,6 +4,11 @@ dependencies { compileOnly group: 'com.google.code.gson', name: 'gson', version: gsonVersion compile group: 'org.apache.httpcomponents', name: 'httpclient', version: httpClientVersion + + testImplementation 'io.javalin:javalin:3.13.10' + testImplementation 'org.junit.jupiter:junit-jupiter-api:5.7.2' + testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.2' + testImplementation 'io.rest-assured:rest-assured:4.4.0' } task exhaustiveTest { diff --git a/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java b/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java index dc786c4f..88cc775d 100644 --- a/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java +++ b/core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java @@ -26,7 +26,9 @@ public final class HttpClientUtils { public static final int CONNECTION_TIMEOUT_MS = 10000; public static final int CONNECTION_REQUEST_TIMEOUT_MS = 5000; public static final int SOCKET_TIMEOUT_MS = 10000; - + public static final int DEFAULT_VALIDATE_AFTER_INACTIVITY = 5000; + public static final int DEFAULT_MAX_CONNECTIONS = 200; + public static final int DEFAULT_MAX_PER_ROUTE = 20; private static RequestConfig requestConfigWithTimeout; private HttpClientUtils() { diff --git a/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java b/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java index 363bce59..0e026f3e 100644 --- a/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java +++ b/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java @@ -17,11 +17,15 @@ package com.optimizely.ab; import com.optimizely.ab.annotations.VisibleForTesting; +import com.optimizely.ab.HttpClientUtils; + import org.apache.http.client.HttpClient; +import org.apache.http.client.HttpRequestRetryHandler; import org.apache.http.client.ResponseHandler; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.DefaultHttpRequestRetryHandler; import org.apache.http.impl.client.HttpClientBuilder; import org.apache.http.impl.client.HttpClients; import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; @@ -73,16 +77,18 @@ public static class Builder { // The following static values are public so that they can be tweaked if necessary. // These are the recommended settings for http protocol. https://hc.apache.org/httpcomponents-client-ga/tutorial/html/connmgmt.html // The maximum number of connections allowed across all routes. - private int maxTotalConnections = 200; + int maxTotalConnections = HttpClientUtils.DEFAULT_MAX_CONNECTIONS; // The maximum number of connections allowed for a route - private int maxPerRoute = 20; + int maxPerRoute = HttpClientUtils.DEFAULT_MAX_PER_ROUTE; // Defines period of inactivity in milliseconds after which persistent connections must be re-validated prior to being leased to the consumer. - private int validateAfterInactivity = 5000; + int validateAfterInactivity = HttpClientUtils.DEFAULT_VALIDATE_AFTER_INACTIVITY; // force-close the connection after this idle time (with 0, eviction is disabled by default) long evictConnectionIdleTimePeriod = 0; + HttpRequestRetryHandler customRetryHandler = null; TimeUnit evictConnectionIdleTimeUnit = TimeUnit.MILLISECONDS; private int timeoutMillis = HttpClientUtils.CONNECTION_TIMEOUT_MS; + private Builder() { } @@ -107,6 +113,12 @@ public Builder withEvictIdleConnections(long maxIdleTime, TimeUnit maxIdleTimeUn this.evictConnectionIdleTimeUnit = maxIdleTimeUnit; return this; } + + // customize retryHandler (DefaultHttpRequestRetryHandler will be used by default) + public Builder withRetryHandler(HttpRequestRetryHandler retryHandler) { + this.customRetryHandler = retryHandler; + return this; + } public Builder setTimeoutMillis(int timeoutMillis) { this.timeoutMillis = timeoutMillis; @@ -124,6 +136,9 @@ public OptimizelyHttpClient build() { .setConnectionManager(poolingHttpClientConnectionManager) .disableCookieManagement() .useSystemProperties(); + if (customRetryHandler != null) { + builder.setRetryHandler(customRetryHandler); + } logger.debug("Creating HttpClient with timeout: " + timeoutMillis); diff --git a/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java b/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java index 434b2910..7b550b2c 100644 --- a/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java +++ b/core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java @@ -16,6 +16,7 @@ */ package com.optimizely.ab.event; +import com.optimizely.ab.HttpClientUtils; import com.optimizely.ab.NamedThreadFactory; import com.optimizely.ab.OptimizelyHttpClient; import com.optimizely.ab.annotations.VisibleForTesting; @@ -31,6 +32,7 @@ import org.apache.http.client.methods.HttpRequestBase; import org.apache.http.client.utils.URIBuilder; import org.apache.http.entity.StringEntity; +import org.apache.http.impl.client.DefaultHttpRequestRetryHandler; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -45,7 +47,6 @@ import java.util.concurrent.TimeUnit; import javax.annotation.CheckForNull; -import javax.annotation.Nullable; /** * {@link EventHandler} implementation that queues events and has a separate pool of threads responsible @@ -61,9 +62,7 @@ public class AsyncEventHandler implements EventHandler, AutoCloseable { public static final int DEFAULT_QUEUE_CAPACITY = 10000; public static final int DEFAULT_NUM_WORKERS = 2; - public static final int DEFAULT_MAX_CONNECTIONS = 200; - public static final int DEFAULT_MAX_PER_ROUTE = 20; - public static final int DEFAULT_VALIDATE_AFTER_INACTIVITY = 5000; + private static final Logger logger = LoggerFactory.getLogger(AsyncEventHandler.class); private static final ProjectConfigResponseHandler EVENT_RESPONSE_HANDLER = new ProjectConfigResponseHandler(); @@ -135,15 +134,17 @@ public AsyncEventHandler(int queueCapacity, if (httpClient != null) { this.httpClient = httpClient; } else { - maxConnections = validateInput("maxConnections", maxConnections, DEFAULT_MAX_CONNECTIONS); - connectionsPerRoute = validateInput("connectionsPerRoute", connectionsPerRoute, DEFAULT_MAX_PER_ROUTE); - validateAfter = validateInput("validateAfter", validateAfter, DEFAULT_VALIDATE_AFTER_INACTIVITY); + maxConnections = validateInput("maxConnections", maxConnections, HttpClientUtils.DEFAULT_MAX_CONNECTIONS); + connectionsPerRoute = validateInput("connectionsPerRoute", connectionsPerRoute, HttpClientUtils.DEFAULT_MAX_PER_ROUTE); + validateAfter = validateInput("validateAfter", validateAfter, HttpClientUtils.DEFAULT_VALIDATE_AFTER_INACTIVITY); this.httpClient = OptimizelyHttpClient.builder() .withMaxTotalConnections(maxConnections) .withMaxPerRoute(connectionsPerRoute) .withValidateAfterInactivity(validateAfter) // infrequent event discards observed. staled connections force-closed after a long idle time. .withEvictIdleConnections(1L, TimeUnit.MINUTES) + // enable retry on POST + .withRetryHandler(new DefaultHttpRequestRetryHandler(3, true)) .build(); } @@ -310,9 +311,9 @@ public static class Builder { int queueCapacity = PropertyUtils.getInteger(CONFIG_QUEUE_CAPACITY, DEFAULT_QUEUE_CAPACITY); int numWorkers = PropertyUtils.getInteger(CONFIG_NUM_WORKERS, DEFAULT_NUM_WORKERS); - int maxTotalConnections = PropertyUtils.getInteger(CONFIG_MAX_CONNECTIONS, DEFAULT_MAX_CONNECTIONS); - int maxPerRoute = PropertyUtils.getInteger(CONFIG_MAX_PER_ROUTE, DEFAULT_MAX_PER_ROUTE); - int validateAfterInactivity = PropertyUtils.getInteger(CONFIG_VALIDATE_AFTER_INACTIVITY, DEFAULT_VALIDATE_AFTER_INACTIVITY); + int maxTotalConnections = PropertyUtils.getInteger(CONFIG_MAX_CONNECTIONS, HttpClientUtils.DEFAULT_MAX_CONNECTIONS); + int maxPerRoute = PropertyUtils.getInteger(CONFIG_MAX_PER_ROUTE, HttpClientUtils.DEFAULT_MAX_PER_ROUTE); + int validateAfterInactivity = PropertyUtils.getInteger(CONFIG_VALIDATE_AFTER_INACTIVITY, HttpClientUtils.DEFAULT_VALIDATE_AFTER_INACTIVITY); private long closeTimeout = Long.MAX_VALUE; private TimeUnit closeTimeoutUnit = TimeUnit.MILLISECONDS; private OptimizelyHttpClient httpClient; diff --git a/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java b/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java index 4667bec3..15a15d21 100644 --- a/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java +++ b/core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java @@ -16,27 +16,29 @@ */ package com.optimizely.ab; +import io.javalin.Javalin; +import org.apache.http.NoHttpResponseException; +import org.apache.http.client.HttpRequestRetryHandler; import org.apache.http.client.ResponseHandler; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.client.methods.RequestBuilder; import org.apache.http.conn.HttpHostConnectException; import org.apache.http.impl.client.CloseableHttpClient; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; +import org.apache.http.impl.client.DefaultHttpRequestRetryHandler; +import org.junit.*; import java.io.IOException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import static com.optimizely.ab.OptimizelyHttpClient.builder; import static java.util.concurrent.TimeUnit.*; import static org.junit.Assert.*; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.*; public class OptimizelyHttpClientTest { - @Before public void setUp() { System.setProperty("https.proxyHost", "localhost"); @@ -51,7 +53,13 @@ public void tearDown() { @Test public void testDefaultConfiguration() { - OptimizelyHttpClient optimizelyHttpClient = builder().build(); + OptimizelyHttpClient.Builder builder = builder(); + assertEquals(builder.validateAfterInactivity, 5000); + assertEquals(builder.maxTotalConnections, 200); + assertEquals(builder.maxPerRoute, 20); + assertNull(builder.customRetryHandler); + + OptimizelyHttpClient optimizelyHttpClient = builder.build(); assertTrue(optimizelyHttpClient.getHttpClient() instanceof CloseableHttpClient); } @@ -101,4 +109,76 @@ public void testExecute() throws IOException { OptimizelyHttpClient optimizelyHttpClient = new OptimizelyHttpClient(mockHttpClient); assertTrue(optimizelyHttpClient.execute(httpUriRequest, responseHandler)); } + + @Test + public void testRetries() throws IOException { + // Javalin intercepts before proxy, so host and port should be set correct here + String host = "http://localhost"; + int port = 8000; + Javalin app = Javalin.create().start(port); + int maxFailures = 2; + + AtomicInteger callTimes = new AtomicInteger(); + app.get("/", ctx -> { + callTimes.addAndGet(1); + int count = callTimes.get(); + if (count < maxFailures) { + throw new NoHttpResponseException("TESTING CONNECTION FAILURE"); + } else { + ctx.status(200).result("Success"); + } + }); + + OptimizelyHttpClient optimizelyHttpClient = spy(OptimizelyHttpClient.builder().build()); + optimizelyHttpClient.execute(new HttpGet(host + ":" + String.valueOf(port))); + assertEquals(3, callTimes.get()); + } + + @Test + public void testRetriesWithCustom() throws IOException { + // Javalin intercepts before proxy, so host and port should be set correct here + String host = "http://localhost"; + int port = 8000; + Javalin app = Javalin.create().start(port); + int maxFailures = 2; + + AtomicInteger callTimes = new AtomicInteger(); + app.get("/", ctx -> { + callTimes.addAndGet(1); + int count = callTimes.get(); + if (count < maxFailures) { +// throw new NoHttpResponseException("TESTING CONNECTION FAILURE"); + throw new IOException("TESTING CONNECTION FAILURE"); + +// ctx.status(500).result("TESTING Server Error"); + } else { + ctx.status(200).result("Success"); + } + }); + + HttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler(3, true); + OptimizelyHttpClient optimizelyHttpClient = spy(OptimizelyHttpClient.builder().withRetryHandler(retryHandler).build()); + + optimizelyHttpClient.execute(new HttpGet(host + ":" + String.valueOf(port))); + assertEquals(3, callTimes.get()); + } + +// +// @Test +// public void testRetriesWithCustom() throws IOException { +// CloseableHttpClient httpClient = mock(CloseableHttpClient.class); +// CloseableHttpResponse response = mock(CloseableHttpResponse.class); +// +// HttpRequestRetryHandler mockRetryHandler = spy(new DefaultHttpRequestRetryHandler(3, true)); +// when(mockRetryHandler.retryRequest(any(), any(), any())).thenReturn(true); +// +// OptimizelyHttpClient optimizelyHttpClient = OptimizelyHttpClient.builder().withRetryHandler(mockRetryHandler).build(); +// try { +// optimizelyHttpClient.execute(new HttpGet("https://example.com")); +// } catch(Exception e) { +// assert(e instanceof IOException); +// } +// verify(mockRetryHandler, times(3)).retryRequest(any(), any(), any()); +// } + } diff --git a/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java b/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java index f87811b9..34a15ef8 100644 --- a/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java +++ b/core-httpclient-impl/src/test/java/com/optimizely/ab/event/AsyncEventHandlerTest.java @@ -124,6 +124,7 @@ public void testBuilderWithCustomHttpClient() { AsyncEventHandler eventHandler = builder() .withOptimizelyHttpClient(customHttpClient) + // these params will be ignored when customHttpClient is injected .withMaxTotalConnections(1) .withMaxPerRoute(2) .withCloseTimeout(10, TimeUnit.SECONDS) @@ -134,6 +135,17 @@ public void testBuilderWithCustomHttpClient() { @Test public void testBuilderWithDefaultHttpClient() { + AsyncEventHandler.Builder builder = builder(); + assertEquals(builder.validateAfterInactivity, 5000); + assertEquals(builder.maxTotalConnections, 200); + assertEquals(builder.maxPerRoute, 20); + + AsyncEventHandler eventHandler = builder.build(); + assert(eventHandler.httpClient != null); + } + + @Test + public void testBuilderWithDefaultHttpClientAndCustomParams() { AsyncEventHandler eventHandler = builder() .withMaxTotalConnections(3) .withMaxPerRoute(4)