DeviceCache: workaround for potential deadlock
#2375
Merged
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
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
Codecov Report
@@ Coverage Diff @@
## main #2375 +/- ##
==========================================
- Coverage 86.78% 86.73% -0.05%
==========================================
Files 198 198
Lines 13206 13204 -2
==========================================
- Hits 11461 11453 -8
- Misses 1745 1751 +6
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
NachoSoto
commented
Mar 24, 2023
| let currentAppUserIDLooksAnonymous = Self.userIsAnonymous(self.currentAppUserID) | ||
| let isLegacyAnonymousAppUserID = self.currentAppUserID == self.deviceCache.cachedLegacyAppUserID | ||
| lazy var currentAppUserIDLooksAnonymous = Self.userIsAnonymous(self.currentAppUserID) | ||
| lazy var isLegacyAnonymousAppUserID = self.currentAppUserID == self.deviceCache.cachedLegacyAppUserID |
There was a problem hiding this comment.
This makes it so that isLegacyAnonymousAppUserID might not have to be read if the first one is already true.
NachoSoto
force-pushed
the
device-cache-deadlock
branch
from
5b2c479
to
2d14f97
Compare
vegaro
approved these changes
Mar 24, 2023
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
5 tasks
NachoSoto
added a commit
that referenced
this pull request
May 12, 2023
… not blocked (#2463) We've had a few reports of deadlocks (#2412, #2375). This might have not detected them, but it would detect future issues, as well as busy operations running on the main thread. Example: 
NachoSoto
added a commit
that referenced
this pull request
May 25, 2023
… not blocked (#2463) We've had a few reports of deadlocks (#2412, #2375). This might have not detected them, but it would detect future issues, as well as busy operations running on the main thread. Example: 
NachoSoto
added a commit
that referenced
this pull request
Jul 14, 2023
This was introduced in #2463, as a way to verify the SDK didn't have any deadlocks (#2412, #2375). However, it has caused more trouble than it's worth, because that loop keeps the main thread busy. This solution proposed by @aboedo makes it only check every 3 seconds. If there is a deadlock, it would still detect it. But after it's verified there is no deadlock, it won't check again until that interval has elapsed again. This makes it so that on a test that takes 6 seconds to run, we only execute this code 2 times instead of... a lot.
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.
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 toUserDefaultsthroughDeviceCache, and at the same time, the main thread wanted to read from it.This is normally fine, except that if there are any
NotificationCenterobservations on theUserDefaultsused, 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 throughNotificationCenter, or using something like@AppStoragewithSwiftUI.Solution
DeviceCache.cachedAppUserIDis a very commonly used method. This PR changes it to an in-memory cache, initialized to theUserDefaultsvalue.By doing that, most code paths that rely on this value won't risk deadlocking with
UserDefaultswrites that jump threads.Long term
I've filed SDK-3034 to fully migrate data from
UserDefaults.standardto avoid this problem altogether in the future.