Skip to content

Commit 6eb5bbe

Browse files
mrobermrober
authored
Fetch remote settings on session, not on launch (#5107)
* Make remote settings fetch non-blocking * Ensure the settings cache has loaded from disk before reading any values * Fix runtime exception with a load-bearing empty block * Inline the definition of metadata to avoid needing an empty block * Fetch remote settings on session, not on launch * Log warning when we fail to fetch the fid
1 parent 2f30231 commit 6eb5bbe

File tree

7 files changed

+175
-95
lines changed

7 files changed

+175
-95
lines changed

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ internal constructor(
5353
private val sessionCoordinator = SessionCoordinator(firebaseInstallations, eventGDTLogger)
5454

5555
init {
56-
sessionSettings.updateSettings()
5756
sessionGenerator = SessionGenerator(collectEvents = shouldCollectEvents(), timeProvider)
5857

5958
val sessionInitiateListener =
@@ -126,13 +125,16 @@ internal constructor(
126125

127126
Log.d(TAG, "Data Collection is enabled for at least one Subscriber")
128127

128+
// This will cause remote settings to be fetched if the cache is expired.
129+
sessionSettings.updateSettings()
130+
129131
if (!sessionSettings.sessionsEnabled) {
130132
Log.d(TAG, "Sessions SDK disabled. Events will not be sent.")
131133
return
132134
}
133135

134136
if (!sessionGenerator.collectEvents) {
135-
Log.d(TAG, "Sessions SDK has sampled this session")
137+
Log.d(TAG, "Sessions SDK has dropped this session due to sampling.")
136138
return
137139
}
138140

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

Lines changed: 82 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,13 @@ import androidx.datastore.core.DataStore
2323
import androidx.datastore.preferences.core.Preferences
2424
import com.google.firebase.installations.FirebaseInstallationsApi
2525
import com.google.firebase.sessions.ApplicationInfo
26-
import java.util.concurrent.atomic.AtomicBoolean
2726
import kotlin.coroutines.CoroutineContext
2827
import kotlin.time.Duration
2928
import kotlin.time.Duration.Companion.seconds
3029
import kotlinx.coroutines.CoroutineScope
3130
import kotlinx.coroutines.launch
31+
import kotlinx.coroutines.sync.Mutex
32+
import kotlinx.coroutines.sync.withLock
3233
import kotlinx.coroutines.tasks.await
3334
import org.json.JSONException
3435
import org.json.JSONObject
@@ -41,7 +42,7 @@ internal class RemoteSettings(
4142
dataStore: DataStore<Preferences>,
4243
) : SettingsProvider {
4344
private val settingsCache = SettingsCache(dataStore)
44-
private val fetchInProgress = AtomicBoolean(false)
45+
private val fetchInProgress = Mutex()
4546

4647
override val sessionEnabled: Boolean?
4748
get() = settingsCache.sessionsEnabled()
@@ -52,101 +53,99 @@ internal class RemoteSettings(
5253
override val samplingRate: Double?
5354
get() = settingsCache.sessionSamplingRate()
5455

55-
override fun updateSettings() {
56-
val scope = CoroutineScope(backgroundDispatcher)
57-
scope.launch { fetchConfigs() }
58-
}
59-
60-
override fun isSettingsStale(): Boolean = settingsCache.hasCacheExpired()
61-
62-
@VisibleForTesting
63-
internal fun clearCachedSettings() {
64-
val scope = CoroutineScope(backgroundDispatcher)
65-
scope.launch { settingsCache.removeConfigs() }
66-
}
67-
68-
private suspend fun fetchConfigs() {
69-
// Check if a fetch is in progress. If yes, return
70-
if (fetchInProgress.get()) {
56+
/**
57+
* Fetch remote settings. This should only be called if data collection is enabled.
58+
*
59+
* This will exit early if the cache is not expired. Otherwise it will block while fetching, even
60+
* if called multiple times. This may fetch the FID, so only call if data collection is enabled.
61+
*/
62+
override suspend fun updateSettings() {
63+
// Check if cache is expired. If not, return early.
64+
if (!fetchInProgress.isLocked && !settingsCache.hasCacheExpired()) {
7165
return
7266
}
7367

74-
// Check if cache is expired. If not, return
75-
if (!settingsCache.hasCacheExpired()) {
76-
return
77-
}
78-
79-
fetchInProgress.set(true)
80-
81-
// TODO(mrober): Avoid sending the fid here, and avoid fetching it when data collection is off.
82-
// Get the installations ID before making a remote config fetch
83-
val installationId = firebaseInstallationsApi.id.await()
84-
if (installationId == null) {
85-
fetchInProgress.set(false)
86-
return
87-
}
88-
89-
// All the required fields are available, start making a network request.
90-
val options =
91-
mapOf(
92-
"X-Crashlytics-Installation-ID" to installationId,
93-
"X-Crashlytics-Device-Model" to
94-
removeForwardSlashesIn(String.format("%s/%s", Build.MANUFACTURER, Build.MODEL)),
95-
"X-Crashlytics-OS-Build-Version" to removeForwardSlashesIn(Build.VERSION.INCREMENTAL),
96-
"X-Crashlytics-OS-Display-Version" to removeForwardSlashesIn(Build.VERSION.RELEASE),
97-
"X-Crashlytics-API-Client-Version" to appInfo.sessionSdkVersion
98-
)
68+
fetchInProgress.withLock {
69+
// Double check if cache is expired. If not, return.
70+
if (!settingsCache.hasCacheExpired()) {
71+
return
72+
}
9973

100-
configsFetcher.doConfigFetch(
101-
headerOptions = options,
102-
onSuccess = {
103-
var sessionsEnabled: Boolean? = null
104-
var sessionSamplingRate: Double? = null
105-
var sessionTimeoutSeconds: Int? = null
106-
var cacheDuration: Int? = null
107-
if (it.has("app_quality")) {
108-
val aqsSettings = it.get("app_quality") as JSONObject
109-
try {
110-
if (aqsSettings.has("sessions_enabled")) {
111-
sessionsEnabled = aqsSettings.get("sessions_enabled") as Boolean?
112-
}
74+
// Get the installations ID before making a remote config fetch.
75+
val installationId = firebaseInstallationsApi.id.await()
76+
if (installationId == null) {
77+
Log.w(TAG, "Error getting Firebase Installation ID. Skipping this Session Event.")
78+
return
79+
}
11380

114-
if (aqsSettings.has("sampling_rate")) {
115-
sessionSamplingRate = aqsSettings.get("sampling_rate") as Double?
81+
// All the required fields are available, start making a network request.
82+
val options =
83+
mapOf(
84+
"X-Crashlytics-Installation-ID" to installationId,
85+
"X-Crashlytics-Device-Model" to
86+
removeForwardSlashesIn(String.format("%s/%s", Build.MANUFACTURER, Build.MODEL)),
87+
"X-Crashlytics-OS-Build-Version" to removeForwardSlashesIn(Build.VERSION.INCREMENTAL),
88+
"X-Crashlytics-OS-Display-Version" to removeForwardSlashesIn(Build.VERSION.RELEASE),
89+
"X-Crashlytics-API-Client-Version" to appInfo.sessionSdkVersion
90+
)
91+
92+
configsFetcher.doConfigFetch(
93+
headerOptions = options,
94+
onSuccess = {
95+
var sessionsEnabled: Boolean? = null
96+
var sessionSamplingRate: Double? = null
97+
var sessionTimeoutSeconds: Int? = null
98+
var cacheDuration: Int? = null
99+
if (it.has("app_quality")) {
100+
val aqsSettings = it.get("app_quality") as JSONObject
101+
try {
102+
if (aqsSettings.has("sessions_enabled")) {
103+
sessionsEnabled = aqsSettings.get("sessions_enabled") as Boolean?
104+
}
105+
106+
if (aqsSettings.has("sampling_rate")) {
107+
sessionSamplingRate = aqsSettings.get("sampling_rate") as Double?
108+
}
109+
110+
if (aqsSettings.has("session_timeout_seconds")) {
111+
sessionTimeoutSeconds = aqsSettings.get("session_timeout_seconds") as Int?
112+
}
113+
114+
if (aqsSettings.has("cache_duration")) {
115+
cacheDuration = aqsSettings.get("cache_duration") as Int?
116+
}
117+
} catch (exception: JSONException) {
118+
Log.e(TAG, "Error parsing the configs remotely fetched: ", exception)
116119
}
120+
}
117121

118-
if (aqsSettings.has("session_timeout_seconds")) {
119-
sessionTimeoutSeconds = aqsSettings.get("session_timeout_seconds") as Int?
120-
}
122+
sessionsEnabled?.let { settingsCache.updateSettingsEnabled(sessionsEnabled) }
121123

122-
if (aqsSettings.has("cache_duration")) {
123-
cacheDuration = aqsSettings.get("cache_duration") as Int?
124-
}
125-
} catch (exception: JSONException) {
126-
Log.e(TAG, "Error parsing the configs remotely fetched: ", exception)
124+
sessionTimeoutSeconds?.let {
125+
settingsCache.updateSessionRestartTimeout(sessionTimeoutSeconds)
127126
}
128-
}
129127

130-
sessionsEnabled?.let { settingsCache.updateSettingsEnabled(sessionsEnabled) }
128+
sessionSamplingRate?.let { settingsCache.updateSamplingRate(sessionSamplingRate) }
131129

132-
sessionTimeoutSeconds?.let {
133-
settingsCache.updateSessionRestartTimeout(sessionTimeoutSeconds)
134-
}
130+
cacheDuration?.let { settingsCache.updateSessionCacheDuration(cacheDuration) }
131+
?: let { settingsCache.updateSessionCacheDuration(86400) }
135132

136-
sessionSamplingRate?.let { settingsCache.updateSamplingRate(sessionSamplingRate) }
133+
settingsCache.updateSessionCacheUpdatedTime(System.currentTimeMillis())
134+
},
135+
onFailure = { msg ->
136+
// Network request failed here.
137+
Log.e(TAG, "Error failing to fetch the remote configs: $msg")
138+
}
139+
)
140+
}
141+
}
137142

138-
cacheDuration?.let { settingsCache.updateSessionCacheDuration(cacheDuration) }
139-
?: let { settingsCache.updateSessionCacheDuration(86400) }
143+
override fun isSettingsStale(): Boolean = settingsCache.hasCacheExpired()
140144

141-
settingsCache.updateSessionCacheUpdatedTime(System.currentTimeMillis())
142-
fetchInProgress.set(false)
143-
},
144-
onFailure = { msg ->
145-
// Network request failed here.
146-
Log.e(TAG, "Error failing to fetch the remote configs: $msg")
147-
fetchInProgress.set(false)
148-
}
149-
)
145+
@VisibleForTesting
146+
internal fun clearCachedSettings() {
147+
val scope = CoroutineScope(backgroundDispatcher)
148+
scope.launch { settingsCache.removeConfigs() }
150149
}
151150

152151
private fun removeForwardSlashesIn(s: String): String {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ internal class SessionsSettings(
112112
}
113113

114114
/** Update the settings for all the settings providers. */
115-
fun updateSettings() {
115+
suspend fun updateSettings() {
116116
localOverrideSettings.updateSettings()
117117
remoteSettings.updateSettings()
118118
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ internal interface SettingsProvider {
2929
val samplingRate: Double?
3030

3131
/** Function to initiate refresh of the settings for the provider. */
32-
fun updateSettings() = Unit // Default to no op.
32+
suspend fun updateSettings() = Unit // Default to no op.
3333

3434
/** Function representing if the settings are stale. */
3535
fun isSettingsStale(): Boolean = false // Default to false.

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,11 @@ import com.google.firebase.sessions.api.SessionSubscriber.Name.CRASHLYTICS
2222
import com.google.firebase.sessions.api.SessionSubscriber.Name.PERFORMANCE
2323
import com.google.firebase.sessions.testing.FakeSessionSubscriber
2424
import kotlin.time.Duration.Companion.seconds
25+
import kotlinx.coroutines.Dispatchers
2526
import kotlinx.coroutines.ExperimentalCoroutinesApi
2627
import kotlinx.coroutines.TimeoutCancellationException
2728
import kotlinx.coroutines.delay
2829
import kotlinx.coroutines.launch
29-
import kotlinx.coroutines.runBlocking
3030
import kotlinx.coroutines.test.runTest
3131
import kotlinx.coroutines.withTimeout
3232
import org.junit.After
@@ -93,14 +93,12 @@ class FirebaseSessionsDependenciesTest {
9393
}
9494

9595
@Test
96-
fun getSubscribers_waitsForRegister(): Unit = runBlocking {
97-
// This test uses runBlocking because runTest skips the delay.
98-
96+
fun getSubscribers_waitsForRegister() = runTest {
9997
val crashlyticsSubscriber = FakeSessionSubscriber(sessionSubscriberName = CRASHLYTICS)
10098
FirebaseSessionsDependencies.addDependency(CRASHLYTICS)
10199

102100
// Wait a few seconds and then register.
103-
launch {
101+
launch(Dispatchers.Default) {
104102
delay(2.seconds)
105103
FirebaseSessionsDependencies.register(crashlyticsSubscriber)
106104
}

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,16 @@ import com.google.firebase.sessions.testing.FakeFirebaseInstallations
2828
import com.google.firebase.sessions.testing.FakeRemoteConfigFetcher
2929
import com.google.firebase.sessions.testing.TestSessionEventData.TEST_APPLICATION_INFO
3030
import kotlin.time.Duration.Companion.minutes
31+
import kotlin.time.Duration.Companion.seconds
32+
import kotlinx.coroutines.Dispatchers
3133
import kotlinx.coroutines.ExperimentalCoroutinesApi
3234
import kotlinx.coroutines.asCoroutineDispatcher
35+
import kotlinx.coroutines.delay
36+
import kotlinx.coroutines.launch
3337
import kotlinx.coroutines.test.UnconfinedTestDispatcher
3438
import kotlinx.coroutines.test.runCurrent
3539
import kotlinx.coroutines.test.runTest
40+
import kotlinx.coroutines.withTimeout
3641
import org.json.JSONObject
3742
import org.junit.After
3843
import org.junit.Test
@@ -225,6 +230,66 @@ class RemoteSettingsTest {
225230
remoteSettings.clearCachedSettings()
226231
}
227232

233+
@Test
234+
fun remoteSettings_fetchWhileFetchInProgress() =
235+
runTest(UnconfinedTestDispatcher()) {
236+
// This test does:
237+
// 1. Do a fetch with a fake fetcher that will block for 3 seconds.
238+
// 2. While that is happening, do a second fetch.
239+
// - First fetch is still fetching, so second fetch should fall through to the mutex.
240+
// - Second fetch will be blocked until first completes.
241+
// - First fetch returns, should unblock the second fetch.
242+
// - Second fetch should go into mutex, sees cache is valid in "double check," exist early.
243+
// 3. After a fetch completes, do a third fetch.
244+
// - First fetch should have have updated the cache.
245+
// - Third fetch should exit even earlier, never having gone into the mutex.
246+
247+
val firebaseApp = FakeFirebaseApp().firebaseApp
248+
val context = firebaseApp.applicationContext
249+
val firebaseInstallations = FakeFirebaseInstallations("FaKeFiD")
250+
val fakeFetcherWithDelay =
251+
FakeRemoteConfigFetcher(
252+
JSONObject(VALID_RESPONSE),
253+
networkDelay = 3.seconds,
254+
)
255+
256+
fakeFetcherWithDelay.responseJSONObject
257+
.getJSONObject("app_quality")
258+
.put("sampling_rate", 0.125)
259+
260+
val remoteSettingsWithDelay =
261+
RemoteSettings(
262+
TestOnlyExecutors.background().asCoroutineDispatcher() + coroutineContext,
263+
firebaseInstallations,
264+
SessionEvents.getApplicationInfo(firebaseApp),
265+
configsFetcher = fakeFetcherWithDelay,
266+
dataStore =
267+
PreferenceDataStoreFactory.create(
268+
scope = this,
269+
produceFile = { context.preferencesDataStoreFile(SESSION_TEST_CONFIGS_NAME) },
270+
),
271+
)
272+
273+
// Do the first fetch. This one should fetched the configsFetcher.
274+
val firstFetch = launch(Dispatchers.Default) { remoteSettingsWithDelay.updateSettings() }
275+
276+
// Wait a second, and then do the second fetch while first is still running.
277+
// This one should block until the first fetch completes, but then exit early.
278+
launch(Dispatchers.Default) {
279+
delay(1.seconds)
280+
remoteSettingsWithDelay.updateSettings()
281+
}
282+
283+
// Wait until the first fetch is done, then do a third fetch.
284+
// This one should not even block, and exit early.
285+
firstFetch.join()
286+
withTimeout(1.seconds) { remoteSettingsWithDelay.updateSettings() }
287+
288+
// Assert that the configsFetcher was fetched exactly once.
289+
assertThat(fakeFetcherWithDelay.timesCalled).isEqualTo(1)
290+
assertThat(remoteSettingsWithDelay.samplingRate).isEqualTo(0.125)
291+
}
292+
228293
@Test
229294
fun remoteSettingsFetcher_badFetch_callsOnFailure() = runTest {
230295
var failure: String? = null

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

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,31 @@
1717
package com.google.firebase.sessions.testing
1818

1919
import com.google.firebase.sessions.settings.CrashlyticsSettingsFetcher
20+
import kotlin.time.Duration
2021
import org.json.JSONObject
2122

22-
internal class FakeRemoteConfigFetcher(var responseJSONObject: JSONObject = JSONObject()) :
23-
CrashlyticsSettingsFetcher {
23+
/**
24+
* Fake [CrashlyticsSettingsFetcher] that, when fetched, will produce given [responseJSONObject]
25+
* after simulating a [networkDelay], while keeping count of calls to [doConfigFetch].
26+
*/
27+
internal class FakeRemoteConfigFetcher(
28+
var responseJSONObject: JSONObject = JSONObject(),
29+
private val networkDelay: Duration = Duration.ZERO,
30+
) : CrashlyticsSettingsFetcher {
31+
/** The number of times [doConfigFetch] was called on this instance. */
32+
var timesCalled: Int = 0
33+
private set
34+
2435
override suspend fun doConfigFetch(
2536
headerOptions: Map<String, String>,
2637
onSuccess: suspend (JSONObject) -> Unit,
27-
onFailure: suspend (String) -> Unit
38+
onFailure: suspend (String) -> Unit,
2839
) {
40+
timesCalled++
41+
if (networkDelay.isPositive()) {
42+
@Suppress("BlockingMethodInNonBlockingContext") // Using sleep, not delay, for runTest.
43+
Thread.sleep(networkDelay.inWholeMilliseconds)
44+
}
2945
onSuccess(responseJSONObject)
3046
}
3147
}

0 commit comments

Comments
 (0)