DeviceCache: changed NotificationCenter observation to be received on posting thread#2078
Merged
NachoSoto merged 2 commits intoNov 22, 2022
Merged
Conversation
aboedo
approved these changes
Nov 22, 2022
…d on posting thread This could solve [CSDK-519], and at the very least it makes the semantics simpler to understand. The traditional `NotificationCenter.addObserver` seems to always lead to a jump to the main thread when modifying `UserDefaults` from any other thread. See: ``` Thread 1: 0 libsystem_kernel.dylib 0x1fa81041c __psynch_cvwait + 8 1 libsystem_pthread.dylib 0x20aa5406c _pthread_cond_wait + 1232 2 Foundation 0x1b85b3800 -[NSOperation waitUntilFinished] + 508 3 CoreFoundation 0x1be16fe0c _CFXNotificationPost + 772 4 Foundation 0x1b85d04dc -[NSNotificationCenter postNotificationName:object:userInfo:] + 92 5 AudioMemos2 0x102ed18a4 specialized closure #1 in DeviceCache.cache(customerInfo:appUserID:) + 972964 (<compiler-generated>:0) ``` There were no potential race condition (and as far as we can tell, no deadlocks possible) because of how synchronization is done, but using the modern `NotificationCenter.addObserver` simplifies this, because `handleUserDefaultsChanged` will now be invoked synchronously in the calling thread.
810476d to
879c4ca
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.
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.
This could solve CSDK-519, and at the very least it makes the semantics simpler to understand.
The traditional
NotificationCenter.addObserverseems to always lead to a jump to the main thread when modifyingUserDefaultsfrom any other thread.See:
There were no potential race condition (and as far as we can tell, no deadlocks possible) because of how synchronization is done, but using the modern
NotificationCenter.addObserversimplifies this, becausehandleUserDefaultsChangedwill now be invoked synchronously in the calling thread.