-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BasePurchasesTests
: detecting and fixing many DeviceCache
leaks
#2105
Conversation
There's still a few tests failing that I'm trying to figure out why. |
@@ -19,7 +19,12 @@ import XCTest | |||
|
|||
class PurchaseDeferredPurchasesTests: BasePurchasesTests { | |||
|
|||
private var storeKit1WrapperDelegate: StoreKit1WrapperDelegate! | |||
private var storeKit1WrapperDelegate: StoreKit1WrapperDelegate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were creating retain cycles with this.
@@ -145,7 +149,11 @@ class PurchaseDeferredPurchasesTests: BasePurchasesTests { | |||
@available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *) | |||
class PurchaseDeferredPurchasesSK2Tests: BasePurchasesTests { | |||
|
|||
private var paymentQueueWrapperDelegate: PaymentQueueWrapperDelegate! | |||
private var paymentQueueWrapperDelegate: PaymentQueueWrapperDelegate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another retain cycle.
@@ -6,7 +6,7 @@ import Foundation | |||
|
|||
class MockNotificationCenter: NotificationCenter { | |||
|
|||
typealias ObserversWithSelector = (observer: AnyObject, | |||
typealias ObserversWithSelector = (observer: WeakBox<AnyObject>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was leaking objects.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
were leaks in tests only or also in the main SDK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -98,7 +98,7 @@ class PurchasesPurchasingTests: BasePurchasesTests { | |||
} | |||
|
|||
func testFinishesTransactionsIfSentToBackendCorrectly() throws { | |||
var finished = true | |||
var finished = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was probably not doing anything before :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
were leaks in tests only or also in the main SDK?
@@ -98,7 +98,7 @@ class PurchasesPurchasingTests: BasePurchasesTests { | |||
} | |||
|
|||
func testFinishesTransactionsIfSentToBackendCorrectly() throws { | |||
var finished = true | |||
var finished = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦
ec006ee
to
425de93
Compare
c4963bd
to
5f9e7c8
Compare
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
5f9e7c8
to
9cd046a
Compare
Tests are passing now 🎉 |
…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.
**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.
Follow up to #2104. See also #2101. This has uncovered a huge source of leaks across many tests. Fixing this provides many benefits: