Fetch remote settings on session, not on launch

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/FirebaseSessions.kt

@@ -53,7 +53,6 @@ internal constructor(
   private val sessionCoordinator = SessionCoordinator(firebaseInstallations, eventGDTLogger)
 
   init {
-    sessionSettings.updateSettings()
     sessionGenerator = SessionGenerator(collectEvents = shouldCollectEvents(), timeProvider)
 
     val sessionInitiateListener =
@@ -126,13 +125,16 @@ internal constructor(
 
     Log.d(TAG, "Data Collection is enabled for at least one Subscriber")
 
+    // This will cause remote settings to be fetched if the cache is expired.
+    sessionSettings.updateSettings()
+
     if (!sessionSettings.sessionsEnabled) {
       Log.d(TAG, "Sessions SDK disabled. Events will not be sent.")
       return
     }
 
     if (!sessionGenerator.collectEvents) {
-      Log.d(TAG, "Sessions SDK has sampled this session")
+      Log.d(TAG, "Sessions SDK has dropped this session due to sampling.")
       return
     }
 

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt

@@ -23,12 +23,13 @@ import androidx.datastore.core.DataStore
 import androidx.datastore.preferences.core.Preferences
 import com.google.firebase.installations.FirebaseInstallationsApi
 import com.google.firebase.sessions.ApplicationInfo
-import java.util.concurrent.atomic.AtomicBoolean
 import kotlin.coroutines.CoroutineContext
 import kotlin.time.Duration
 import kotlin.time.Duration.Companion.seconds
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
 import kotlinx.coroutines.tasks.await
 import org.json.JSONException
 import org.json.JSONObject
@@ -41,7 +42,7 @@ internal class RemoteSettings(
   dataStore: DataStore<Preferences>,
 ) : SettingsProvider {
   private val settingsCache = SettingsCache(dataStore)
-  private val fetchInProgress = AtomicBoolean(false)
+  private val fetchInProgress = Mutex()
 
   override val sessionEnabled: Boolean?
     get() = settingsCache.sessionsEnabled()
@@ -52,101 +53,99 @@ internal class RemoteSettings(
   override val samplingRate: Double?
     get() = settingsCache.sessionSamplingRate()
 
-  override fun updateSettings() {
-    val scope = CoroutineScope(backgroundDispatcher)
-    scope.launch { fetchConfigs() }
-  }
-
-  override fun isSettingsStale(): Boolean = settingsCache.hasCacheExpired()
-
-  @VisibleForTesting
-  internal fun clearCachedSettings() {
-    val scope = CoroutineScope(backgroundDispatcher)
-    scope.launch { settingsCache.removeConfigs() }
-  }
-
-  private suspend fun fetchConfigs() {
-    // Check if a fetch is in progress. If yes, return
-    if (fetchInProgress.get()) {
+  /**
+   * Fetch remote settings. This should only be called if data collection is enabled.
+   *
+   * This will exit early if the cache is not expired. Otherwise it will block while fetching, even
+   * if called multiple times. This may fetch the FID, so only call if data collection is enabled.
+   */
+  override suspend fun updateSettings() {
+    // Check if cache is expired. If not, return early.
+    if (!fetchInProgress.isLocked && !settingsCache.hasCacheExpired()) {
       return
     }
 
-    // Check if cache is expired. If not, return
-    if (!settingsCache.hasCacheExpired()) {
-      return
-    }
-
-    fetchInProgress.set(true)
-
-    // TODO(mrober): Avoid sending the fid here, and avoid fetching it when data collection is off.
-    // Get the installations ID before making a remote config fetch
-    val installationId = firebaseInstallationsApi.id.await()
-    if (installationId == null) {
-      fetchInProgress.set(false)
-      return
-    }
-
-    // All the required fields are available, start making a network request.
-    val options =
-      mapOf(
-        "X-Crashlytics-Installation-ID" to installationId,
-        "X-Crashlytics-Device-Model" to
-          removeForwardSlashesIn(String.format("%s/%s", Build.MANUFACTURER, Build.MODEL)),
-        "X-Crashlytics-OS-Build-Version" to removeForwardSlashesIn(Build.VERSION.INCREMENTAL),
-        "X-Crashlytics-OS-Display-Version" to removeForwardSlashesIn(Build.VERSION.RELEASE),
-        "X-Crashlytics-API-Client-Version" to appInfo.sessionSdkVersion
-      )
+    fetchInProgress.withLock {
+      // Double check if cache is expired. If not, return.
+      if (!settingsCache.hasCacheExpired()) {
+        return
+      }
 
-    configsFetcher.doConfigFetch(
-      headerOptions = options,
-      onSuccess = {
-        var sessionsEnabled: Boolean? = null
-        var sessionSamplingRate: Double? = null
-        var sessionTimeoutSeconds: Int? = null
-        var cacheDuration: Int? = null
-        if (it.has("app_quality")) {
-          val aqsSettings = it.get("app_quality") as JSONObject
-          try {
-            if (aqsSettings.has("sessions_enabled")) {
-              sessionsEnabled = aqsSettings.get("sessions_enabled") as Boolean?
-            }
+      // Get the installations ID before making a remote config fetch.
+      val installationId = firebaseInstallationsApi.id.await()
+      if (installationId == null) {
+        Log.w(TAG, "Error getting Firebase Installation ID. Skipping this Session Event.")
+        return
+      }
 
-            if (aqsSettings.has("sampling_rate")) {
-              sessionSamplingRate = aqsSettings.get("sampling_rate") as Double?
+      // All the required fields are available, start making a network request.
+      val options =
+        mapOf(
+          "X-Crashlytics-Installation-ID" to installationId,
+          "X-Crashlytics-Device-Model" to
+            removeForwardSlashesIn(String.format("%s/%s", Build.MANUFACTURER, Build.MODEL)),
+          "X-Crashlytics-OS-Build-Version" to removeForwardSlashesIn(Build.VERSION.INCREMENTAL),
+          "X-Crashlytics-OS-Display-Version" to removeForwardSlashesIn(Build.VERSION.RELEASE),
+          "X-Crashlytics-API-Client-Version" to appInfo.sessionSdkVersion
+        )
+
+      configsFetcher.doConfigFetch(
+        headerOptions = options,
+        onSuccess = {
+          var sessionsEnabled: Boolean? = null
+          var sessionSamplingRate: Double? = null
+          var sessionTimeoutSeconds: Int? = null
+          var cacheDuration: Int? = null
+          if (it.has("app_quality")) {
+            val aqsSettings = it.get("app_quality") as JSONObject
+            try {
+              if (aqsSettings.has("sessions_enabled")) {
+                sessionsEnabled = aqsSettings.get("sessions_enabled") as Boolean?
+              }
+
+              if (aqsSettings.has("sampling_rate")) {
+                sessionSamplingRate = aqsSettings.get("sampling_rate") as Double?
+              }
+
+              if (aqsSettings.has("session_timeout_seconds")) {
+                sessionTimeoutSeconds = aqsSettings.get("session_timeout_seconds") as Int?
+              }
+
+              if (aqsSettings.has("cache_duration")) {
+                cacheDuration = aqsSettings.get("cache_duration") as Int?
+              }
+            } catch (exception: JSONException) {
+              Log.e(TAG, "Error parsing the configs remotely fetched: ", exception)
             }
+          }
 
-            if (aqsSettings.has("session_timeout_seconds")) {
-              sessionTimeoutSeconds = aqsSettings.get("session_timeout_seconds") as Int?
-            }
+          sessionsEnabled?.let { settingsCache.updateSettingsEnabled(sessionsEnabled) }
 
-            if (aqsSettings.has("cache_duration")) {
-              cacheDuration = aqsSettings.get("cache_duration") as Int?
-            }
-          } catch (exception: JSONException) {
-            Log.e(TAG, "Error parsing the configs remotely fetched: ", exception)
+          sessionTimeoutSeconds?.let {
+            settingsCache.updateSessionRestartTimeout(sessionTimeoutSeconds)
           }
-        }
 
-        sessionsEnabled?.let { settingsCache.updateSettingsEnabled(sessionsEnabled) }
+          sessionSamplingRate?.let { settingsCache.updateSamplingRate(sessionSamplingRate) }
 
-        sessionTimeoutSeconds?.let {
-          settingsCache.updateSessionRestartTimeout(sessionTimeoutSeconds)
-        }
+          cacheDuration?.let { settingsCache.updateSessionCacheDuration(cacheDuration) }
+            ?: let { settingsCache.updateSessionCacheDuration(86400) }
 
-        sessionSamplingRate?.let { settingsCache.updateSamplingRate(sessionSamplingRate) }
+          settingsCache.updateSessionCacheUpdatedTime(System.currentTimeMillis())
+        },
+        onFailure = { msg ->
+          // Network request failed here.
+          Log.e(TAG, "Error failing to fetch the remote configs: $msg")
+        }
+      )
+    }
+  }
 
-        cacheDuration?.let { settingsCache.updateSessionCacheDuration(cacheDuration) }
-          ?: let { settingsCache.updateSessionCacheDuration(86400) }
+  override fun isSettingsStale(): Boolean = settingsCache.hasCacheExpired()
 
-        settingsCache.updateSessionCacheUpdatedTime(System.currentTimeMillis())
-        fetchInProgress.set(false)
-      },
-      onFailure = { msg ->
-        // Network request failed here.
-        Log.e(TAG, "Error failing to fetch the remote configs: $msg")
-        fetchInProgress.set(false)
-      }
-    )
+  @VisibleForTesting
+  internal fun clearCachedSettings() {
+    val scope = CoroutineScope(backgroundDispatcher)
+    scope.launch { settingsCache.removeConfigs() }
   }
 
   private fun removeForwardSlashesIn(s: String): String {

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/SessionsSettings.kt

@@ -112,7 +112,7 @@ internal class SessionsSettings(
   }
 
   /** Update the settings for all the settings providers. */
-  fun updateSettings() {
+  suspend fun updateSettings() {
     localOverrideSettings.updateSettings()
     remoteSettings.updateSettings()
   }

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/SettingsProvider.kt

@@ -29,7 +29,7 @@ internal interface SettingsProvider {
   val samplingRate: Double?
 
   /** Function to initiate refresh of the settings for the provider. */
-  fun updateSettings() = Unit // Default to no op.
+  suspend fun updateSettings() = Unit // Default to no op.
 
   /** Function representing if the settings are stale. */
   fun isSettingsStale(): Boolean = false // Default to false.

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/api/FirebaseSessionsDependenciesTest.kt

@@ -22,11 +22,11 @@ import com.google.firebase.sessions.api.SessionSubscriber.Name.CRASHLYTICS
 import com.google.firebase.sessions.api.SessionSubscriber.Name.PERFORMANCE
 import com.google.firebase.sessions.testing.FakeSessionSubscriber
 import kotlin.time.Duration.Companion.seconds
+import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.TimeoutCancellationException
 import kotlinx.coroutines.delay
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.test.runTest
 import kotlinx.coroutines.withTimeout
 import org.junit.After
@@ -93,14 +93,12 @@ class FirebaseSessionsDependenciesTest {
   }
 
   @Test
-  fun getSubscribers_waitsForRegister(): Unit = runBlocking {
-    // This test uses runBlocking because runTest skips the delay.
-
+  fun getSubscribers_waitsForRegister() = runTest {
     val crashlyticsSubscriber = FakeSessionSubscriber(sessionSubscriberName = CRASHLYTICS)
     FirebaseSessionsDependencies.addDependency(CRASHLYTICS)
 
     // Wait a few seconds and then register.
-    launch {
+    launch(Dispatchers.Default) {
       delay(2.seconds)
       FirebaseSessionsDependencies.register(crashlyticsSubscriber)
     }

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/settings/RemoteSettingsTest.kt

@@ -28,11 +28,16 @@ import com.google.firebase.sessions.testing.FakeFirebaseInstallations
 import com.google.firebase.sessions.testing.FakeRemoteConfigFetcher
 import com.google.firebase.sessions.testing.TestSessionEventData.TEST_APPLICATION_INFO
 import kotlin.time.Duration.Companion.minutes
+import kotlin.time.Duration.Companion.seconds
+import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.asCoroutineDispatcher
+import kotlinx.coroutines.delay
+import kotlinx.coroutines.launch
 import kotlinx.coroutines.test.UnconfinedTestDispatcher
 import kotlinx.coroutines.test.runCurrent
 import kotlinx.coroutines.test.runTest
+import kotlinx.coroutines.withTimeout
 import org.json.JSONObject
 import org.junit.After
 import org.junit.Test
@@ -225,6 +230,66 @@ class RemoteSettingsTest {
       remoteSettings.clearCachedSettings()
     }
 
+  @Test
+  fun remoteSettings_fetchWhileFetchInProgress() =
+    runTest(UnconfinedTestDispatcher()) {
+      // This test does:
+      // 1. Do a fetch with a fake fetcher that will block for 3 seconds.
+      // 2. While that is happening, do a second fetch.
+      //    - First fetch is still fetching, so second fetch should fall through to the mutex.
+      //    - Second fetch will be blocked until first completes.
+      //    - First fetch returns, should unblock the second fetch.
+      //    - Second fetch should go into mutex, sees cache is valid in "double check," exist early.
+      // 3. After a fetch completes, do a third fetch.
+      //    - First fetch should have have updated the cache.
+      //    - Third fetch should exit even earlier, never having gone into the mutex.
+
+      val firebaseApp = FakeFirebaseApp().firebaseApp
+      val context = firebaseApp.applicationContext
+      val firebaseInstallations = FakeFirebaseInstallations("FaKeFiD")
+      val fakeFetcherWithDelay =
+        FakeRemoteConfigFetcher(
+          JSONObject(VALID_RESPONSE),
+          networkDelay = 3.seconds,
+        )
+
+      fakeFetcherWithDelay.responseJSONObject
+        .getJSONObject("app_quality")
+        .put("sampling_rate", 0.125)
+
+      val remoteSettingsWithDelay =
+        RemoteSettings(
+          TestOnlyExecutors.background().asCoroutineDispatcher() + coroutineContext,
+          firebaseInstallations,
+          SessionEvents.getApplicationInfo(firebaseApp),
+          configsFetcher = fakeFetcherWithDelay,
+          dataStore =
+            PreferenceDataStoreFactory.create(
+              scope = this,
+              produceFile = { context.preferencesDataStoreFile(SESSION_TEST_CONFIGS_NAME) },
+            ),
+        )
+
+      // Do the first fetch. This one should fetched the configsFetcher.
+      val firstFetch = launch(Dispatchers.Default) { remoteSettingsWithDelay.updateSettings() }
+
+      // Wait a second, and then do the second fetch while first is still running.
+      // This one should block until the first fetch completes, but then exit early.
+      launch(Dispatchers.Default) {
+        delay(1.seconds)
+        remoteSettingsWithDelay.updateSettings()
+      }
+
+      // Wait until the first fetch is done, then do a third fetch.
+      // This one should not even block, and exit early.
+      firstFetch.join()
+      withTimeout(1.seconds) { remoteSettingsWithDelay.updateSettings() }
+
+      // Assert that the configsFetcher was fetched exactly once.
+      assertThat(fakeFetcherWithDelay.timesCalled).isEqualTo(1)
+      assertThat(remoteSettingsWithDelay.samplingRate).isEqualTo(0.125)
+    }
+
   @Test
   fun remoteSettingsFetcher_badFetch_callsOnFailure() = runTest {
     var failure: String? = null

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/testing/FakeRemoteConfigFetcher.kt

@@ -17,15 +17,31 @@
 package com.google.firebase.sessions.testing
 
 import com.google.firebase.sessions.settings.CrashlyticsSettingsFetcher
+import kotlin.time.Duration
 import org.json.JSONObject
 
-internal class FakeRemoteConfigFetcher(var responseJSONObject: JSONObject = JSONObject()) :
-  CrashlyticsSettingsFetcher {
+/**
+ * Fake [CrashlyticsSettingsFetcher] that, when fetched, will produce given [responseJSONObject]
+ * after simulating a [networkDelay], while keeping count of calls to [doConfigFetch].
+ */
+internal class FakeRemoteConfigFetcher(
+  var responseJSONObject: JSONObject = JSONObject(),
+  private val networkDelay: Duration = Duration.ZERO,
+) : CrashlyticsSettingsFetcher {
+  /** The number of times [doConfigFetch] was called on this instance. */
+  var timesCalled: Int = 0
+    private set
+
   override suspend fun doConfigFetch(
     headerOptions: Map<String, String>,
     onSuccess: suspend (JSONObject) -> Unit,
-    onFailure: suspend (String) -> Unit
+    onFailure: suspend (String) -> Unit,
   ) {
+    timesCalled++
+    if (networkDelay.isPositive()) {
+      @Suppress("BlockingMethodInNonBlockingContext") // Using sleep, not delay, for runTest.
+      Thread.sleep(networkDelay.inWholeMilliseconds)
+    }
     onSuccess(responseJSONObject)
   }
 }