DeviceCache: removed fatalError for users not overriding UserDefaults
#2079
Conversation
This file contains 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
Base automatically changed from
device-cache-synchronous-user-defaults-observation
to
main
November 22, 2022 21:39
NachoSoto
force-pushed
the
device-cache-remove-fatal-error
branch
from
50cbee6
to
3dce90f
Compare
aboedo
approved these changes
Nov 22, 2022
Comment on lines
+62
to
+64
| // leave it in an undetermined state. See https://rev.cat/userdefaults-crash | ||
| // If the user is not using a custom `UserDefaults`, we don't need to | ||
| // because they have no access to it. |
There was a problem hiding this comment.
the good news is that folks who are setting their custom user defaults for revenuecat usually know not to delete its contents, so the code will likely very rarely run
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.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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 dangerousfatalError, and makes CSDK-519 less likely.