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

vegaro · 2024-07-02T12:27:43Z

Description

This PR addresses a significant number of ANRs (Application Not Responding errors) reported, such as in issue #1629.

Problem

The method IdentityManager.configure is currently running on the main thread and accessing SharedPreferences. Accessing SharedPreferences can be costly, especially if the XML file storing the preferences is large. This cost is particularly high during the initial accesses, as the XML file needs to be read and parsed, which can block the main thread and lead to ANRs.

Solution

Custom Dispatcher: This PR introduces a custom dispatcher to the IdentityManager, allowing configure to run on a background thread. By moving this operation off the main thread, we avoid blocking it with the potentially expensive SharedPreferences access.
Main Thread Optimization: The remaining setup in PurchasesOrchestrator has been deferred to execute after configure completes, ensuring the main thread remains unblocked and improving overall app responsiveness.

Notes

While this fix should mitigate the reported ANRs, it's possible there are other underlying causes contributing to these issues. This PR serves as a good initial step towards improving performance.

tonidero · 2024-07-02T12:41:40Z

purchases/src/main/kotlin/com/revenuecat/purchases/identity/IdentityManager.kt

 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


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

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?

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

vegaro · 2024-07-02T19:53:55Z

purchases/src/main/kotlin/com/revenuecat/purchases/PurchasesOrchestrator.kt

-        flushPaywallEvents()
-        if (firstTimeInForeground && isAndroidNOrNewer()) {
-            diagnosticsSynchronizer?.syncDiagnosticsFileIfNeeded()
+        enqueue {


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

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 👍

vegaro · 2024-07-02T19:54:38Z

purchases/src/main/kotlin/com/revenuecat/purchases/identity/IdentityManager.kt

-        deviceCache.cleanupOldAttributionData()
-        invalidateCustomerInfoAndETagCacheIfNeeded(appUserIDToUse)
+
+        val cacheEditor = deviceCache.startEditing()


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

tonidero

Good ideas! I just left some concerns but I think this should improve things a lot.

tonidero · 2024-07-03T06:59:21Z

purchases/src/main/kotlin/com/revenuecat/purchases/PurchasesOrchestrator.kt

-        flushPaywallEvents()
-        if (firstTimeInForeground && isAndroidNOrNewer()) {
-            diagnosticsSynchronizer?.syncDiagnosticsFileIfNeeded()
+        enqueue {


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 👍

tonidero · 2024-07-03T07:08:52Z

purchases/src/main/kotlin/com/revenuecat/purchases/identity/IdentityManager.kt

+        appUserID: String,
+        cacheEditor: SharedPreferences.Editor,
+    ) {
+        if (backend.verificationMode == SignatureVerificationMode.Disabled) {


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

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

purchases/src/main/kotlin/com/revenuecat/purchases/identity/IdentityManager.kt

purchases/src/main/kotlin/com/revenuecat/purchases/PurchasesOrchestrator.kt

codecov · 2024-07-08T14:24:51Z

Codecov Report

Attention: Patch coverage is 97.87234% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.31%. Comparing base (80781e4) to head (639329d).
Report is 3 commits behind head on main.

Files	Patch %	Lines
.../com/revenuecat/purchases/PurchasesOrchestrator.kt	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1772      +/-   ##
==========================================
+ Coverage   83.26%   83.31%   +0.05%     
==========================================
  Files         223      223              
  Lines        7617     7634      +17     
  Branches     1071     1072       +1     
==========================================
+ Hits         6342     6360      +18     
+ Misses        854      853       -1     
  Partials      421      421

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tonidero

LGTM!

tonidero · 2024-07-08T17:19:59Z

purchases/src/main/kotlin/com/revenuecat/purchases/PurchasesFactory.kt

@@ -179,6 +179,7 @@ internal class PurchasesFactory(
                offeringsCache,
                backend,
                offlineEntitlementsManager,
+                dispatcher,


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.

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?

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

…background (#1772)" This reverts commit c6b365f.

@vegaro

**This is an automatic release.** ### Bugfixes * Attempt to fix ANRs by moving some tasks during configure to background (#1772) via Cesar de la Vega (@vegaro) * Use trimmed API key (#1768) via Toni Rico (@tonidero) ### Other Changes * Remove leftovers `ExperimentalPreviewRevenueCatUIPurchasesAPI` (#1778) via Cesar de la Vega (@vegaro) * Update migration docs adding back support for `DEFERRED` upgrades (#1774) via Toni Rico (@tonidero) * Add 7.12.0 to CHANGELOG (#1769) via Toni Rico (@tonidero) --------- Co-authored-by: revenuecat-ops <[email protected]> Co-authored-by: Toni Rico <[email protected]>

vegaro added the pr:fix A bug fix label Jul 2, 2024

tonidero reviewed Jul 2, 2024

View reviewed changes

vegaro added 2 commits July 2, 2024 21:42

chain editions

64d2d0d

enqueue onAppForegrounded

293500b

vegaro force-pushed the sdk-3510-change-identitymanagerconfigure-to-execute-in-background branch from 0473940 to 293500b Compare July 2, 2024 19:50

vegaro commented Jul 2, 2024

View reviewed changes

tonidero reviewed Jul 3, 2024

View reviewed changes

vegaro added 2 commits July 3, 2024 09:23

remove @synchronized from enqueue

39af8ff

fix tests

639329d

vegaro marked this pull request as ready for review July 8, 2024 15:37

vegaro requested a review from a team July 8, 2024 15:37

tonidero approved these changes Jul 8, 2024

View reviewed changes

vegaro changed the title ~~Change configure in IdentityManager to execute on a thread~~ Attempt to fix ANRs by moving some tasks during configure to background Jul 8, 2024

vegaro merged commit c6b365f into main Jul 8, 2024
9 checks passed

vegaro deleted the sdk-3510-change-identitymanagerconfigure-to-execute-in-background branch July 8, 2024 20:17

RCGitBot mentioned this pull request Jul 10, 2024

[AUTOMATIC] Release/8.2.1 #1779

Merged

tonidero added a commit that referenced this pull request Jul 11, 2024

Revert "Attempt to fix ANRs by moving some tasks during configure to …

08f23e9

…background (#1772)" This reverts commit c6b365f.

tonidero mentioned this pull request Jul 11, 2024

Test integration tests failures #1780

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vegaro commented Jul 2, 2024 •

edited

Loading

tonidero Jul 2, 2024

vegaro Jul 2, 2024

tonidero Jul 2, 2024

vegaro Jul 2, 2024

tonidero Jul 3, 2024

vegaro Jul 2, 2024

tonidero left a comment •

edited

Loading

tonidero Jul 3, 2024

tonidero Jul 3, 2024

vegaro Jul 3, 2024

codecov bot commented Jul 8, 2024

tonidero left a comment

tonidero Jul 8, 2024

vegaro Jul 8, 2024

tonidero Jul 9, 2024

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

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

Conversation

vegaro commented Jul 2, 2024 • edited Loading

Description

Problem

Solution

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonidero left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 8, 2024

Codecov Report

tonidero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegaro commented Jul 2, 2024 •

edited

Loading

tonidero left a comment •

edited

Loading