Changed default `UserDefaults` from `.standard` to our own Suite #2046

NachoSoto · 2022-11-09T19:29:02Z

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.standard as the default value. The advantage of this was simplicity, however, this brings a few challenges.

Issues

While UserDefaults is itself thread-safe, our users might be observing UserDefaults.defaults changes via KVO, NotificationCenter, or @AppStorage (see crash: EXC_BAD_ACCESS in 4.2.1, DeviceCache.swift - Line 123 #1527). We update UserDefaults from background threads, and users might be getting updates on those threads without handling thread-safety correctly.
Users can accidentally delete data from the shared UserDefaults (see https://rev.cat/userdefaults-crash).

Using a custom UserDefaults solves all these issues.

Change

The new UserDefaults.revenueCatSuite is used when:

Only if users aren’t overriding the UserDefaults parameter
Only if UserDefaults.standard has no value for com.revenuecat.userdefaults.appUserID.new (the user hasn't used the SDK in that app before).

Logged during app start:

PurchaseTester[35110:6323140] DEBUG: ℹ️ Configuring SDK using RevenueCat's UserDefaults suite.

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.

vegaro

LGTM!

NachoSoto · 2022-11-14T17:46:30Z

Thanks!
I'll leave this up for a few more days since it's an important change, also in case @aboedo wants to take a look at it.

NachoSoto · 2022-11-16T21:45:44Z

I just did some extra testing with PurchaseTester and this is working as expected.

aboedo

Looks awesome! Left a question re: DI

aboedo · 2022-11-21T22:06:10Z

Sources/FoundationExtensions/UserDefaults+Extensions.swift

+    #if DEBUG
+    /// The "default" `UserDefaults` to use for the SDK.
+    ///
+    /// Moving foward, this default is `UserDefaults.revenueCatSuite`,
+    /// but existing users will continue using `UserDefaults.standard` for compatibility.
+    /// This is determined by the presence of an app user ID in `UserDefaults.standard`.
+    ///
+    /// Computed property only in `DEBUG` to be able to test it under different conditions.
+    /// In release mode, it gets cached.
+    static var `default`: UserDefaults { .computeDefault() }
+    #else
+    static let `default`: UserDefaults = .computeDefault()
+    #endif


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.

That's a good point, we don't need to cache this because we only call it once!

Sources/FoundationExtensions/UserDefaults+Extensions.swift

…aults` We can do this now thanks to #2046. Solves #1906. We no longer need to do this check if we're using our own `UserDefaults`, which simplifies the code, avoids a dangerous `fatalError`, and makes [CSDK-519] less likely.

@NachoSoto

**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>

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.

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

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

NachoSoto added the fix A bug fix label Nov 9, 2022

NachoSoto requested a review from a team November 9, 2022 19:29

vegaro approved these changes Nov 14, 2022

View reviewed changes

NachoSoto requested a review from a team November 14, 2022 17:45

NachoSoto requested a review from aboedo November 16, 2022 21:26

NachoSoto force-pushed the user-defaults-default branch from 283de18 to 18f14d6 Compare November 16, 2022 21:31

aboedo approved these changes Nov 21, 2022

View reviewed changes

NachoSoto added 7 commits November 22, 2022 11:06

UserDefaults.default

25df5ee

ETagManager: added explanation

8443fad

DeviceCache: non-optional UserDefaults

759f650

Using UserDefaults.default

c936337

Finished tests

09728cf

Improved docstring

b886e8f

Added logs

7dffafc

NachoSoto force-pushed the user-defaults-default branch from 7a2f700 to 7dffafc Compare November 22, 2022 19:06

NachoSoto added 2 commits November 22, 2022 11:22

Removed cached UserDefaults.default

90836de

Improved log

69196bf

NachoSoto enabled auto-merge (squash) November 22, 2022 19:24

NachoSoto merged commit ecce3ff into main Nov 22, 2022

NachoSoto deleted the user-defaults-default branch November 22, 2022 19:32

NachoSoto mentioned this pull request Nov 22, 2022

DeviceCache: removed fatalError for users not overriding UserDefaults #2079

Merged

RCGitBot mentioned this pull request Nov 23, 2022

[AUTOMATIC] Release/4.14.3 #2083

Merged

NachoSoto mentioned this pull request Feb 14, 2023

UserDefaultsDefaultTests: fixed flaky failures #2284

Merged

NachoSoto mentioned this pull request Mar 24, 2023

DeviceCache: workaround for potential deadlock #2375

Merged

This was referenced Mar 24, 2023

Revenuecat hangs the UI and the app crashes because of memory #2252

Closed

4.17.4 Breaks purchases y and now continue breaking #2364

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed default `UserDefaults` from `.standard` to our own Suite #2046

Changed default `UserDefaults` from `.standard` to our own Suite #2046

NachoSoto commented Nov 9, 2022 •

edited

vegaro left a comment

NachoSoto commented Nov 14, 2022 •

edited

NachoSoto commented Nov 16, 2022

aboedo left a comment

aboedo Nov 21, 2022

NachoSoto Nov 22, 2022

NachoSoto Nov 22, 2022

Changed default UserDefaults from .standard to our own Suite #2046

Changed default UserDefaults from .standard to our own Suite #2046

Conversation

NachoSoto commented Nov 9, 2022 • edited

Motivation

Issues

Change

Logged during app start:

Alternatives Considered

vegaro left a comment

Choose a reason for hiding this comment

NachoSoto commented Nov 14, 2022 • edited

NachoSoto commented Nov 16, 2022

aboedo left a comment

Choose a reason for hiding this comment

aboedo Nov 21, 2022

Choose a reason for hiding this comment

NachoSoto Nov 22, 2022

Choose a reason for hiding this comment

NachoSoto Nov 22, 2022

Choose a reason for hiding this comment

Changed default `UserDefaults` from `.standard` to our own Suite #2046

Changed default `UserDefaults` from `.standard` to our own Suite #2046

NachoSoto commented Nov 9, 2022 •

edited

NachoSoto commented Nov 14, 2022 •

edited