Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Attempt to fix ANRs by moving some tasks during configure to background #1772

Merged
merged 4 commits into from
Jul 8, 2024
Merged

Attempt to fix ANRs by moving some tasks during configure to background #1772

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64d2d0d
chain editions
vegaro Jul 2, 2024
293500b
enqueue onAppForegrounded
vegaro Jul 2, 2024
39af8ff
remove @synchronized from enqueue
vegaro Jul 3, 2024
639329d
fix tests
vegaro Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions purchases/src/main/kotlin/com/revenuecat/purchases/PurchasesFactory.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ internal class PurchasesFactory(
offeringsCache,
backend,
offlineEntitlementsManager,
dispatcher,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this dispatcher was only used previously to parse offerings right? I think that should be ok then (as long as it's not the same dispatcher we run backend queries on)... I'm wondering if we should rename this, but it could come on a separate PR if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we should rename it. I had originally created a new backgroundTaskDispatcher before I noticed we could reuse this one. Is that naming good?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that could be enough as a "generic" dispatcher 👍

)

val customerInfoUpdateHandler = CustomerInfoUpdateHandler(
Expand Down Expand Up @@ -285,6 +286,7 @@ internal class PurchasesFactory(
createPaywallEventsManager(application, identityManager, eventsDispatcher, backend),
paywallPresentedCache,
purchasesStateProvider,
dispatcher,
)

return Purchases(purchasesOrchestrator)
Expand Down
43 changes: 26 additions & 17 deletions purchases/src/main/kotlin/com/revenuecat/purchases/PurchasesOrchestrator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import com.revenuecat.purchases.common.Backend
import com.revenuecat.purchases.common.BillingAbstract
import com.revenuecat.purchases.common.Config
import com.revenuecat.purchases.common.Constants
import com.revenuecat.purchases.common.Delay
import com.revenuecat.purchases.common.Dispatcher
import com.revenuecat.purchases.common.LogIntent
import com.revenuecat.purchases.common.PlatformInfo
import com.revenuecat.purchases.common.ReceiptInfo
Expand Down Expand Up @@ -73,7 +75,7 @@ import java.util.concurrent.atomic.AtomicBoolean
import kotlin.time.Duration.Companion.seconds

@Suppress("LongParameterList", "LargeClass", "TooManyFunctions")
internal class PurchasesOrchestrator constructor(
internal class PurchasesOrchestrator(
private val application: Application,
backingFieldAppUserID: String?,
private val backend: Backend,
Expand All @@ -97,6 +99,7 @@ internal class PurchasesOrchestrator constructor(
private val purchasesStateCache: PurchasesStateCache,
// This is nullable due to: https://github.com/RevenueCat/purchases-flutter/issues/408
private val mainHandler: Handler? = Handler(Looper.getMainLooper()),
private val dispatcher: Dispatcher,
) : LifecycleDelegate, CustomActivityLifecycleHandler {

internal var state: PurchasesState
Expand Down Expand Up @@ -194,22 +197,24 @@ internal class PurchasesOrchestrator constructor(
state = state.copy(appInBackground = false, firstTimeInForeground = false)
}
log(LogIntent.DEBUG, ConfigureStrings.APP_FOREGROUNDED)
if (shouldRefreshCustomerInfo(firstTimeInForeground)) {
log(LogIntent.DEBUG, CustomerInfoStrings.CUSTOMERINFO_STALE_UPDATING_FOREGROUND)
customerInfoHelper.retrieveCustomerInfo(
identityManager.currentAppUserID,
fetchPolicy = CacheFetchPolicy.FETCH_CURRENT,
appInBackground = false,
allowSharingPlayStoreAccount = allowSharingPlayStoreAccount,
)
}
offeringsManager.onAppForeground(identityManager.currentAppUserID)
postPendingTransactionsHelper.syncPendingPurchaseQueue(allowSharingPlayStoreAccount)
synchronizeSubscriberAttributesIfNeeded()
offlineEntitlementsManager.updateProductEntitlementMappingCacheIfStale()
flushPaywallEvents()
if (firstTimeInForeground && isAndroidNOrNewer()) {
diagnosticsSynchronizer?.syncDiagnosticsFileIfNeeded()
enqueue {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there should be any issue with enqueuing all this right? It is all pretty much updating caches, which is already changing into another thread to make the API call. This way we guarantee the accesses to the deviceCache are in the background

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup I think it should be mostly ok... My only concern is that there is a very small chance of the app moving to background before this is executed, so the code on onAppBackgrounded being called before... But I don't think that should cause any issues as far as I can tell 👍

if (shouldRefreshCustomerInfo(firstTimeInForeground)) {
log(LogIntent.DEBUG, CustomerInfoStrings.CUSTOMERINFO_STALE_UPDATING_FOREGROUND)
customerInfoHelper.retrieveCustomerInfo(
identityManager.currentAppUserID,
fetchPolicy = CacheFetchPolicy.FETCH_CURRENT,
appInBackground = false,
allowSharingPlayStoreAccount = allowSharingPlayStoreAccount,
)
}
offeringsManager.onAppForeground(identityManager.currentAppUserID)
postPendingTransactionsHelper.syncPendingPurchaseQueue(allowSharingPlayStoreAccount)
synchronizeSubscriberAttributesIfNeeded()
offlineEntitlementsManager.updateProductEntitlementMappingCacheIfStale()
flushPaywallEvents()
if (firstTimeInForeground && isAndroidNOrNewer()) {
diagnosticsSynchronizer?.syncDiagnosticsFileIfNeeded()
}
}
}

Expand Down Expand Up @@ -775,6 +780,10 @@ internal class PurchasesOrchestrator constructor(
//endregion

// region Private Methods
@Synchronized
vegaro marked this conversation as resolved.
Show resolved Hide resolved
private fun enqueue(command: () -> Unit) {
dispatcher.enqueue({ command() }, Delay.NONE)
}

private fun shouldRefreshCustomerInfo(firstTimeInForeground: Boolean): Boolean {
return !appConfig.customEntitlementComputation &&
Expand Down
24 changes: 22 additions & 2 deletions purchases/src/main/kotlin/com/revenuecat/purchases/common/caching/DeviceCache.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ internal open class DeviceCache(

private val offeringsResponseCacheKey: String by lazy { "$apiKeyPrefix.offeringsResponse" }

fun startEditing(): SharedPreferences.Editor {
return preferences.edit()
}

// region app user id

@Synchronized
Expand All @@ -74,7 +78,15 @@ internal open class DeviceCache(

@Synchronized
fun cacheAppUserID(appUserID: String) {
preferences.edit().putString(appUserIDCacheKey, appUserID).apply()
cacheAppUserID(appUserID, preferences.edit()).apply()
}

@Synchronized
fun cacheAppUserID(
appUserID: String,
cacheEditor: SharedPreferences.Editor,
): SharedPreferences.Editor {
return cacheEditor.putString(appUserIDCacheKey, appUserID)
}

@Synchronized
Expand Down Expand Up @@ -168,9 +180,17 @@ internal open class DeviceCache(
@Synchronized
fun clearCustomerInfoCache(appUserID: String) {
val editor = preferences.edit()
clearCustomerInfoCache(appUserID, editor)
editor.apply()
}

@Synchronized
fun clearCustomerInfoCache(
appUserID: String,
editor: SharedPreferences.Editor,
) {
editor.clearCustomerInfoCacheTimestamp(appUserID)
editor.remove(customerInfoCacheKey(appUserID))
editor.apply()
}

@Synchronized
Expand Down
2 changes: 1 addition & 1 deletion .../kotlin/com/revenuecat/purchases/common/offlineentitlements/OfflineEntitlementsManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ internal class OfflineEntitlementsManager(
warnLog(OfflineEntitlementsStrings.USING_OFFLINE_ENTITLEMENTS_CUSTOMER_INFO)
diagnosticsTracker?.trackEnteredOfflineEntitlementsMode()
_offlineCustomerInfo = customerInfo
deviceCache.getCachedAppUserID()?.let { deviceCache.clearCustomerInfoCache(it) }
deviceCache.getCachedAppUserID()?.let { deviceCache.clearCustomerInfoCache(it, cacheEditor) }
val callbacks = offlineCustomerInfoCallbackCache.remove(appUserId)
callbacks?.forEach { (onSuccess, _) ->
onSuccess(customerInfo)
Expand Down
39 changes: 31 additions & 8 deletions purchases/src/main/kotlin/com/revenuecat/purchases/identity/IdentityManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package com.revenuecat.purchases.identity

import android.content.SharedPreferences
import com.revenuecat.purchases.CustomerInfo
import com.revenuecat.purchases.PurchasesError
import com.revenuecat.purchases.PurchasesErrorCode
import com.revenuecat.purchases.VerificationResult
import com.revenuecat.purchases.common.Backend
import com.revenuecat.purchases.common.Delay
import com.revenuecat.purchases.common.Dispatcher
import com.revenuecat.purchases.common.LogIntent
import com.revenuecat.purchases.common.caching.DeviceCache
import com.revenuecat.purchases.common.debugLog
Expand All @@ -20,14 +23,15 @@ import com.revenuecat.purchases.subscriberattributes.caching.SubscriberAttribute
import java.util.Locale
import java.util.UUID

@Suppress("TooManyFunctions")
@Suppress("TooManyFunctions", "LongParameterList")
internal class IdentityManager(
private val deviceCache: DeviceCache,
private val subscriberAttributesCache: SubscriberAttributesCache,
private val subscriberAttributesManager: SubscriberAttributesManager,
private val offeringsCache: OfferingsCache,
private val backend: Backend,
private val offlineEntitlementsManager: OfflineEntitlementsManager,
private val dispatcher: Dispatcher,
) {

val currentAppUserID: String
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could potentially fail now since setting the user id happens asynchronously now right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm right... actually... any call in this class right? for example currentUserIsAnonymous.

I think we need to make all async and make sure they are all enqueued in the same dispatcher, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup pretty much, which seems it would be a pretty big undertaking...

Expand All @@ -38,7 +42,9 @@ internal class IdentityManager(
// region Public functions

@Synchronized
fun configure(appUserID: String?) {
fun configure(
appUserID: String?,
) {
if (appUserID?.isBlank() == true) {
log(LogIntent.WARNING, IdentityStrings.EMPTY_APP_USER_ID_WILL_BECOME_ANONYMOUS)
}
Expand All @@ -49,10 +55,16 @@ internal class IdentityManager(
?: deviceCache.getLegacyCachedAppUserID()
?: generateRandomID()
log(LogIntent.USER, IdentityStrings.IDENTIFYING_APP_USER_ID.format(appUserIDToUse))
deviceCache.cacheAppUserID(appUserIDToUse)
subscriberAttributesCache.cleanUpSubscriberAttributeCache(appUserIDToUse)
deviceCache.cleanupOldAttributionData()
invalidateCustomerInfoAndETagCacheIfNeeded(appUserIDToUse)

val cacheEditor = deviceCache.startEditing()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change (chaining the editions) will probably have marginal effects, but I don't think it hurts

deviceCache.cacheAppUserID(appUserIDToUse, cacheEditor)
subscriberAttributesCache.cleanUpSubscriberAttributeCache(appUserIDToUse, cacheEditor)
invalidateCustomerInfoAndETagCacheIfNeeded(appUserIDToUse, cacheEditor)
cacheEditor.apply()

enqueue {
deviceCache.cleanupOldAttributionData()
tonidero marked this conversation as resolved.
Show resolved Hide resolved
}
}

fun logIn(
Expand Down Expand Up @@ -135,11 +147,17 @@ internal class IdentityManager(
}
}

private fun invalidateCustomerInfoAndETagCacheIfNeeded(appUserID: String) {
private fun invalidateCustomerInfoAndETagCacheIfNeeded(
appUserID: String,
cacheEditor: SharedPreferences.Editor,
) {
if (backend.verificationMode == SignatureVerificationMode.Disabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is to avoid the getCachedCustomerInfo below from happening when it's disabled right? Makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly! I noticed we were getting the customer info all the time! I will add a comment here to make sure it's clear

return
}
val cachedCustomerInfo = deviceCache.getCachedCustomerInfo(appUserID)
if (shouldInvalidateCustomerInfoAndETagCache(cachedCustomerInfo)) {
infoLog(IdentityStrings.INVALIDATING_CACHED_CUSTOMER_INFO)
deviceCache.clearCustomerInfoCache(appUserID)
deviceCache.clearCustomerInfoCache(appUserID, cacheEditor)
backend.clearCaches()
}
}
Expand Down Expand Up @@ -172,5 +190,10 @@ internal class IdentityManager(
backend.clearCaches()
}

@Synchronized
private fun enqueue(command: () -> Unit) {
dispatcher.enqueue({ command() }, Delay.NONE)
}

// endregion
}
22 changes: 16 additions & 6 deletions ...kotlin/com/revenuecat/purchases/subscriberattributes/caching/SubscriberAttributesCache.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package com.revenuecat.purchases.subscriberattributes.caching

import android.content.SharedPreferences
import com.revenuecat.purchases.common.LogIntent
import com.revenuecat.purchases.common.caching.DeviceCache
import com.revenuecat.purchases.common.log
Expand Down Expand Up @@ -76,12 +77,15 @@ internal class SubscriberAttributesCache(
}

@Synchronized
fun cleanUpSubscriberAttributeCache(currentAppUserID: String) {
migrateSubscriberAttributesIfNeeded()
deleteSyncedSubscriberAttributesForOtherUsers(currentAppUserID)
fun cleanUpSubscriberAttributeCache(
currentAppUserID: String,
cacheEditor: SharedPreferences.Editor,
) {
migrateSubscriberAttributesIfNeeded(cacheEditor)
deleteSyncedSubscriberAttributesForOtherUsers(currentAppUserID, cacheEditor)
}

internal fun DeviceCache.putAttributes(
private fun DeviceCache.putAttributes(
updatedSubscriberAttributesForAll: SubscriberAttributesPerAppUserIDMap,
) {
return deviceCache.putString(
Expand All @@ -91,7 +95,10 @@ internal class SubscriberAttributesCache(
}

@Synchronized
private fun deleteSyncedSubscriberAttributesForOtherUsers(currentAppUserID: String) {
private fun deleteSyncedSubscriberAttributesForOtherUsers(
currentAppUserID: String,
cacheEditor: SharedPreferences.Editor,
) {
log(LogIntent.DEBUG, AttributionStrings.DELETING_ATTRIBUTES_OTHER_USERS.format(currentAppUserID))

val allStoredSubscriberAttributes = getAllStoredSubscriberAttributes()
Expand All @@ -105,7 +112,10 @@ internal class SubscriberAttributesCache(
}
}.toMap().filterValues { it.isNotEmpty() }

deviceCache.putAttributes(filteredMap)
cacheEditor.putString(
subscriberAttributesCacheKey,
filteredMap.toJSONObject().toString(),
)
}

private fun SubscriberAttributeMap.filterUnsynced(appUserID: AppUserID): SubscriberAttributeMap =
Expand Down
17 changes: 11 additions & 6 deletions ...enuecat/purchases/subscriberattributes/caching/SubscriberAttributesMigrationExtensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
package com.revenuecat.purchases.subscriberattributes.caching

import android.content.SharedPreferences
import com.revenuecat.purchases.subscriberattributes.buildLegacySubscriberAttributes

@Synchronized
internal fun SubscriberAttributesCache.migrateSubscriberAttributesIfNeeded() {
internal fun SubscriberAttributesCache.migrateSubscriberAttributesIfNeeded(cacheEditor: SharedPreferences.Editor) {
getAllLegacyStoredSubscriberAttributes()
.takeIf { it.isNotEmpty() }
?.let { legacySubscriberAttributes ->
migrateSubscriberAttributes(legacySubscriberAttributes)
migrateSubscriberAttributes(legacySubscriberAttributes, cacheEditor)
}
}

@Synchronized
internal fun SubscriberAttributesCache.migrateSubscriberAttributes(
legacySubscriberAttributesForAppUserID: SubscriberAttributesPerAppUserIDMap,
cacheEditor: SharedPreferences.Editor,
) {
val storedSubscriberAttributesForAll: SubscriberAttributesPerAppUserIDMap =
getAllStoredSubscriberAttributes()
Expand All @@ -25,10 +27,13 @@ internal fun SubscriberAttributesCache.migrateSubscriberAttributes(
val current: SubscriberAttributeMap = storedSubscriberAttributesForAll[appUserID] ?: emptyMap()
val updated: SubscriberAttributeMap = legacy + current
updatedStoredSubscriberAttributesForAll[appUserID] = updated
deviceCache.remove(legacySubscriberAttributesCacheKey(appUserID))
cacheEditor.remove(legacySubscriberAttributesCacheKey(appUserID))
}

deviceCache.putAttributes(updatedStoredSubscriberAttributesForAll)
cacheEditor.putString(
subscriberAttributesCacheKey,
updatedStoredSubscriberAttributesForAll.toJSONObject().toString(),
)
}

internal fun SubscriberAttributesCache.legacySubscriberAttributesCacheKey(appUserID: String) =
Expand All @@ -39,12 +44,12 @@ internal fun SubscriberAttributesCache.getAllLegacyStoredSubscriberAttributes():
val legacySubscriberAttributesCacheKeyPrefix = legacySubscriberAttributesCacheKey("")
val allSubscriberAttributesKeys = deviceCache.findKeysThatStartWith(legacySubscriberAttributesCacheKeyPrefix)

return allSubscriberAttributesKeys.map { preferencesKey ->
return allSubscriberAttributesKeys.associate { preferencesKey ->
val appUserIDFromKey: AppUserID =
preferencesKey.split(legacySubscriberAttributesCacheKeyPrefix)[1]
val subscriberAttributeMap: SubscriberAttributeMap =
deviceCache.getJSONObjectOrNull(preferencesKey)
?.buildLegacySubscriberAttributes() ?: emptyMap()
appUserIDFromKey to subscriberAttributeMap
}.toMap()
}
}