BasePurchasesTests: fixed leak detection
#2534
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
While working on #2533, I knew that my proof of concept had a retain cycle. I however was very confused when it wasn't detected by this mechanism introduced first introduced in #2104. Turns out that this had been wrong the whole time: the current implementation, as explained by the comment, was executing those blocks in LIFO order, which meant that the references were being set to `nil` before the assertions could be created. This meant that by the time the `except { [weak purchases = self.purchases] ... }` lines were called, `self.purchases` itself was `nil`, so nothing was being checked. This simplifies the implementation by inlining the checks, and saving a `weak` reference before clearing the singleton and all the local references. Luckily there was only one failing test that had to do with how `Purchases` was being created, and implementation details of `expectFatalError`. To avoid this, I changed the test to use the `Purchases.init` method directly.
|
And now this correctly detects the retain cycle I introduced 🎉 |
Codecov Report
@@ Coverage Diff @@
## main #2534 +/- ##
=======================================
Coverage 87.76% 87.77%
=======================================
Files 199 199
Lines 13710 13721 +11
=======================================
+ Hits 12032 12043 +11
Misses 1678 1678
|
tonidero
approved these changes
May 24, 2023
| } | ||
| weak var purchases = self.purchases | ||
| weak var orchestrator = self.purchasesOrchestrator | ||
| weak var deviceCache = self.deviceCache |
There was a problem hiding this comment.
Should we rename these 3 so there is less confusion with the global variables?
There was a problem hiding this comment.
You'd rename them to weakPurchases, etc?
There was a problem hiding this comment.
They're limited to the scope of this method so I think it's clear that they're shadowing those variables. But let me know if you disagree.
NachoSoto
added a commit
that referenced
this pull request
May 30, 2023
…2533) This fixes the last edge-case for offline entitlements. Solves the following scenario: ```swift func testPurchaseWhileServerIsDownPostsReceiptWhenForegroundingApp() async throws { let logger = TestLogHandler() // 1. Purchase while server is down self.serverDown() try await self.purchaseMonthlyProduct() logger.verifyMessageWasNotLogged("Finishing transaction") // 2. Server is back self.serverUp() // 3. Request current CustomerInfo let info1 = try await Purchases.shared.customerInfo() try await self.verifyEntitlementWentThrough(info1) // 4. Ensure transaction is finished logger.verifyMessageWasLogged("Finishing transaction", level: .info) // 5. Restart app Purchases.shared.invalidateCustomerInfoCache() await self.resetSingleton() // 6. To ensure (with a clean cache) that the receipt was posted let info2 = try await Purchases.shared.customerInfo() try await self.verifyEntitlementWentThrough(info2) } ``` This race condition happened when fetching `CustomerInfo` after we had computed an offline `CustomerInfo` with a purchase: if the server is still not aware of the purchase, then we lose that until some arbitrary amount of time when `StoreKit` might decide to send the pending transactions in the queue. For that reason, instead of waiting, we proactively check now whenever `CustomerInfoManager` fetches new info. The refactors #2540 and #2542 where required to break a retain cycle. Also thanks to #2534 we know for sure that there aren't any cycles. Note that this is an iOS 15.0+ only feature. It would be useful to better ensure consistency in older versions, but it's not strictly required, as those versions don't support offline entitlements either.
This was referenced May 31, 2023
Closed
NachoSoto
added a commit
that referenced
this pull request
Jun 1, 2023
**This is an automatic release.** ### New Features * `Offline Entitlements`: use offline-computed `CustomerInfo` when server is down (#2368) via NachoSoto (@NachoSoto) ### Bugfixes * `AppleReceipt.debugDescription`: don't pretty-print JSON (#2564) via NachoSoto (@NachoSoto) * `SK2StoreProduct`: fix crash on iOS 12 (#2565) via NachoSoto (@NachoSoto) * `GetCustomerInfo` posts receipts if there are pending transactions (#2533) via NachoSoto (@NachoSoto) ### Performance Improvements * `PurchasedProductsFetcher`: cache current entitlements (#2507) via NachoSoto (@NachoSoto) * Performance: new check to ensure serialization / deserialization is done from background thread (#2496) via NachoSoto (@NachoSoto) ### Dependency Updates * Bump fastlane from 2.212.2 to 2.213.0 (#2544) via dependabot[bot] (@dependabot[bot]) ### Other Changes * `CustomerInfoManager`: post all unfinished transactions (#2563) via NachoSoto (@NachoSoto) * `PostReceiptOperation`: added ability to also post `AdServices` token (#2566) via NachoSoto (@NachoSoto) * `Offline Entitlements`: improved computation log (#2562) via NachoSoto (@NachoSoto) * Added `TransactionPoster` tests (#2557) via NachoSoto (@NachoSoto) * Refactored `TransactionPoster`: removed 2 dependencies and abstracted parameters (#2542) via NachoSoto (@NachoSoto) * `CustomerInfoManagerTests`: wait for `getAndCacheCustomerInfo` to finish (#2555) via NachoSoto (@NachoSoto) * `StoreTransaction`: implemented `description` (#2556) via NachoSoto (@NachoSoto) * `Backend.ResponseHandler` is now `@Sendable` (#2541) via NachoSoto (@NachoSoto) * Extracted `TransactionPoster` from `PurchasesOrchestrator` (#2540) via NachoSoto (@NachoSoto) * `enableAdServicesAttributionTokenCollection`: added integration test (#2551) via NachoSoto (@NachoSoto) * `AttributionPoster`: replaced hardcoded strings with constants (#2548) via NachoSoto (@NachoSoto) * `DefaultDecodable`: moved to `Misc/Codable/DefaultDecodable.swift` (#2528) via NachoSoto (@NachoSoto) * `CircleCI`: specify device to run `backend_integration_tests` (#2547) via NachoSoto (@NachoSoto) * Created `StoreKit2TransactionFetcher` (#2539) via NachoSoto (@NachoSoto) * Fix load shedder integration tests (#2546) via Josh Holtz (@joshdholtz) * Fix doc on `Offering.getMetadataValue` (#2545) via Josh Holtz (@joshdholtz) * Extracted and tested `AsyncSequence.extractValues` (#2538) via NachoSoto (@NachoSoto) * `Offline Entitlements`: don't compute offline `CustomerInfo` when purchasing a consumable products (#2522) via NachoSoto (@NachoSoto) * `OfflineEntitlementsManager`: disable offline `CustomerInfo` in observer mode (#2520) via NachoSoto (@NachoSoto) * `BasePurchasesTests`: fixed leak detection (#2534) via NachoSoto (@NachoSoto) * `PurchaseTesterSwiftUI`: added `ProxyView` to `iOS` (#2531) via NachoSoto (@NachoSoto) * `PurchasedProductsFetcher`: removed `AppStore.sync` call (#2521) via NachoSoto (@NachoSoto) * `PurchaseTesterSwiftUI`: added new window on Mac to manage proxy (#2518) via NachoSoto (@NachoSoto) * `PurchasedProductsFetcher`: added log if fetching purchased products is slow (#2515) via NachoSoto (@NachoSoto) * `Offline Entitlements`: disable for custom entitlements mode (#2509) via NachoSoto (@NachoSoto) * `Offline Entitlements`: fixed iOS 12 tests (#2514) via NachoSoto (@NachoSoto) * `PurchasedProductsFetcher`: don't throw errors if purchased products were found (#2506) via NachoSoto (@NachoSoto) * `Offline Entitlements`: allow creating offline `CustomerInfo` with empty `ProductEntitlementMapping` (#2504) via NachoSoto (@NachoSoto) * `Offline Entitlements`: integration tests (#2501) via NachoSoto (@NachoSoto) * `CustomerInfoManager`: don't cache offline `CustomerInfo` (#2378) via NachoSoto (@NachoSoto) * `DangerousSettings`: debug-only `forceServerErrors` (#2486) via NachoSoto (@NachoSoto) * `CocoapodsInstallation`: fixed `Xcode 14.3.0` issue (#2489) via NachoSoto (@NachoSoto) * `CarthageInstallation`: removed workaround (#2488) via NachoSoto (@NachoSoto) --------- Co-authored-by: NachoSoto <ignaciosoto90@gmail.com>
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.
While working on #2533, I knew that my proof of concept had a retain cycle. I however was very confused when it wasn't detected by this mechanism introduced first introduced in #2104.
Turns out that this had been wrong the whole time: the current implementation, as explained by the comment, was executing those blocks in LIFO order, which meant that the references were being set to
nilbefore the assertions could be created.This meant that by the time the
except { [weak purchases = self.purchases] ... }lines were called,self.purchasesitself wasnil, so nothing was being checked.This simplifies the implementation by inlining the checks, and saving a
weakreference before clearing the singleton and all the local references.Luckily there was only one failing test that had to do with how
Purchaseswas being created, and implementation details ofexpectFatalError. To avoid this, I changed the test to use thePurchases.initmethod directly.