Merge pull request #75 from launchdarkly/eb/ch18901/4xx-errors

treat most 4xx errors as unrecoverable

Author: eli-darkly

src/main/java/com/launchdarkly/client/DefaultEventProcessor.java

@@ -25,6 +25,8 @@
 import java.util.concurrent.atomic.AtomicLong;
 
 import static com.launchdarkly.client.Util.getRequestBuilder;
+import static com.launchdarkly.client.Util.httpErrorMessage;
+import static com.launchdarkly.client.Util.isHttpErrorRecoverable;
 
 import okhttp3.MediaType;
 import okhttp3.Request;
@@ -386,9 +388,12 @@ private void handleResponse(Response response) {
         } catch (ParseException e) {
         }
       }
-      if (response.code() == 401) {
+      if (!isHttpErrorRecoverable(response.code())) {
         disabled.set(true);
-        logger.error("Received 401 error, no further events will be posted since SDK key is invalid");
+        logger.error(httpErrorMessage(response.code(), "posting events", "some events were dropped"));
+        // It's "some events were dropped" because we're not going to retry *this* request any more times -
+        // we only get to this point if we have used up our retry attempts. So the last batch of events was
+        // lost, even though we will still try to post *other* events in the future.
       }
     }
   }
@@ -530,7 +535,7 @@ private void postEvents(List<EventOutput> eventsOut) {
           logger.debug("Event delivery took {} ms, response status {}", endTime - startTime, response.code());
           if (!response.isSuccessful()) {
             logger.warn("Unexpected response status when posting events: {}", response.code());
-            if (response.code() >= 500) {
+            if (isHttpErrorRecoverable(response.code())) {
               continue;
             }
           }

src/main/java/com/launchdarkly/client/FeatureRequestor.java

@@ -37,27 +37,27 @@ static class AllData {
     this.config = config;
   }
 
-  Map<String, FeatureFlag> getAllFlags() throws IOException, InvalidSDKKeyException {
+  Map<String, FeatureFlag> getAllFlags() throws IOException, HttpErrorException {
     String body = get(GET_LATEST_FLAGS_PATH);
     return FeatureFlag.fromJsonMap(config, body);
   }
 
-  FeatureFlag getFlag(String featureKey) throws IOException, InvalidSDKKeyException {
+  FeatureFlag getFlag(String featureKey) throws IOException, HttpErrorException {
     String body = get(GET_LATEST_FLAGS_PATH + "/" + featureKey);
     return FeatureFlag.fromJson(config, body);
   }
 
-  Map<String, Segment> getAllSegments() throws IOException, InvalidSDKKeyException {
+  Map<String, Segment> getAllSegments() throws IOException, HttpErrorException {
     String body = get(GET_LATEST_SEGMENTS_PATH);
     return Segment.fromJsonMap(config, body);
   }
 
-  Segment getSegment(String segmentKey) throws IOException, InvalidSDKKeyException {
+  Segment getSegment(String segmentKey) throws IOException, HttpErrorException {
     String body = get(GET_LATEST_SEGMENTS_PATH + "/" + segmentKey);
     return Segment.fromJson(config, body);
   }
 
-  AllData getAllData() throws IOException, InvalidSDKKeyException {
+  AllData getAllData() throws IOException, HttpErrorException {
     String body = get(GET_LATEST_ALL_PATH);
     return config.gson.fromJson(body, AllData.class);
   }
@@ -69,7 +69,7 @@ AllData getAllData() throws IOException, InvalidSDKKeyException {
     return ret;
   }
 
-  private String get(String path) throws IOException, InvalidSDKKeyException {
+  private String get(String path) throws IOException, HttpErrorException {
     Request request = getRequestBuilder(sdkKey)
         .url(config.baseURI.toString() + path)
         .get()
@@ -81,12 +81,7 @@ private String get(String path) throws IOException, InvalidSDKKeyException {
       String body = response.body().string();
 
       if (!response.isSuccessful()) {
-        if (response.code() == 401) {
-          logger.error("[401] Invalid SDK key when accessing URI: " + request.url());
-          throw new InvalidSDKKeyException();
-        }
-        throw new IOException("Unexpected response when retrieving Feature Flag(s): " + response + " using url: "
-            + request.url() + " with body: " + body);
+        throw new HttpErrorException(response.code());
       }
       logger.debug("Get flag(s) response: " + response.toString() + " with body: " + body);
       logger.debug("Network response: " + response.networkResponse());
@@ -98,10 +93,4 @@ private String get(String path) throws IOException, InvalidSDKKeyException {
       return body;
     }
   }
-  
-  @SuppressWarnings("serial")
-  public static class InvalidSDKKeyException extends Exception {
-    public InvalidSDKKeyException() {
-    }
-  }
 }

src/main/java/com/launchdarkly/client/HttpErrorException.java

@@ -0,0 +1,15 @@
+package com.launchdarkly.client;
+
+@SuppressWarnings("serial")
+class HttpErrorException extends Exception {
+  private final int status;
+  
+  public HttpErrorException(int status) {
+    super("HTTP error " + status);
+    this.status = status;
+  }
+  
+  public int getStatus() {
+    return status;
+  }
+}

src/main/java/com/launchdarkly/client/LDConfig.java

@@ -113,7 +113,7 @@ protected LDConfig(Builder builder) {
         .connectTimeout(connectTimeoutMillis, TimeUnit.MILLISECONDS)
         .readTimeout(socketTimeoutMillis, TimeUnit.MILLISECONDS)
         .writeTimeout(socketTimeoutMillis, TimeUnit.MILLISECONDS)
-        .retryOnConnectionFailure(true);
+        .retryOnConnectionFailure(false); // we will implement our own retry logic
 
     // When streaming is enabled, http GETs made by FeatureRequester will
     // always guarantee a new flag state. So, disable http response caching

src/main/java/com/launchdarkly/client/PollingProcessor.java

@@ -2,13 +2,21 @@
 
 import com.google.common.util.concurrent.SettableFuture;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
-import java.util.concurrent.*;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import static com.launchdarkly.client.Util.httpErrorMessage;
+import static com.launchdarkly.client.Util.isHttpErrorRecoverable;
+
 class PollingProcessor implements UpdateProcessor {
   private static final Logger logger = LoggerFactory.getLogger(PollingProcessor.class);
 
@@ -55,10 +63,12 @@ public void run() {
             logger.info("Initialized LaunchDarkly client.");
             initFuture.set(null);
           }
-        } catch (FeatureRequestor.InvalidSDKKeyException e) {
-          logger.error("Received 401 error, no further polling requests will be made since SDK key is invalid");
-          scheduler.shutdown();
-          initFuture.set(null); // if client is initializing, make it stop waiting; has no effect if already inited
+        } catch (HttpErrorException e) {
+          logger.error(httpErrorMessage(e.getStatus(), "polling request", "will retry"));
+          if (!isHttpErrorRecoverable(e.getStatus())) {
+            scheduler.shutdown();
+            initFuture.set(null); // if client is initializing, make it stop waiting; has no effect if already inited
+          }
         } catch (IOException e) {
           logger.error("Encountered exception in LaunchDarkly client when retrieving update", e);
         }

src/main/java/com/launchdarkly/client/StreamProcessor.java

@@ -17,6 +17,8 @@
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import static com.launchdarkly.client.Util.httpErrorMessage;
+import static com.launchdarkly.client.Util.isHttpErrorRecoverable;
 import static com.launchdarkly.client.VersionedDataKind.FEATURES;
 import static com.launchdarkly.client.VersionedDataKind.SEGMENTS;
 
@@ -65,11 +67,13 @@ public Future<Void> start() {
     ConnectionErrorHandler connectionErrorHandler = new ConnectionErrorHandler() {
       @Override
       public Action onConnectionError(Throwable t) {
-        if ((t instanceof UnsuccessfulResponseException) &&
-            ((UnsuccessfulResponseException) t).getCode() == 401) {
-          logger.error("Received 401 error, no further streaming connection will be made since SDK key is invalid");
-          initFuture.set(null); // if client is initializing, make it stop waiting; has no effect if already inited
-          return Action.SHUTDOWN;
+        if (t instanceof UnsuccessfulResponseException) {
+          int status = ((UnsuccessfulResponseException)t).getCode();
+          logger.error(httpErrorMessage(status, "streaming connection", "will retry"));
+          if (!isHttpErrorRecoverable(status)) {
+            initFuture.set(null); // if client is initializing, make it stop waiting; has no effect if already inited
+            return Action.SHUTDOWN;
+          }
         }
         return Action.PROCEED;
       }

src/main/java/com/launchdarkly/client/Util.java

@@ -32,4 +32,42 @@ static Request.Builder getRequestBuilder(String sdkKey) {
         .addHeader("Authorization", sdkKey)
         .addHeader("User-Agent", "JavaClient/" + LDClient.CLIENT_VERSION);
   }
+  
+  /**
+   * Tests whether an HTTP error status represents a condition that might resolve on its own if we retry.
+   * @param statusCode the HTTP status
+   * @return true if retrying makes sense; false if it should be considered a permanent failure
+   */
+  static boolean isHttpErrorRecoverable(int statusCode) {
+    if (statusCode >= 400 && statusCode < 500) {
+      switch (statusCode) {
+      case 408: // request timeout
+      case 429: // too many requests
+        return true;
+      default:
+        return false; // all other 4xx errors are unrecoverable
+      }
+    }
+    return true;
+  }
+  
+  /**
+   * Builds an appropriate log message for an HTTP error status.
+   * @param statusCode the HTTP status
+   * @param context description of what we were trying to do
+   * @param recoverableMessage description of our behavior if the error is recoverable; typically "will retry"
+   * @return a message string
+   */
+  static String httpErrorMessage(int statusCode, String context, String recoverableMessage) {
+    StringBuilder sb = new StringBuilder();
+    sb.append("Received HTTP error ").append(statusCode);
+    switch (statusCode) {
+    case 401:
+    case 403:
+      sb.append(" (invalid SDK key)");
+    }
+    sb.append(" for ").append(context).append(" - ");
+    sb.append(isHttpErrorRecoverable(statusCode) ? recoverableMessage : "giving up permanently");
+    return sb.toString();
+  }
 }

src/test/java/com/launchdarkly/client/DefaultEventProcessorTest.java

@@ -8,7 +8,6 @@
 import com.launchdarkly.client.DefaultEventProcessor.EventDispatcher;
 
 import org.hamcrest.Matcher;
-import org.hamcrest.Matchers;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -391,29 +390,60 @@ public void sdkKeyIsSent() throws Exception {
 
     assertThat(req.getHeader("Authorization"), equalTo(SDK_KEY));
   }
+
+  @Test
+  public void http401ErrorIsUnrecoverable() throws Exception {
+    testUnrecoverableHttpError(401);
+  }
+
+  @Test
+  public void http403ErrorIsUnrecoverable() throws Exception {
+    testUnrecoverableHttpError(403);
+  }
+
+  // Cannot test our retry logic for 408, because OkHttp insists on doing its own retry on 408 so that
+  // we never actually see that response status.
+//  @Test
+//  public void http408ErrorIsRecoverable() throws Exception {
+//    testRecoverableHttpError(408);
+//  }
+
+  @Test
+  public void http429ErrorIsRecoverable() throws Exception {
+    testRecoverableHttpError(429);
+  }
+
+  @Test
+  public void http500ErrorIsRecoverable() throws Exception {
+    testRecoverableHttpError(500);
+  }
 
   @Test
-  public void noMorePayloadsAreSentAfter401Error() throws Exception {
+  public void flushIsRetriedOnceAfter5xxError() throws Exception {
+  }
+  
+  private void testUnrecoverableHttpError(int status) throws Exception {
     ep = new DefaultEventProcessor(SDK_KEY, configBuilder.build());
     Event e = EventFactory.DEFAULT.newIdentifyEvent(user);
     ep.sendEvent(e);
-    flushAndGetEvents(new MockResponse().setResponseCode(401));
+    flushAndGetEvents(new MockResponse().setResponseCode(status));
 
     ep.sendEvent(e);
     ep.flush();
     ep.waitUntilInactive();
     RecordedRequest req = server.takeRequest(0, TimeUnit.SECONDS);
     assertThat(req, nullValue(RecordedRequest.class));
   }
-
-  @Test
-  public void flushIsRetriedOnceAfter5xxError() throws Exception {
+  
+  private void testRecoverableHttpError(int status) throws Exception {
     ep = new DefaultEventProcessor(SDK_KEY, configBuilder.build());
     Event e = EventFactory.DEFAULT.newIdentifyEvent(user);
     ep.sendEvent(e);
 
-    server.enqueue(new MockResponse().setResponseCode(503));
-    server.enqueue(new MockResponse().setResponseCode(503));
+    server.enqueue(new MockResponse().setResponseCode(status));
+    server.enqueue(new MockResponse().setResponseCode(status));
+    server.enqueue(new MockResponse());
+    // need two responses because flush will be retried one time
 
     ep.flush();
     ep.waitUntilInactive();