chore: make prefs operation async #578
Conversation
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature-performance-improvements #578 +/- ##
===================================================================
Coverage ? 67.66%
Complexity ? 558
===================================================================
Files ? 121
Lines ? 3748
Branches ? 455
===================================================================
Hits ? 2536
Misses ? 1040
Partials ? 172 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Build available to test |
📏 SDK Binary Size Comparison Report
|
|
|
…io/customerio-android into make-saving-configuration-async
|
|
|
|
|
|
|
|
||
| override val registeredDeviceToken: String? | ||
| get() = globalPreferenceStore.getDeviceToken() | ||
| get() = deviceTokenManager.deviceToken |
There was a problem hiding this comment.
I'm going to defer discussing the complexity of the solution but my most prominent question is that our main CustomerIO instance access the device token through DeviceTokenManager synchronously while DeviceTokenManager loads the initial value of the token in the background.
Why do we think there is a guarantee that for the initial load in the case of a token being saved locally, the call to registeredDeviceToken will happen after the DeviceTokenManager.init runs have loaded its value?
There was a problem hiding this comment.
Thats a fair questions, I avoided tackling it in this PR as its already big, the next PR tackles this, introduces another API thats async, thats how it should be. We should not provide this value synchronously just like FCM only provides token only async.
I would ideally want to remove this API, instead of deprecating it as keeping it comes as a cost, but if we wanted to keep it then we can make the synchronous request and get the device token for it, until we decide to remove it.
| deviceTokenManager.setDeviceToken(givenToken) | ||
|
|
||
| deviceTokenManager.deviceToken shouldBeEqualTo givenToken | ||
| } |
There was a problem hiding this comment.
The point I mention above manifests in the tests here, we consider the state of givenValidToken as token being set using DeviceTokenManager.setDeviceToken() but not from saved GlobalPreferenceStore
What happens if the token isn't set through DeviceTokenManager.setDeviceToken() but exists saved in GlobalPreferenceStore, the init call loads the token on a background thread but calls to deviceTokenManager.deviceToken don't necessarily wait for that value to be loaded?
Am i getting this correctly?
Ahmed-Ali
left a comment
Ahmed-Ali
left a comment
There was a problem hiding this comment.
Let's hold on for this one till we get better clarity on what the StrictMode violations were, and how are we approaching them holistically. Specially with the incomplete clarity on how the device token storage will eventually look like
closes: https://linear.app/customerio/issue/MBL-1231/refactor-sdk-initialization-component-to-avoid-main-thread-usage
Summary
Migrates device token management from
SharedPreferencestoDataStoreto eliminate main thread violations discovered via StrictMode and synchronous access dealing with saving/retrieving data from storage. IntroducesDeviceTokenManageras single source of truth with thread-safe access patterns.Context & Problem
StrictMode violations revealed that SharedPreferences calls in
GlobalPreferenceStorewere blocking the main thread during SDK initialization. Device token access during event processing also had possible race condition risks with concurrent operations.Key Changes
Performance Improvements
Test Coverage