Merge remote-tracking branch 'origin/main' into MutableStateFlowUseUpdateInsteadOfCompareAndSet

Author: dconeybe

.github/workflows/fireci.yml

@@ -24,4 +24,4 @@ jobs:
       - run: |
           pytest ci/fireci
       - run: |
-          mypy --config-file ci/fireci/setup.cfg ci/fireci/
+          mypy --config-file ci/fireci/pyproject.toml ci/fireci/

ci/fireci/pyproject.toml

@@ -1,3 +1,44 @@
 [build-system]
-requires = ["setuptools ~= 58.0"]
+requires = ["setuptools ~= 70.0"]
 build-backend = "setuptools.build_meta"
+
+[project]
+name = "fireci"
+version = "0.1"
+dependencies = [
+  "protobuf==3.20.3",
+  "click==8.1.7",
+  "google-cloud-storage==2.18.2",
+  "mypy==1.6.0",
+  "numpy==1.24.4",
+  "pandas==1.5.3",
+  "PyGithub==1.58.2",
+  "pystache==0.6.0",
+  "requests==2.31.0",
+  "seaborn==0.12.2",
+  "PyYAML==6.0.1",
+  "termcolor==2.4.0",
+  "pytest"
+]
+
+[project.scripts]
+fireci = "fireci.main:cli"
+
+[tool.setuptools]
+packages = ["fireci", "fireciplugins"]
+
+[tool.mypy]
+strict_optional = false
+
+[[tool.mypy.overrides]]
+  module = [
+	"google.cloud",
+	"matplotlib",
+	"matplotlib.pyplot",
+	"pandas",
+	"pystache",
+	"requests",
+	"seaborn",
+	"yaml"
+  ]
+  ignore_missing_imports = true

ci/fireci/setup.cfg

@@ -15,7 +15,12 @@
 # limitations under the License.
 
 set -e
+set -x
 
 DIRECTORY=$(cd `dirname $0` && pwd)
-pip3 install -e $DIRECTORY/fireci >> /dev/null
+python3 -m ensurepip --upgrade
+python3 -m pip install --upgrade setuptools
+python3 -m pip install --upgrade pip
+python3 -m pip install --upgrade wheel
+python3 -m pip install -e $DIRECTORY/fireci >> /dev/null
 fireci $@

ci/run.sh

@@ -1,5 +1,6 @@
 # Unreleased
-
+[fixed] Fixed an issue where the connection to the real-time Remote Config backend could remain
+open in the background.
 
 # 22.1.0
 * [feature] Added support for custom signal targeting in Remote Config. Use `setCustomSignals` API for setting custom signals and use them to build custom targeting conditions in Remote Config.

firebase-config/CHANGELOG.md

