CustomerInfoManager: fixed deadlock caused by reading CustomerInfo inside of observer#2412
Merged
Merged
Conversation
5 tasks
…` inside of observer Fixes #2410 and RevenueCat/react-native-purchases#579 This adds 2 previously failing tests that reproduce this issue: one directly on `CustomerInfoManager` and another one utilizing `PurchasesDelegate`. These illustrate the issue: if the observer (through `CustomerInfoManager.monitorChanges` or `PurchasesDelegate`) called a method that ended up reading `CustomerInfoManager`'s data inside of the observation, the SDK would deadlock. This is because we were invoking the observers while inside the lock. This was unnecessary, so this fixes the issue by ensuring that the observers receive updates after the lock is released, using a new `OperationDispatcher.dispatchAsyncOnMainThread`.
fb8e171 to
6099ab2
Compare
tonidero
approved these changes
Apr 18, 2023
| // See https://github.com/RevenueCat/purchases-ios/issues/2410 | ||
| func testDelegateWithGetCustomerInfoCallDoesNotDeadlock() throws { | ||
| final class GetCustomerInfoPurchasesDelegate: NSObject, PurchasesDelegate { | ||
| func purchases(_ purchases: Purchases, receivedUpdated customerInfo: CustomerInfo) { |
Contributor
There was a problem hiding this comment.
Should we add a check to make sure this is actually called? To make sure nothing changes that could cause us to miss the important part of the test (this might be part of another test so it might be ok).
Contributor
Author
There was a problem hiding this comment.
Yeah that's part of another test but it doesn't hurt.
vegaro
approved these changes
Apr 18, 2023
aboedo
approved these changes
Apr 18, 2023
Comment on lines
+253
to
+254
| // This must be async to prevent deadlocks if the observer calls a method that ends up reading | ||
| // this class' data. By making it async, the closure is invoked outside of the lock. |
Member
There was a problem hiding this comment.
already asked through slack, just wondering why we need a read lock for customerInfo in the first place
Contributor
Author
There was a problem hiding this comment.
Hmm yeah I think that's leftover from the prior implementation. This whole method could be a lot simpler if we just read the underlying data (info, observers, etc) at the beginning and we do the rest outside of the lock, then we wouldn't need the dispatch async change. I'll do that and it should still make the tests pass 👍🏻
Contributor
Author
There was a problem hiding this comment.
Oh wait we do modify the lastSentCustomerInfo that's why it needs to be thread safe so we don't end up reading and writing to it outside the lock.
5 tasks
NachoSoto
added a commit
that referenced
this pull request
Apr 19, 2023
…` inside of observer (#2412) Fixes #2410 and RevenueCat/react-native-purchases#579. This adds 2 previously failing tests that reproduce this issue: one directly on `CustomerInfoManager` and another one utilizing `PurchasesDelegate`. These illustrate the issue: if the observer (through `CustomerInfoManager.monitorChanges` or `PurchasesDelegate`) called a method that ended up reading `CustomerInfoManager`'s data inside of the observation, the SDK would deadlock. This is because we were invoking the observers while inside the lock. This was unnecessary, so this fixes the issue by ensuring that the observers receive updates after the lock is released, using a new `OperationDispatcher.dispatchAsyncOnMainThread`.
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.
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
4 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 #2410 and RevenueCat/react-native-purchases#579.
This adds 2 previously failing tests that reproduce this issue: one directly on
CustomerInfoManagerand another one utilizingPurchasesDelegate.These illustrate the issue: if the observer (through
CustomerInfoManager.monitorChangesorPurchasesDelegate) called a method that ended up readingCustomerInfoManager's data inside of the observation, the SDK would deadlock.This is because we were invoking the observers while inside the lock. This was unnecessary, so this fixes the issue by ensuring that the observers receive updates after the lock is released, using a new
OperationDispatcher.dispatchAsyncOnMainThread.