Realtime RC - Check HTTP status before listening and retrying (#4121)

* Update client to check Http status codes before listening or connecting

* Fix tests and add more tests.

* Format files.

* Close error stream

* Update status code check and initialize response code to 0

* Change responseCode var to Integer and initialize to null

Author: qdpham13

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigAutoFetch.java

@@ -97,16 +97,9 @@ private String parseAndValidateConfigUpdateMessage(String message) {
   public void listenForNotifications() {
     if (httpURLConnection != null) {
       try {
-        int responseCode = httpURLConnection.getResponseCode();
-        if (responseCode == 200) {
-          InputStream inputStream = httpURLConnection.getInputStream();
-          handleNotifications(inputStream);
-          inputStream.close();
-        } else {
-          propagateErrors(
-              new FirebaseRemoteConfigRealtimeUpdateStreamException(
-                  "Http connection responded with error: " + responseCode));
-        }
+        InputStream inputStream = httpURLConnection.getInputStream();
+        handleNotifications(inputStream);
+        inputStream.close();
       } catch (IOException ex) {
         propagateErrors(
             new FirebaseRemoteConfigRealtimeUpdateFetchException(

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java

@@ -16,6 +16,7 @@
 
 import static com.google.firebase.remoteconfig.FirebaseRemoteConfig.TAG;
 import static com.google.firebase.remoteconfig.RemoteConfigConstants.REALTIME_REGEX_URL;
+import static com.google.firebase.remoteconfig.internal.ConfigFetchHandler.HTTP_TOO_MANY_REQUESTS;
 
 import android.annotation.SuppressLint;
 import android.content.Context;
@@ -236,7 +237,9 @@ private URL getUrl() {
     return realtimeURL;
   }
 
-  private HttpURLConnection createRealtimeConnection() throws IOException {
+  /** Create HTTP connection and set headers. */
+  @SuppressLint("VisibleForTests")
+  public HttpURLConnection createRealtimeConnection() throws IOException {
     URL realtimeUrl = getUrl();
     HttpURLConnection httpURLConnection = (HttpURLConnection) realtimeUrl.openConnection();
     setCommonRequestHeaders(httpURLConnection);
@@ -245,8 +248,9 @@ private HttpURLConnection createRealtimeConnection() throws IOException {
     return httpURLConnection;
   }
 
-  // Try to reopen HTTP connection after a random amount of time
-  private synchronized void retryHTTPConnection() {
+  /** Retries HTTP stream connection asyncly in random time intervals. */
+  @SuppressLint("VisibleForTests")
+  public synchronized void retryHTTPConnection() {
     if (canMakeHttpStreamConnection() && httpRetriesRemaining > 0) {
       if (httpRetriesRemaining < ORIGINAL_RETRIES) {
         httpRetrySeconds *= getRetryMultiplier();
@@ -273,7 +277,12 @@ synchronized void stopRealtime() {
     scheduledExecutorService.shutdownNow();
   }
 
-  private synchronized ConfigAutoFetch startAutoFetch(HttpURLConnection httpURLConnection) {
+  /**
+   * Create Autofetch class that listens on HTTP stream for ConfigUpdate messages and calls Fetch
+   * accordingly.
+   */
+  @SuppressLint("VisibleForTests")
+  public synchronized ConfigAutoFetch startAutoFetch(HttpURLConnection httpURLConnection) {
     ConfigUpdateListener retryCallback =
         new ConfigUpdateListener() {
           @Override
@@ -298,6 +307,15 @@ public void onError(Exception error) {
     return new ConfigAutoFetch(httpURLConnection, configFetchHandler, listeners, retryCallback);
   }
 
+  // HTTP status code that the Realtime client should retry on.
+  private boolean isStatusCodeRetryable(int statusCode) {
+    return statusCode == HttpURLConnection.HTTP_CLIENT_TIMEOUT
+        || statusCode == HTTP_TOO_MANY_REQUESTS
+        || statusCode == HttpURLConnection.HTTP_BAD_GATEWAY
+        || statusCode == HttpURLConnection.HTTP_UNAVAILABLE
+        || statusCode == HttpURLConnection.HTTP_GATEWAY_TIMEOUT;
+  }
+
   /**
    * Open the realtime connection, begin listening for updates, and auto-fetch when an update is
    * received.
@@ -306,39 +324,55 @@ public void onError(Exception error) {
    * chunk-encoded HTTP body. When the connection closes, it attempts to reestablish the stream.
    */
   @SuppressLint("VisibleForTests")
-  synchronized void beginRealtimeHttpStream() {
+  public synchronized void beginRealtimeHttpStream() {
     if (!canMakeHttpStreamConnection()) {
       return;
     }
 
+    Integer responseCode = null;
     try {
       // Create the open the connection.
       httpURLConnection = createRealtimeConnection();
-      httpURLConnection.connect();
+      responseCode = httpURLConnection.getResponseCode();
 
-      // Reset the retries remaining if we opened the connection without an exception.
-      resetRetryParameters();
+      // If the connection returned a 200 response code, start listening for messages.
+      if (responseCode == HttpURLConnection.HTTP_OK) {
+        // Reset the retries remaining if we opened the connection without an exception.
+        resetRetryParameters();
 
-      // Start listening for realtime notifications.
-      ConfigAutoFetch configAutoFetch = startAutoFetch(httpURLConnection);
-      configAutoFetch.listenForNotifications();
+        // Start listening for realtime notifications.
+        ConfigAutoFetch configAutoFetch = startAutoFetch(httpURLConnection);
+        configAutoFetch.listenForNotifications();
+      }
     } catch (IOException e) {
       Log.d(TAG, "Exception connecting to realtime stream. Retrying the connection...");
     } finally {
       closeRealtimeHttpStream();
-      retryHTTPConnection();
+
+      // If responseCode is null then no connection was made to server and the SDK should still
+      // retry.
+      if (responseCode == null
+          || responseCode == HttpURLConnection.HTTP_OK
+          || isStatusCodeRetryable(responseCode)) {
+        retryHTTPConnection();
+      } else {
+        propagateErrors(
+            new FirebaseRemoteConfigRealtimeUpdateStreamException(
+                "The server returned a status code that is not retryable. Realtime is shutting down."));
+      }
     }
   }
 
   // Pauses Http stream listening
-  synchronized void closeRealtimeHttpStream() {
+  public synchronized void closeRealtimeHttpStream() {
     if (httpURLConnection != null) {
       this.httpURLConnection.disconnect();
 
       // Explicitly close the input stream due to a bug in the Android okhttp implementation.
       // See github.com/firebase/firebase-android-sdk/pull/808.
       try {
         this.httpURLConnection.getInputStream().close();
+        this.httpURLConnection.getErrorStream().close();
       } catch (IOException e) {
       }
       this.httpURLConnection = null;

firebase-config/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigTest.java

@@ -30,8 +30,11 @@
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -65,6 +68,7 @@
 import com.google.firebase.remoteconfig.internal.ConfigGetParameterHandler;
 import com.google.firebase.remoteconfig.internal.ConfigMetadataClient;
 import com.google.firebase.remoteconfig.internal.ConfigRealtimeHandler;
+import com.google.firebase.remoteconfig.internal.ConfigRealtimeHttpClient;
 import com.google.firebase.remoteconfig.internal.Personalization;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
@@ -142,6 +146,7 @@ public final class FirebaseRemoteConfigTest {
   @Mock private ConfigMetadataClient metadataClient;
 
   @Mock private ConfigRealtimeHandler mockConfigRealtimeHandler;
+  @Mock private ConfigAutoFetch mockConfigAutoFetch;
   @Mock private ConfigUpdateListenerRegistration mockRealtimeRegistration;
   @Mock private HttpURLConnection mockHttpURLConnection;
   @Mock private ConfigUpdateListener mockListener;
@@ -167,6 +172,7 @@ public final class FirebaseRemoteConfigTest {
   private FetchResponse realtimeFetchedContainerResponse;
   private ConfigContainer realtimeFetchedContainer;
   private ConfigAutoFetch configAutoFetch;
+  private ConfigRealtimeHttpClient configRealtimeHttpClient;
 
   private FetchResponse firstFetchedContainerResponse;
 
@@ -271,6 +277,14 @@ public void setUp() throws Exception {
     listeners.add(mockListener);
     configAutoFetch =
         new ConfigAutoFetch(mockHttpURLConnection, mockFetchHandler, listeners, mockRetryListener);
+    configRealtimeHttpClient =
+        new ConfigRealtimeHttpClient(
+            firebaseApp,
+            mockFirebaseInstallations,
+            mockFetchHandler,
+            context,
+            "firebase",
+            listeners);
   }
 
   @Test
@@ -1109,7 +1123,6 @@ public void realtime_client_removeListener_success() {
 
   @Test
   public void realtime_stream_listen_and_retry_success() throws Exception {
-    when(mockHttpURLConnection.getResponseCode()).thenReturn(200);
     when(mockHttpURLConnection.getInputStream())
         .thenReturn(
             new ByteArrayInputStream(
@@ -1123,17 +1136,60 @@ public void realtime_stream_listen_and_retry_success() throws Exception {
 
   @Test
   public void realtime_stream_listen_fail() throws Exception {
-    when(mockHttpURLConnection.getResponseCode()).thenReturn(400);
-    when(mockHttpURLConnection.getInputStream())
-        .thenReturn(
-            new ByteArrayInputStream(
-                "{\\r\\n   \\\"latestTemplateVersionNumber\\\": 1\\r\\n}"
-                    .getBytes(StandardCharsets.UTF_8)));
-    when(mockFetchHandler.getTemplateVersionNumber()).thenReturn(1L);
-    when(mockFetchHandler.fetch(0)).thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
+    when(mockHttpURLConnection.getInputStream()).thenThrow(IOException.class);
     configAutoFetch.listenForNotifications();
 
-    verify(mockListener).onError(any(FirebaseRemoteConfigRealtimeUpdateStreamException.class));
+    verify(mockListener).onError(any(FirebaseRemoteConfigRealtimeUpdateFetchException.class));
+  }
+
+  @Test
+  public void realtime_redirectStatusCode_noRetries() throws Exception {
+    ConfigRealtimeHttpClient configRealtimeHttpClientSpy = spy(configRealtimeHttpClient);
+    doReturn(mockHttpURLConnection).when(configRealtimeHttpClientSpy).createRealtimeConnection();
+    doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
+    when(mockHttpURLConnection.getResponseCode()).thenReturn(301);
+    configRealtimeHttpClientSpy.beginRealtimeHttpStream();
+    verify(configRealtimeHttpClientSpy, never()).startAutoFetch(any());
+    verify(configRealtimeHttpClientSpy, never()).retryHTTPConnection();
+  }
+
+  @Test
+  public void realtime_okStatusCode_startAutofetchAndRetries() throws Exception {
+    ConfigRealtimeHttpClient configRealtimeHttpClientSpy = spy(configRealtimeHttpClient);
+    doReturn(mockHttpURLConnection).when(configRealtimeHttpClientSpy).createRealtimeConnection();
+    doReturn(mockConfigAutoFetch).when(configRealtimeHttpClientSpy).startAutoFetch(any());
+    doNothing().when(configRealtimeHttpClientSpy).retryHTTPConnection();
+    doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
+    when(mockHttpURLConnection.getResponseCode()).thenReturn(200);
+
+    configRealtimeHttpClientSpy.beginRealtimeHttpStream();
+    verify(mockConfigAutoFetch).listenForNotifications();
+    verify(configRealtimeHttpClientSpy).retryHTTPConnection();
+  }
+
+  @Test
+  public void realtime_badGatewayStatusCode_noAutofetchButRetries() throws Exception {
+    ConfigRealtimeHttpClient configRealtimeHttpClientSpy = spy(configRealtimeHttpClient);
+    doReturn(mockHttpURLConnection).when(configRealtimeHttpClientSpy).createRealtimeConnection();
+    doNothing().when(configRealtimeHttpClientSpy).retryHTTPConnection();
+    doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
+    when(mockHttpURLConnection.getResponseCode()).thenReturn(502);
+
+    configRealtimeHttpClientSpy.beginRealtimeHttpStream();
+    verify(configRealtimeHttpClientSpy, never()).startAutoFetch(any());
+    verify(configRealtimeHttpClientSpy).retryHTTPConnection();
+  }
+
+  @Test
+  public void realtime_exceptionThrown_noAutofetchButRetries() throws Exception {
+    ConfigRealtimeHttpClient configRealtimeHttpClientSpy = spy(configRealtimeHttpClient);
+    doThrow(IOException.class).when(configRealtimeHttpClientSpy).createRealtimeConnection();
+    doNothing().when(configRealtimeHttpClientSpy).retryHTTPConnection();
+    doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
+
+    configRealtimeHttpClientSpy.beginRealtimeHttpStream();
+    verify(configRealtimeHttpClientSpy, never()).startAutoFetch(any());
+    verify(configRealtimeHttpClientSpy).retryHTTPConnection();
   }
 
   @Test