reduce request drops with smaller validation time and retries

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 idle 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 idle 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
 

core-httpclient-impl/build.gradle

@@ -1,14 +1,9 @@
 dependencies {
-    compile project(':core-api')
-
+    implementation project(':core-api')
     compileOnly group: 'com.google.code.gson', name: 'gson', version: gsonVersion
+    implementation group: 'org.apache.httpcomponents', name: 'httpclient', version: httpClientVersion
 
-    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'
+    testImplementation 'org.mock-server:mockserver-netty:5.15.0'
 }
 
 task exhaustiveTest {

core-httpclient-impl/src/main/java/com/optimizely/ab/HttpClientUtils.java

@@ -26,7 +26,7 @@ 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_VALIDATE_AFTER_INACTIVITY = 1000;
     public static final int DEFAULT_MAX_CONNECTIONS = 200;
     public static final int DEFAULT_MAX_PER_ROUTE = 20;
     private static RequestConfig requestConfigWithTimeout;

core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyHttpClient.java

@@ -81,6 +81,8 @@ public static class Builder {
         // The maximum number of connections allowed for a route
         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.
+        // If this is too long, it's expected to see more requests dropped on staled connections (dropped by the server or networks).
+        // We can configure retries (POST for AsyncEventDispatcher) to cover the staled connections.
         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;

core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java

@@ -143,7 +143,7 @@ public AsyncEventHandler(int queueCapacity,
                 .withValidateAfterInactivity(validateAfter)
                 // infrequent event discards observed. staled connections force-closed after a long idle time.
                 .withEvictIdleConnections(1L, TimeUnit.MINUTES)
-                // enable retry on POST
+                // enable retry on event POST (default: retry on GET only)
                 .withRetryHandler(new DefaultHttpRequestRetryHandler(3, true))
                 .build();
         }

core-httpclient-impl/src/test/java/com/optimizely/ab/OptimizelyHttpClientTest.java

@@ -16,27 +16,37 @@
  */
 package com.optimizely.ab;
 
-import io.javalin.Javalin;
-import org.apache.http.NoHttpResponseException;
+import org.apache.http.HttpException;
 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.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.apache.http.impl.client.DefaultHttpRequestRetryHandler;
+import org.apache.http.protocol.HttpContext;
 import org.junit.*;
+import org.mockserver.integration.ClientAndServer;
+import org.mockserver.model.ConnectionOptions;
+import org.mockserver.model.HttpError;
+import org.mockserver.model.HttpRequest;
+import org.mockserver.model.HttpResponse;
 
 import java.io.IOException;
+import java.util.concurrent.ExecutionException;
 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.Matchers.any;
 import static org.mockito.Mockito.*;
+import static org.mockserver.model.HttpForward.forward;
+import static org.mockserver.model.HttpRequest.request;
+import static org.mockserver.model.HttpResponse.*;
+import static org.mockserver.model.HttpResponse.response;
+import static org.mockserver.verify.VerificationTimes.exactly;
 
 public class OptimizelyHttpClientTest {
     @Before
@@ -111,74 +121,72 @@ public void testExecute() throws IOException {
     }
 
     @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");
-            }
-        });
+    public void testRetriesWithCustomRetryHandler() throws IOException {
 
-        OptimizelyHttpClient optimizelyHttpClient = spy(OptimizelyHttpClient.builder().build());
-        optimizelyHttpClient.execute(new HttpGet(host + ":" + String.valueOf(port)));
-        assertEquals(3, callTimes.get());
-    }
+        // [NOTE] Request retries are all handled inside HttpClient. Not easy for unit test.
+        //  - "DefaultHttpRetryHandler" in HttpClient retries only with special types of Exceptions
+        //    like "NoHttpResponseException", etc.
+        //    Other exceptions (SocketTimeout, ProtocolException, etc.) all ignored.
+        //  - Not easy to force the specific exception type in the low-level.
+        //  - This test just validates custom retry handler injected ok by validating the number of retries.
 
-    @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");
+        class CustomRetryHandler implements HttpRequestRetryHandler {
+            private final int maxRetries;
+
+            public CustomRetryHandler(int maxRetries) {
+                this.maxRetries = maxRetries;
             }
-        });
 
-        HttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler(3, true);
-        OptimizelyHttpClient optimizelyHttpClient = spy(OptimizelyHttpClient.builder().withRetryHandler(retryHandler).build());
+            @Override
+            public boolean retryRequest(IOException exception, int executionCount, HttpContext context) {
+                // override to retry for any type of exceptions
+                return executionCount < maxRetries;
+            }
+        }
 
-        optimizelyHttpClient.execute(new HttpGet(host + ":" + String.valueOf(port)));
-        assertEquals(3, callTimes.get());
-    }
+        int port = 9999;
+        ClientAndServer mockServer;
+        int retryCount;
 
-//
-//    @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());
-// }
+        // default httpclient (retries enabled by default, but no retry for timeout connection)
 
+        mockServer = ClientAndServer.startClientAndServer(port);
+        mockServer
+            .when(request().withMethod("GET").withPath("/"))
+            .error(HttpError.error());
+
+        OptimizelyHttpClient clientDefault = OptimizelyHttpClient.builder()
+            .setTimeoutMillis(100)
+            .build();
+
+        try {
+            clientDefault.execute(new HttpGet("http://localhost:" + port));
+            fail();
+        } catch (Exception e) {
+            retryCount = mockServer.retrieveRecordedRequests(request()).length;
+            assertEquals(1, retryCount);
+        }
+        mockServer.stop();
+
+        // httpclient with custom retry handler (5 times retries for any request)
+
+        mockServer = ClientAndServer.startClientAndServer(port);
+        mockServer
+            .when(request().withMethod("GET").withPath("/"))
+            .error(HttpError.error());
+
+        OptimizelyHttpClient clientWithRetries = OptimizelyHttpClient.builder()
+            .withRetryHandler(new CustomRetryHandler(5))
+            .setTimeoutMillis(100)
+            .build();
+
+        try {
+            clientWithRetries.execute(new HttpGet("http://localhost:" + port));
+            fail();
+        } catch (Exception e) {
+            retryCount = mockServer.retrieveRecordedRequests(request()).length;
+            assertEquals(5, retryCount);
+        }
+        mockServer.stop();
+    }
 }