Changed default UserDefaults from .standard to our own Suite#2046
Merged
Conversation
Contributor
Author
|
Thanks! |
283de18 to
18f14d6
Compare
Contributor
Author
|
I just did some extra testing with PurchaseTester and this is working as expected. |
aboedo
approved these changes
Nov 21, 2022
aboedo
left a comment
Member
There was a problem hiding this comment.
Looks awesome! Left a question re: DI
Comment on lines
18
to
30
Member
There was a problem hiding this comment.
This feels like an odd workaround to solve a problem that the rest of the project solves with DI - we even pass in the userDefaults when initializing purchases.
Not saying that doing all the DI in one place is necessarily better, but it does have the advantage of looking exactly the same in all our SDKs, whereas this would be probably impossible to achieve in most languages.
Contributor
Author
There was a problem hiding this comment.
That's a good point, we don't need to cache this because we only call it once!
7a2f700 to
7dffafc
Compare
NachoSoto
added a commit
that referenced
this pull request
Nov 23, 2022
**This is an automatic release.** ### Bugfixes * Changed default `UserDefaults` from `.standard` to our own Suite (#2046) via NachoSoto (@NachoSoto) ### Other Changes * `Logging`: added log when configuring SDK in observer mode (#2065) via NachoSoto (@NachoSoto) * `PurchaseTesterSwiftUI`: added observer mode setting (#2052) via NachoSoto (@NachoSoto) * Exposed `SystemInfo.observerMode` to simplify code (#2064) via NachoSoto (@NachoSoto) * `Result.init(catching:)` with `async` method (#2060) via NachoSoto (@NachoSoto) * Updated schemes and project for Xcode 14.1 (#2081) via NachoSoto (@NachoSoto) * `PurchasesSubscriberAttributesTests`: simplified tests (#2056) via NachoSoto (@NachoSoto) * `DeviceCache`: removed `fatalError` for users not overriding `UserDefaults` (#2079) via NachoSoto (@NachoSoto) * `DeviceCache`: changed `NotificationCenter` observation to be received on posting thread (#2078) via NachoSoto (@NachoSoto) * `StoreKit1Wrapper`: added instance address when detecting transactions (#2055) via NachoSoto (@NachoSoto) * Fixed lint issues with `SwiftLint 0.5.0` (#2076) via NachoSoto (@NachoSoto) * `NSData+RCExtensionsTests`: improved errors (#2043) via NachoSoto (@NachoSoto) * `APITester`: fixed warning in `SubscriptionPeriodAPI` (#2054) via NachoSoto (@NachoSoto) * `Integration Tests`: always run them in random order locally (#2068) via NachoSoto (@NachoSoto) Co-authored-by: NachoSoto <ignaciosoto90@gmail.com>
NachoSoto
added a commit
that referenced
this pull request
Mar 24, 2023
Fixes #2252, #2364, RevenueCat/react-native-purchases#579, [SDKONCALL-214], [SDKONCALL-238], [SDKONCALL-241]. See also #2078 and #2079. ### Background #2078 solved a potential deadlock in `DeviceCache`. This could happen when a background thread was in the middle of writing to `UserDefaults` through `DeviceCache`, and at the same time, the main thread wanted to read from it. This is normally fine, except that if there are any `NotificationCenter` observations on the `UserDefaults` used, the background thread would then post that notification and wait for the main thread, leading to a deadlock. What #2078 did was to use a different method to make sure that the observation was received in the calling thread. Additionally, #2079 avoided that observation whenever we used a different `UserDefaults`. ### Problem When the SDK changed to a custom `UserDefaults` (#2046), we made the choice to only do this if there was no prior data. However, if an app has prior RevenueCat data saved in `UserDefaults.standard`, that instance is used. This potential deadlock is possible for any app with prior data, that also contains observations to `UserDefaults`, either manually through `NotificationCenter`, or using something like `@AppStorage` with `SwiftUI`. ### Solution `DeviceCache.cachedAppUserID` is a very commonly used method. This PR changes it to an in-memory cache, initialized to the `UserDefaults` value. By doing that, most code paths that rely on this value won't risk deadlocking with `UserDefaults` writes that jump threads. ### Long term I've filed SDK-3034 to fully migrate data from `UserDefaults.standard` to avoid this problem altogether in the future.
This was referenced Mar 24, 2023
NachoSoto
added a commit
that referenced
this pull request
Mar 24, 2023
Fixes #2252, #2364, RevenueCat/react-native-purchases#579, [SDKONCALL-214], [SDKONCALL-238], [SDKONCALL-241]. See also #2078 and #2079. ### Background #2078 solved a potential deadlock in `DeviceCache`. This could happen when a background thread was in the middle of writing to `UserDefaults` through `DeviceCache`, and at the same time, the main thread wanted to read from it. This is normally fine, except that if there are any `NotificationCenter` observations on the `UserDefaults` used, the background thread would then post that notification and wait for the main thread, leading to a deadlock. What #2078 did was to use a different method to make sure that the observation was received in the calling thread. Additionally, #2079 avoided that observation whenever we used a different `UserDefaults`. ### Problem When the SDK changed to a custom `UserDefaults` (#2046), we made the choice to only do this if there was no prior data. However, if an app has prior RevenueCat data saved in `UserDefaults.standard`, that instance is used. This potential deadlock is possible for any app with prior data, that also contains observations to `UserDefaults`, either manually through `NotificationCenter`, or using something like `@AppStorage` with `SwiftUI`. ### Solution `DeviceCache.cachedAppUserID` is a very commonly used method. This PR changes it to an in-memory cache, initialized to the `UserDefaults` value. By doing that, most code paths that rely on this value won't risk deadlocking with `UserDefaults` writes that jump threads. ### Long term I've filed SDK-3034 to fully migrate data from `UserDefaults.standard` to avoid this problem altogether in the future.
NachoSoto
added a commit
that referenced
this pull request
Mar 24, 2023
Fixes #2252, #2364, [SDKONCALL-214], [SDKONCALL-241]. See also #2078 and #2079. ### Background #2078 solved a potential deadlock in `DeviceCache`. This could happen when a background thread was in the middle of writing to `UserDefaults` through `DeviceCache`, and at the same time, the main thread wanted to read from it. This is normally fine, except that if there are any `NotificationCenter` observations on the `UserDefaults` used, the background thread would then post that notification and wait for the main thread, leading to a deadlock. What #2078 did was to use a different method to make sure that the observation was received in the calling thread. Additionally, #2079 avoided that observation whenever we used a different `UserDefaults`. ### Problem When the SDK changed to a custom `UserDefaults` (#2046), we made the choice to only do this if there was no prior data. However, if an app has prior RevenueCat data saved in `UserDefaults.standard`, that instance is used. This potential deadlock is possible for any app with prior data, that also contains observations to `UserDefaults`, either manually through `NotificationCenter`, or using something like `@AppStorage` with `SwiftUI`. ### Solution `DeviceCache.cachedAppUserID` is a very commonly used method. This PR changes it to an in-memory cache, initialized to the `UserDefaults` value. By doing that, most code paths that rely on this value won't risk deadlocking with `UserDefaults` writes that jump threads. ### Long term I've filed SDK-3034 to fully migrate data from `UserDefaults.standard` to avoid this problem altogether in the future. [SDKONCALL-214]: https://revenuecats.atlassian.net/browse/SDKONCALL-214?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [SDKONCALL-238]: https://revenuecats.atlassian.net/browse/SDKONCALL-238?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [SDKONCALL-241]: https://revenuecats.atlassian.net/browse/SDKONCALL-241?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
NachoSoto
added a commit
that referenced
this pull request
Apr 10, 2023
Fixes #2252, #2364, [SDKONCALL-214], [SDKONCALL-241]. See also #2078 and #2079. when a background thread was in the middle of writing to `UserDefaults` through `DeviceCache`, and at the same time, the main thread wanted to read from it. This is normally fine, except that if there are any `NotificationCenter` observations on the `UserDefaults` used, the background thread would then post that notification and wait for the main thread, leading to a deadlock. What #2078 did was to use a different method to make sure that the observation was received in the calling thread. Additionally, #2079 avoided that observation whenever we used a different `UserDefaults`. When the SDK changed to a custom `UserDefaults` (#2046), we made the choice to only do this if there was no prior data. However, if an app has prior RevenueCat data saved in `UserDefaults.standard`, that instance is used. This potential deadlock is possible for any app with prior data, that also contains observations to `UserDefaults`, either manually through `NotificationCenter`, or using something like `@AppStorage` with `SwiftUI`. `DeviceCache.cachedAppUserID` is a very commonly used method. This PR changes it to an in-memory cache, initialized to the `UserDefaults` value. By doing that, most code paths that rely on this value won't risk deadlocking with `UserDefaults` writes that jump threads. I've filed SDK-3034 to fully migrate data from `UserDefaults.standard` to avoid this problem altogether in the future. [SDKONCALL-214]: https://revenuecats.atlassian.net/browse/SDKONCALL-214?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [SDKONCALL-238]: https://revenuecats.atlassian.net/browse/SDKONCALL-238?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [SDKONCALL-241]: https://revenuecats.atlassian.net/browse/SDKONCALL-241?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes CSDK-16.
See https://docs.google.com/document/d/1DDyed98RFpIvUwUPJcetc7X4fBhDzf-r1trC4g3zf4c/edit# for reasoning and documentation.
Motivation
The RevenueCat SDK has always used
UserDefaults.standardas the default value. The advantage of this was simplicity, however, this brings a few challenges.Issues
UserDefaultsis itself thread-safe, our users might be observingUserDefaults.defaultschanges viaKVO,NotificationCenter, or@AppStorage(see crash: EXC_BAD_ACCESS in 4.2.1, DeviceCache.swift - Line 123 #1527). We updateUserDefaultsfrom background threads, and users might be getting updates on those threads without handling thread-safety correctly.UserDefaults(see https://rev.cat/userdefaults-crash).Using a custom
UserDefaultssolves all these issues.Change
The new
UserDefaults.revenueCatSuiteis used when:UserDefaultsparameterUserDefaults.standardhas no value forcom.revenuecat.userdefaults.appUserID.new(the user hasn't used the SDK in that app before).Logged during app start:
Alternatives Considered
See https://docs.google.com/document/d/1c2B4MNAFKs_DcFJ3OE_KnmryHCArBkjlZbAD9_kSXu0/edit#
We considered also migrating data for everyone into the new
UserDefaults.This new approach is much simpler, less error prone, easier to implement, and maintain.
The only downside is that existing users won’t benefit from this change, but @aboedo correctly pointed out: if the app is working for them, no reason to change anything.
So in this way, this becomes a purely forward-looking improvement.