fix: handle non-json response bodies and retry on 5xx errors. (#85)

.github/workflows/pull-request-test.yml

@@ -13,5 +13,5 @@ jobs:
           distribution: 'zulu'
       - uses: actions/checkout@v2
       - name: Test with Gradle
-        run: gradle test:test --stacktrace
+        run: ./gradlew test:test --stacktrace
 

.github/workflows/semantic-pr.yml

@@ -7,7 +7,7 @@ on:
 jobs:
   pr-title-check:
     name: Check PR for semantic title
-    runs-on: ubuntu-18.04
+    runs-on: ubuntu-latest
     steps:
       - name: PR title is valid
         if: >

.gitignore

@@ -19,6 +19,9 @@
 *.tar.gz
 *.rar
 
+# Keep gradle wrapper
+!gradle/wrapper/gradle-wrapper.jar
+
 # virtual machine crash logs, see http://www.java.com/en/download/help/error_hotspot.xml
 hs_err_pid*
 

gradle/wrapper/gradle-wrapper.properties

@@ -1,5 +1,5 @@
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-6.8.3-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-6.8.3-all.zip
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists

src/main/java/com/amplitude/HttpCall.java

@@ -2,6 +2,7 @@
 
 import com.amplitude.exception.AmplitudeInvalidAPIKeyException;
 import org.json.JSONArray;
+import org.json.JSONException;
 import org.json.JSONObject;
 
 import javax.net.ssl.HttpsURLConnection;
@@ -89,6 +90,12 @@ protected Response makeRequest(List<Event> events) throws AmplitudeInvalidAPIKey
       timesOutResponse.put("status", Status.TIMEOUT);
       timesOutResponse.put("code", 408);
       responseBody = Response.populateResponse(timesOutResponse);
+    } catch (JSONException e) {
+      // Some error responses from load balancers and reverse proxies may have
+      // response bodies that are not JSON (e.g. HTML, XML).
+      JSONObject decodeFailureResponse = new JSONObject();
+      decodeFailureResponse.put("code", responseCode);
+      responseBody = Response.populateResponse(decodeFailureResponse);
     } finally {
       if (inputStream != null) {
         try {

src/main/java/com/amplitude/HttpTransport.java

@@ -199,8 +199,7 @@ private EventsRetryResult retryEventsOnce(String userId, String deviceId, List<E
         triggerEventCallbacks(events, response.code, "Unknown response status.");
         break;
       case FAILED:
-        shouldRetry = false;
-        triggerEventCallbacks(events, response.code, "Event sent Failed.");
+        shouldRetry = true;
         break;
       default:
         break;
@@ -266,7 +265,8 @@ protected boolean shouldRetryForStatus(Status status) {
     return (status == Status.INVALID
         || status == Status.PAYLOAD_TOO_LARGE
         || status == Status.RATELIMIT
-        || status == Status.TIMEOUT);
+        || status == Status.TIMEOUT
+        || status == Status.FAILED);
   }
 
   private void triggerEventCallbacks(List<Event> events, int status, String message) {

src/test/java/com/amplitude/HttpCallTest.java

@@ -268,6 +268,22 @@ public void testHttpCallWithCustomProxy(HttpCallMode httpCallMode, URL url)
     assertTrue(connection.usingProxy());
   }
 
+  @ParameterizedTest
+  @MethodSource("httpCallArguments")
+  public void testHttpCallWithNonJsonResponseBody(HttpCallMode httpCallMode, URL url)
+          throws IOException, AmplitudeInvalidAPIKeyException {
+    HttpsURLConnection connection =
+            MockHttpsURLConnectionHelper.getMockHttpsURLConnection(502, "<Response></Response>");
+    mockURLStreamHandler.setConnection(url, connection);
+    HttpCall httpCall = getHttpCallFromCallMode(httpCallMode, null, new Proxy(Proxy.Type.HTTP, new InetSocketAddress("0.0.0.0", 443)));
+    List<Event> events = EventsGenerator.generateEvents(1);
+    Response response = httpCall.makeRequest(events);
+
+    assertEquals(502, response.code);
+    assertEquals(Status.FAILED, response.status);
+    verifyConnectionOption(connection);
+  }
+
   static Stream<Arguments> httpCallArguments() {
     return Stream.of(
         arguments(HttpCallMode.REGULAR, Constants.API_URL),

src/test/java/com/amplitude/HttpTransportTest.java

@@ -42,7 +42,7 @@ public void setUp() {
     "RATELIMIT, true",
     "PAYLOAD_TOO_LARGE, true",
     "TIMEOUT, true",
-    "FAILED, false",
+    "FAILED, true",
     "UNKNOWN, false"
   })
   public void testShouldRetryForStatus(Status status, boolean expected) {
@@ -55,9 +55,10 @@ public void testRetryEvents() throws AmplitudeInvalidAPIKeyException, Interrupte
     Response payloadTooLargeResponse = ResponseUtil.getPayloadTooLargeResponse();
     Response invalidResponse = ResponseUtil.getInvalidResponse(false);
     Response rateLimitResponse = ResponseUtil.getRateLimitResponse(false);
+    Response failedResponse = ResponseUtil.getFailedResponse();
 
     HttpCall httpCall = mock(HttpCall.class);
-    CountDownLatch latch = new CountDownLatch(4);
+    CountDownLatch latch = new CountDownLatch(5);
     CountDownLatch latch2 = new CountDownLatch(10);
     when(httpCall.makeRequest(anyList()))
         .thenAnswer(
@@ -75,6 +76,11 @@ public void testRetryEvents() throws AmplitudeInvalidAPIKeyException, Interrupte
               latch.countDown();
               return payloadTooLargeResponse;
             })
+        .thenAnswer(
+            invocation -> {
+                latch.countDown();
+                return failedResponse;
+            })
         .thenAnswer(
             invocation -> {
               latch.countDown();
@@ -94,9 +100,9 @@ public void onLogEventServerResponse(Event event, int status, String message) {
     httpTransport.setHttpCall(httpCall);
     httpTransport.setCallbacks(callbacks);
     httpTransport.retryEvents(events, invalidResponse);
-    assertTrue(latch.await(1L, TimeUnit.SECONDS));
+    assertTrue(latch.await(2L, TimeUnit.SECONDS));
     assertTrue(latch2.await(1L, TimeUnit.SECONDS));
-    verify(httpCall, times(4)).makeRequest(anyList());
+    verify(httpCall, times(5)).makeRequest(anyList());
     for (int i = 0; i < events.size(); i++) {
       if (i < (events.size() / 4)) {
         assertEquals(200, resultMap.get(events.get(i)));
@@ -299,14 +305,20 @@ public void onLogEventServerResponse(Event event, int status, String message) {
   @Test
   public void testFailedResponse() throws AmplitudeInvalidAPIKeyException, InterruptedException {
     Response failedResponse = ResponseUtil.getFailedResponse();
+    Response successResponse = ResponseUtil.getSuccessResponse();
     HttpCall httpCall = mock(HttpCall.class);
-    CountDownLatch latch = new CountDownLatch(1);
+    CountDownLatch latch = new CountDownLatch(2);
     CountDownLatch latch2 = new CountDownLatch(10);
     when(httpCall.makeRequest(anyList()))
         .thenAnswer(
             invocation -> {
               latch.countDown();
               return failedResponse;
+            })
+        .thenAnswer(
+            invocation -> {
+                latch.countDown();
+                return successResponse;
             });
 
     List<Event> events = EventsGenerator.generateEvents(10);
@@ -324,9 +336,9 @@ public void onLogEventServerResponse(Event event, int status, String message) {
     httpTransport.sendEventsWithRetry(events);
     assertTrue(latch.await(1L, TimeUnit.SECONDS));
     assertTrue(latch2.await(1L, TimeUnit.SECONDS));
-    verify(httpCall, times(1)).makeRequest(anyList());
+    verify(httpCall, times(2)).makeRequest(anyList());
     for (Event event : events) {
-      assertEquals(500, resultMap.get(event));
+      assertEquals(200, resultMap.get(event));
     }
   }