`BasePurchasesTests`: detecting and fixing many `DeviceCache` leaks by NachoSoto · Pull Request #2105 · RevenueCat/purchases-ios

NachoSoto · 2022-11-30T01:29:16Z

Follow up to #2104. See also #2101. This has uncovered a huge source of leaks across many tests. Fixing this provides many benefits:

Faster test runs
Lower memory overhead
Isolated state across each test run
Ensure that these types don't leak outside of tests too

NachoSoto · 2022-11-30T01:29:27Z

There's still a few tests failing that I'm trying to figure out why.

NachoSoto · 2022-11-30T01:37:16Z

 class PurchaseDeferredPurchasesTests: BasePurchasesTests {

-    private var storeKit1WrapperDelegate: StoreKit1WrapperDelegate!
+    private var storeKit1WrapperDelegate: StoreKit1WrapperDelegate {


We were creating retain cycles with this.

NachoSoto · 2022-11-30T01:37:23Z

 class PurchaseDeferredPurchasesSK2Tests: BasePurchasesTests {

-    private var paymentQueueWrapperDelegate: PaymentQueueWrapperDelegate!
+    private var paymentQueueWrapperDelegate: PaymentQueueWrapperDelegate {


Another retain cycle.

NachoSoto · 2022-11-30T01:37:30Z

 class MockNotificationCenter: NotificationCenter {

-    typealias ObserversWithSelector = (observer: AnyObject,
+    typealias ObserversWithSelector = (observer: WeakBox<AnyObject>,


This was leaking objects.

NachoSoto · 2022-11-30T01:38:00Z

+            self.mockOperationDispatcher = nil
+            self.paymentQueueWrapper = nil
+            self.requestFetcher = nil
+            self.receiptFetcher = nil
+            self.mockProductsManager = nil
+            self.mockIntroEligibilityCalculator = nil
+            self.mockTransactionsManager = nil
+            self.backend = nil
+            self.attributionFetcher = nil
+            self.purchasesDelegate.makeDeferredPurchase = nil
+            self.purchasesDelegate = nil
+            self.storeKit1Wrapper.delegate = nil
+            self.storeKit1Wrapper = nil
+            self.systemInfo = nil
+            self.notificationCenter = nil
+            self.subscriberAttributesManager = nil
+            self.trialOrIntroPriceEligibilityChecker = nil
+            self.attributionPoster = nil
+            self.attribution = nil
+            self.customerInfoManager = nil
+            self.identityManager = nil
+            self.mockOfferingsManager = nil
+            self.mockManageSubsHelper = nil
+            self.mockBeginRefundRequestHelper = nil
+            self.purchasesOrchestrator = nil


This is a lot, but it's been the only way to remove one by one the references at the end of the tests to see what's leaking. And there's still some retain cycle somewhere!

were leaks in tests only or also in the main SDK?

So far the ones I've found are only in tests, but DeviceCache is still leaking. I need to spend a bit more time to find the last cause, which might be in the main SDK.

NachoSoto · 2022-11-30T01:38:22Z


    func testFinishesTransactionsIfSentToBackendCorrectly() throws {
-        var finished = true
+        var finished = false


This test was probably not doing anything before :)

Follow up to #2104 and #2105. This ensures that `Purchases` is re-created for each test.

aboedo · 2022-12-07T20:43:52Z

+            self.mockOperationDispatcher = nil
+            self.paymentQueueWrapper = nil
+            self.requestFetcher = nil
+            self.receiptFetcher = nil
+            self.mockProductsManager = nil
+            self.mockIntroEligibilityCalculator = nil
+            self.mockTransactionsManager = nil
+            self.backend = nil
+            self.attributionFetcher = nil
+            self.purchasesDelegate.makeDeferredPurchase = nil
+            self.purchasesDelegate = nil
+            self.storeKit1Wrapper.delegate = nil
+            self.storeKit1Wrapper = nil
+            self.systemInfo = nil
+            self.notificationCenter = nil
+            self.subscriberAttributesManager = nil
+            self.trialOrIntroPriceEligibilityChecker = nil
+            self.attributionPoster = nil
+            self.attribution = nil
+            self.customerInfoManager = nil
+            self.identityManager = nil
+            self.mockOfferingsManager = nil
+            self.mockManageSubsHelper = nil
+            self.mockBeginRefundRequestHelper = nil
+            self.purchasesOrchestrator = nil


were leaks in tests only or also in the main SDK?

aboedo · 2022-12-07T20:44:52Z


    func testFinishesTransactionsIfSentToBackendCorrectly() throws {
-        var finished = true
+        var finished = false


…llbacks See also #2135. I'm looking into the leak in #2105, and I have a theory that it's due to callbacks being stored in this cache, but sometimes not being dequeued, so leaking all references in them. This assertion will only run in RC tests, and will ensure that this doesn't happen.

Follow up to #2100. This has uncovered a huge source of leaks across many tests. Fixing this will provide many benefits: - Faster test runs - Lower memory overhead - Isolated state across each test run - Ensure that these types don't leak outside of tests too

NachoSoto · 2022-12-12T21:35:17Z

Tests are passing now 🎉

…llbacks (#2137) See also #2135. I'm looking into the leak in #2105, and I have a theory that it's due to callbacks being stored in this cache, but sometimes not being dequeued, so leaking all references in them. This assertion will only run in RC tests, and will ensure that this doesn't happen.

…for cache hits - Created `CacheableNetworkOperationFactory` to abstract operation and cache key creation - Instead of always creating an operation, we create a factory first, which pre-computes the cache key - This cache key is used to determine if it's a cache hit, as well as for the subsequently created operation I'm still investigating a leak (#2105), and I suspect it's coming from us creating and storing completion blocks, and those staying in memory beyond the lifetime of the operation. This refactor and performance improvement helps us avoid get more things in memory than we need to.

@NachoSoto

**This is an automatic release.** ### Bugfixes * Un-deprecate `Purchases.configure(withAPIKey:appUserID:)` and `Purchases.configure(withAPIKey:appUserID:observerMode:)` (#2129) via NachoSoto (@NachoSoto) ### Other Changes * `ReceiptFetcherTests`: refactored tests using `waitUntilValue` (#2144) via NachoSoto (@NachoSoto) * Added a few performance improvements for `ReceiptParser` (#2124) via NachoSoto (@NachoSoto) * `CallbackCache`: fixed reference (#2143) via NachoSoto (@NachoSoto) * `PostReceiptDataOperation`: clarified receipt debug log (#2128) via NachoSoto (@NachoSoto) * `CallbackCache`: avoid exposing internal mutable cache (#2136) via NachoSoto (@NachoSoto) * `CallbackCache`: added assertion for tests to ensure we don't leak callbacks (#2137) via NachoSoto (@NachoSoto) * `NetworkOperation`: made `Atomic` references immutable (#2139) via NachoSoto (@NachoSoto) * `ReceiptParser`: ensure parsing never happens in the main thread (#2123) via NachoSoto (@NachoSoto) * `PostReceiptDataOperation`: also print receipt data with `verbose` logs (#2127) via NachoSoto (@NachoSoto) * `BasePurchasesTests`: detecting and fixing many `DeviceCache` leaks (#2105) via NachoSoto (@NachoSoto) * `StoreKitIntegrationTests`: added test to check applying a promotional offer during subscription (#1588) via NachoSoto (@NachoSoto)

…for cache hits (#2135) ### Changes: - Created `CacheableNetworkOperationFactory` to abstract operation and cache key creation - Instead of always creating an operation, we create a factory first, which pre-computes the cache key - This cache key is used to determine if it's a cache hit, as well as for the subsequently created operation I'm still investigating a leak (#2105), and I suspect it's coming from us creating and storing completion blocks, and those staying in memory beyond the lifetime of the operation. This refactor and performance improvement helps us avoid get more things in memory than we need to. See #2138 for how this is tested.

NachoSoto added WIP test labels Nov 30, 2022

NachoSoto requested a review from a team November 30, 2022 01:29

NachoSoto mentioned this pull request Nov 30, 2022

Integration Tests: verify Purchases does not leak across tests #2106

Merged

NachoSoto commented Nov 30, 2022

View reviewed changes

NachoSoto added a commit that referenced this pull request Nov 30, 2022

Integration Tests: verify Purchases does not leak across tests

3250201

Follow up to #2104 and #2105. This ensures that `Purchases` is re-created for each test.

aboedo approved these changes Dec 7, 2022

View reviewed changes

NachoSoto force-pushed the purchase-leaking-test branch from ec006ee to 425de93 Compare December 9, 2022 01:24

Base automatically changed from purchase-leaking-test to main December 9, 2022 01:36

NachoSoto force-pushed the device-cache-leaking-test branch 2 times, most recently from c4963bd to 5f9e7c8 Compare December 12, 2022 18:28

NachoSoto mentioned this pull request Dec 12, 2022

CacheableNetworkOperation: avoid unnecessarily creating operations for cache hits #2135

Merged

NachoSoto mentioned this pull request Dec 12, 2022

CallbackCache: added assertion for tests to ensure we don't leak callbacks #2137

Merged

NachoSoto added 2 commits December 12, 2022 13:34

Fixed leak

9cd046a

NachoSoto force-pushed the device-cache-leaking-test branch from 5f9e7c8 to 9cd046a Compare December 12, 2022 21:35

NachoSoto removed the WIP label Dec 12, 2022

NachoSoto merged commit 52fd952 into main Dec 12, 2022

NachoSoto deleted the device-cache-leaking-test branch December 12, 2022 21:43

RCGitBot mentioned this pull request Dec 14, 2022

[AUTOMATIC] Release/4.15.3 #2149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`BasePurchasesTests`: detecting and fixing many `DeviceCache` leaks#2105

`BasePurchasesTests`: detecting and fixing many `DeviceCache` leaks#2105
NachoSoto merged 2 commits into
mainfrom
device-cache-leaking-test

NachoSoto commented Nov 30, 2022 •

edited

Loading

Uh oh!

NachoSoto commented Nov 30, 2022

Uh oh!

NachoSoto Nov 30, 2022

Uh oh!

NachoSoto Nov 30, 2022

Uh oh!

NachoSoto Nov 30, 2022

Uh oh!

NachoSoto Nov 30, 2022

Uh oh!

aboedo Dec 7, 2022

Uh oh!

NachoSoto Dec 9, 2022

Uh oh!

NachoSoto Nov 30, 2022

Uh oh!

aboedo Dec 7, 2022

Uh oh!

aboedo Dec 7, 2022

Uh oh!

aboedo Dec 7, 2022

Uh oh!

NachoSoto commented Dec 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

NachoSoto commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NachoSoto commented Nov 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto commented Dec 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NachoSoto commented Nov 30, 2022 •

edited

Loading