@@ -54,6 +54,7 @@ public class ConfigAutoFetch {
   private final ConfigUpdateListener retryCallback;
   private final ScheduledExecutorService scheduledExecutorService;
   private final Random random;
+  private boolean isInBackground;
 
   public ConfigAutoFetch(
       HttpURLConnection httpURLConnection,
@@ -69,6 +70,7 @@ public ConfigAutoFetch(
     this.retryCallback = retryCallback;
     this.scheduledExecutorService = scheduledExecutorService;
     this.random = new Random();
+    this.isInBackground = false;
   }
 
   private synchronized void propagateErrors(FirebaseRemoteConfigException exception) {
@@ -87,6 +89,10 @@ private synchronized boolean isEventListenersEmpty() {
     return this.eventListeners.isEmpty();
   }
 
+  public void setIsInBackground(boolean isInBackground) {
+    this.isInBackground = isInBackground;
+  }
+
   private String parseAndValidateConfigUpdateMessage(String message) {
     int left = message.indexOf('{');
     int right = message.lastIndexOf('}');
@@ -105,15 +111,29 @@ public void listenForNotifications() {
       return;
     }
 
+    // Maintain a reference to the InputStream to guarantee its closure upon completion or in case
+    // of an exception.
+    InputStream inputStream = null;
     try {
-      InputStream inputStream = httpURLConnection.getInputStream();
+      inputStream = httpURLConnection.getInputStream();
       handleNotifications(inputStream);
-      inputStream.close();
     } catch (IOException ex) {
-      // Stream was interrupted due to a transient issue and the system will retry the connection.
-      Log.d(TAG, "Stream was cancelled due to an exception. Retrying the connection...", ex);
+      // If the real-time connection is at an unexpected lifecycle state when the app is
+      // backgrounded, it's expected closing the httpURLConnection will throw an exception.
+      if (!isInBackground) {
+        // Otherwise, the real-time server connection was closed due to a transient issue.
+        Log.d(TAG, "Real-time connection was closed due to an exception.", ex);
+      }
     } finally {
-      httpURLConnection.disconnect();
+      if (inputStream != null) {
+        try {
+          // Only need to close the InputStream, ConfigRealtimeHttpClient will disconnect
+          // HttpUrlConnection
+          inputStream.close();
+        } catch (IOException ex) {
+          Log.d(TAG, "Exception thrown when closing connection stream. Retrying connection...", ex);
+        }
+      }
     }
   }
 
@@ -186,7 +206,6 @@ private void handleNotifications(InputStream inputStream) throws IOException {
     }
 
     reader.close();
-    inputStream.close();
   }
 
   private void autoFetch(int remainingAttempts, long targetVersion) {

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

@@ -91,7 +91,7 @@ public synchronized ConfigUpdateListenerRegistration addRealtimeConfigUpdateList
   }
 
   public synchronized void setBackgroundState(boolean isInBackground) {
-    configRealtimeHttpClient.setRealtimeBackgroundState(isInBackground);
+    configRealtimeHttpClient.setIsInBackground(isInBackground);
     if (!isInBackground) {
       beginRealtime();
     }

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

@@ -100,6 +100,10 @@ public class ConfigRealtimeHttpClient {
   /** Flag to indicate whether or not the app is in the background or not. */
   private boolean isInBackground;
 
+  // The HttpUrlConnection and auto-fetcher for this client. Only one of each exist at a time.
+  private HttpURLConnection httpURLConnection;
+  private ConfigAutoFetch configAutoFetch;
+
   private final int ORIGINAL_RETRIES = 8;
   private final ScheduledExecutorService scheduledExecutorService;
   private final ConfigFetchHandler configFetchHandler;
@@ -111,6 +115,7 @@ public class ConfigRealtimeHttpClient {
   private final Random random;
   private final Clock clock;
   private final ConfigSharedPrefsClient sharedPrefsClient;
+  private final Object backgroundLock;
 
   public ConfigRealtimeHttpClient(
       FirebaseApp firebaseApp,
@@ -145,6 +150,7 @@ public ConfigRealtimeHttpClient(
     this.sharedPrefsClient = sharedPrefsClient;
     this.isRealtimeDisabled = false;
     this.isInBackground = false;
+    this.backgroundLock = new Object();
   }
 
   private static String extractProjectNumberFromAppId(String gmpAppId) {
@@ -391,14 +397,42 @@ public void run() {
     }
   }
 
-  void setRealtimeBackgroundState(boolean backgroundState) {
-    isInBackground = backgroundState;
+  public void setIsInBackground(boolean isInBackground) {
+    // Make changes in synchronized block so only one thread sets the background state and calls
+    // disconnect.
+    synchronized (backgroundLock) {
+      this.isInBackground = isInBackground;
+
+      // Propagate to ConfigAutoFetch as well.
+      if (configAutoFetch != null) {
+        configAutoFetch.setIsInBackground(isInBackground);
+      }
+      // Close the connection if the app is in the background and there is an active
+      // HttpUrlConnection.
+      if (isInBackground && httpURLConnection != null) {
+        httpURLConnection.disconnect();
+      }
+    }
   }
 
   private synchronized void resetRetryCount() {
     httpRetriesRemaining = ORIGINAL_RETRIES;
   }
 
+  /**
+   * The check and set http connection method are combined so that when canMakeHttpStreamConnection
+   * returns true, the same thread can mark isHttpConnectionIsRunning as true to prevent a race
+   * condition with another thread.
+   */
+  private synchronized boolean checkAndSetHttpConnectionFlagIfNotRunning() {
+    boolean canMakeConnection = canMakeHttpStreamConnection();
+    if (canMakeConnection) {
+      setIsHttpConnectionRunning(true);
+    }
+
+    return canMakeConnection;
+  }
+
   private synchronized void setIsHttpConnectionRunning(boolean connectionRunning) {
     isHttpConnectionRunning = connectionRunning;
   }
@@ -469,7 +503,7 @@ private String parseForbiddenErrorResponseMessage(InputStream inputStream) {
    */
   @SuppressLint({"VisibleForTests", "DefaultLocale"})
   public void beginRealtimeHttpStream() {
-    if (!canMakeHttpStreamConnection()) {
+    if (!checkAndSetHttpConnectionFlagIfNotRunning()) {
       return;
     }
 
@@ -489,17 +523,21 @@ public void beginRealtimeHttpStream() {
             this.scheduledExecutorService,
             (completedHttpUrlConnectionTask) -> {
               Integer responseCode = null;
-              HttpURLConnection httpURLConnection = null;
+              // Get references to InputStream and ErrorStream before listening on the stream so
+              // that they can be closed without getting them from HttpUrlConnection.
+              InputStream inputStream = null;
+              InputStream errorStream = null;
 
               try {
                 // If HTTP connection task failed throw exception to move to the catch block.
                 if (!httpURLConnectionTask.isSuccessful()) {
                   throw new IOException(httpURLConnectionTask.getException());
                 }
-                setIsHttpConnectionRunning(true);
 
                 // Get HTTP connection and check response code.
                 httpURLConnection = httpURLConnectionTask.getResult();
+                inputStream = httpURLConnection.getInputStream();
+                errorStream = httpURLConnection.getErrorStream();
                 responseCode = httpURLConnection.getResponseCode();
 
                 // If the connection returned a 200 response code, start listening for messages.
@@ -509,23 +547,32 @@ public void beginRealtimeHttpStream() {
                   sharedPrefsClient.resetRealtimeBackoff();
 
                   // Start listening for realtime notifications.
-                  ConfigAutoFetch configAutoFetch = startAutoFetch(httpURLConnection);
+                  configAutoFetch = startAutoFetch(httpURLConnection);
                   configAutoFetch.listenForNotifications();
                 }
               } catch (IOException e) {
-                // Stream could not be open due to a transient issue and the system will retry the
-                // connection
-                // without user intervention.
-                Log.d(
-                    TAG,
-                    "Exception connecting to real-time RC backend. Retrying the connection...",
-                    e);
+                if (isInBackground) {
+                  // It's possible the app was backgrounded while the connection was open, which
+                  // threw an exception trying to read the response. No real error here, so treat
+                  // this as a success, even if we haven't read a 200 response code yet.
+                  resetRetryCount();
+                } else {
+                  // If it's not in the background, there might have been a transient error so the
+                  // client will retry the connection.
+                  Log.d(
+                      TAG,
+                      "Exception connecting to real-time RC backend. Retrying the connection...",
+                      e);
+                }
               } finally {
-                closeRealtimeHttpStream(httpURLConnection);
+                // Close HTTP connection and associated streams.
+                closeRealtimeHttpConnection(inputStream, errorStream);
                 setIsHttpConnectionRunning(false);
 
+                // Update backoff metadata if the connection failed in the foreground.
                 boolean connectionFailed =
-                    responseCode == null || isStatusCodeRetryable(responseCode);
+                    !isInBackground
+                        && (responseCode == null || isStatusCodeRetryable(responseCode));
                 if (connectionFailed) {
                   updateBackoffMetadataWithLastFailedStreamConnectionTime(
                       new Date(clock.currentTimeMillis()));
@@ -556,24 +603,34 @@ public void beginRealtimeHttpStream() {
                 }
               }
 
+              // Reset parameters.
+              httpURLConnection = null;
+              configAutoFetch = null;
+
               return Tasks.forResult(null);
             });
   }
 
-  // Pauses Http stream listening
-  public void closeRealtimeHttpStream(HttpURLConnection httpURLConnection) {
-    if (httpURLConnection != null) {
-      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.
+  private void closeHttpConnectionInputStream(InputStream inputStream) {
+    if (inputStream != null) {
       try {
-        httpURLConnection.getInputStream().close();
-        if (httpURLConnection.getErrorStream() != null) {
-          httpURLConnection.getErrorStream().close();
-        }
-      } catch (IOException e) {
+        inputStream.close();
+      } catch (IOException ex) {
+        Log.d(TAG, "Error closing connection stream.", ex);
       }
     }
   }
+
+  // Pauses Http stream listening by disconnecting the HttpUrlConnection and underlying InputStream
+  // and ErrorStream if they exist.
+  @VisibleForTesting
+  public void closeRealtimeHttpConnection(InputStream inputStream, InputStream errorStream) {
+    // Disconnect only if the connection is not null and in the foreground.
+    if (httpURLConnection != null && !isInBackground) {
+      httpURLConnection.disconnect();
+    }
+
+    closeHttpConnectionInputStream(inputStream);
+    closeHttpConnectionInputStream(errorStream);
+  }
 }