TransactionPoster: avoid posting transactions multiple times
#2914
Conversation
NachoSoto
added
Investigating
PostReceipt optimizations
and removed
Investigating
labels
NachoSoto
force-pushed
the
cache-posted-transactions
branch
from
b3ceb83
to
a6777aa
Compare
NachoSoto
force-pushed
the
cache-posted-transactions-2
branch
2 times, most recently
from
6531ed1
to
97ac084
Compare
NachoSoto
changed the base branch from
cache-posted-transactions
to
customer-info-post-transactions-dedup
Codecov Report
@@ Coverage Diff @@
## customer-info-post-transactions-dedup #2914 +/- ##
======================================================================
Coverage 86.64% 86.64%
======================================================================
Files 217 217
Lines 15536 15536
======================================================================
Hits 13461 13461
Misses 2075 2075 |
NachoSoto
force-pushed
the
cache-posted-transactions-2
branch
from
97ac084
to
298081e
Compare
NachoSoto
force-pushed
the
cache-posted-transactions-2
branch
from
298081e
to
bea8876
Compare
|
This is ready now |
| self.paymentQueueWrapper = paymentQueueWrapper | ||
| self.systemInfo = systemInfo | ||
| self.operationDispatcher = operationDispatcher | ||
| } | ||
|
|
||
| // TODO: consumables |
There was a problem hiding this comment.
Gotta consider how this works with #2841.
There was a problem hiding this comment.
maybe we should get both into an integration branch before shipping and test them together?
NachoSoto
force-pushed
the
cache-posted-transactions-2
branch
from
bea8876
to
c64653e
Compare
| expect(self.cache.hasPostedTransaction(transaction)) == true | ||
| } | ||
|
|
||
| func testSaveMultipleTransactions() { |
There was a problem hiding this comment.
we should add a test to verify the behavior if the same transaction gets posted over and over (like if it's not finished)
There was a problem hiding this comment.
basically to ensure that we don't grow the cache forever
There was a problem hiding this comment.
It's stored as a Set, but yeah I can add that.
There was a problem hiding this comment.
Done:
func testHandlePurchasedTransactionMultipleTimesPostsItOnce() throws {
let transactionData = PurchasedTransactionData(
appUserID: "user",
source: .init(isRestore: false, initiationSource: .queue)
)
let count = 10
self.backend.stubbedPostReceiptResult = .success(self.createCustomerInfo(nonSubscriptionProductID: nil))
for _ in 0..<count {
_ = try self.handleTransaction(transactionData)
}
expect(self.backend.invokedPostReceiptDataCount) == 1
expect(self.mockTransaction.finishInvokedCount) == count
expect(self.cache.postedTransactions) == [self.mockTransaction.transactionIdentifier]
}| expect(parameters.data.source.initiationSource) == .queue | ||
| } | ||
|
|
||
| func testFetchesCustomerInfoIfTransactionWasAlreadyPostedButNoCachedCustomerInfo() async throws { |
There was a problem hiding this comment.
🤔
This is a weird case to think about. Not sure fetching customer info is the best path here?
I can only imagine arriving here if:
- we've posted the transaction before
- we call invalidate customer info cache
- we get notified again for an unfinished transaction
Or similar thinking but someone calls restoreCompletedTransactions.
Feels like it'd be safer to just post in this case? Would that be a huge refactor? WDYT?
There was a problem hiding this comment.
That complicates the code unnecessarily. I'd have to give TransactionPoster knowledge of whether there is a cached customer info or not.
NachoSoto
force-pushed
the
customer-info-post-transactions-dedup
branch
from
8d1d751
to
e214dfe
Compare
NachoSoto
force-pushed
the
cache-posted-transactions-2
branch
from
c64653e
to
f0ef51f
Compare
| @@ -1088,7 +1090,7 @@ private extension PurchasesOrchestrator { | |||
| if let completion = self.getAndRemovePurchaseCompletedCallback(forTransaction: purchasedTransaction) { | |||
| self.operationDispatcher.dispatchOnMainActor { | |||
| completion(purchasedTransaction, | |||
| result.value, | |||
There was a problem hiding this comment.
There's a possibility that this ends up with nil customer info and nil error, leading to a crash: https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/13951/workflows/3b380d52-bc83-4449-aa91-cf6b55235ec8/jobs/103818
Thread 0 Crashed:
0 libswiftCore.dylib 0x1021b1e81 _assertionFailure(_:_:file:line:flags:) + 353
1 RevenueCat 0x1310b2dc7 Result.init(_:_:file:line:) + 1047 (Result+Extensions.swift:24)
2 RevenueCat 0x130f35cf9 closure #1 in closure #1 in Purchases.purchaseAsync(product:promotionalOffer:) + 361 (Purchases+async.swift:97)
3 RevenueCat 0x1310dcd9f closure #2 in PurchasesOrchestrator.purchase(sk1Product:payment:package:wrapper:completion:) + 847 (PurchasesOrchestrator.swift:395)
4 RevenueCat 0x1310ec8dc partial apply for closure #2 in PurchasesOrchestrator.purchase(sk1Product:payment:package:wrapper:completion:) + 44
5 RevenueCat 0x1310e4ce9 thunk for @escaping @callee_guaranteed @Sendable (@guaranteed StoreTransaction?, @guaranteed CustomerInfo?, @guaranteed NSError?, @unowned Bool) -> () + 25
6 RevenueCat 0x1310e8806 thunk for @escaping @callee_guaranteed @Sendable (@in_guaranteed StoreTransaction?, @in_guaranteed CustomerInfo?, @in_guaranteed NSError?, @in_guaranteed Bool) -> (@out ()) + 134
7 RevenueCat 0x1310e4ce9 thunk for @escaping @callee_guaranteed @Sendable (@guaranteed StoreTransaction?, @guaranteed CustomerInfo?, @guaranteed NSError?, @unowned Bool) -> () + 25
8 RevenueCat 0x1310e8806 thunk for @escaping @callee_guaranteed @Sendable (@in_guaranteed StoreTransaction?, @in_guaranteed CustomerInfo?, @in_guaranteed NSError?, @in_guaranteed Bool) -> (@out ()) + 134
9 RevenueCat 0x1310eb387 closure #1 in closure #1 in PurchasesOrchestrator.handlePurchasedTransaction(_:storefront:restored:) + 759 (PurchasesOrchestrator.swift:1092)
10 RevenueCat 0x130edc340 closure #1 in static OperationDispatcher.dispatchOnMainActor(_:) + 80 (OperationDispatcher.swift:60)
11 RevenueCat 0x130edc5d1 partial apply for closure #1 in static OperationDispatcher.dispatchOnMainActor(_:) + 1
12 RevenueCat 0x130edc3e1 thunk for @escaping @callee_guaranteed @Sendable @async () -> () + 1
13 RevenueCat 0x130edc6e1 partial apply for thunk for @escaping @callee_guaranteed @Sendable @async () -> () + 1
14 RevenueCat 0x130e6bbe1 thunk for @escaping @callee_guaranteed @Sendable @async () -> (@out A) + 1
15 RevenueCat 0x130e6bd11 partial apply for thunk for @escaping @callee_guaranteed @Sendable @async () -> (@out A) + 1
Need to write a unit test for this and fix it.
There was a problem hiding this comment.
Fixed and tested.
NachoSoto
force-pushed
the
customer-info-post-transactions-dedup
branch
from
e214dfe
to
75f5734
Compare
NachoSoto
force-pushed
the
cache-posted-transactions-2
branch
from
f0ef51f
to
7f5b182
Compare
See #2914 (comment) This will be used by that PR as a fallback whenever we try to post a transaction that had already been posted.
NachoSoto
force-pushed
the
cache-posted-transactions-2
branch
from
7f5b182
to
df76073
Compare
| } | ||
| } | ||
|
|
||
| self.handleReceiptPost( |
There was a problem hiding this comment.
This uses handleReceiptPost which simplifies handling .alreadyPosted.
| case success(CustomerInfo) | ||
|
|
||
| /// Transaction had already been posted. | ||
| case alreadyPosted |
There was a problem hiding this comment.
New type helps ensure that we handle this scenario everywhere that used Result<CustomerInfo, BackendError>.
| for: product, | ||
| customerInfo: customerInfo | ||
| ) { | ||
| self.finishTransactionIfNeeded(transaction) { |
There was a problem hiding this comment.
A subtle change here, suggested by @aboedo: we no longer call finishTransactionIfNeeded from the main actor, we only call the completion (see complete) above block after we're done with everything.
| expect(parameters.data.source.initiationSource) == .queue | ||
| } | ||
|
|
||
| func testPostsFirstUnpostedTransaction() async throws { |
There was a problem hiding this comment.
Updated this test to have some previously posted transactions too.
| let payment = SKPayment(product: self.product) | ||
|
|
||
| let customerInfoBeforePurchase = try CustomerInfo(data: [ | ||
| self.customerInfoBeforePurchase = try CustomerInfo(data: [ |
There was a problem hiding this comment.
This is extracting these 2 CustomerInfos to avoid duplicate code.
| try self.delegate.storeKit1Wrapper(self.storeKit1Wrapper, updatedTransaction: transaction1) | ||
| try self.delegate.storeKit1Wrapper(self.storeKit1Wrapper, updatedTransaction: transaction2) |
There was a problem hiding this comment.
This test no longer had the same semantics because it used the same transaction, which was a shortcut. I made this reflect a more normal flow, and added another one below that uses the same transaction, for which the second one we don't post
| expect(result.error?.finishable) == false | ||
|
|
||
| expect(self.cache.postedTransactions).to(beEmpty()) |
There was a problem hiding this comment.
testHandlePurchasedTransactionWithMissingReceipt is basically testing a not-finishable error. In this case we don't save the posted transaction.
| expect(result.error?.finishable) == true | ||
|
|
||
| self.verifyTransactionWasCached() |
There was a problem hiding this comment.
For finishable errors we do.
| @@ -64,13 +84,57 @@ class TransactionPosterTests: TestCase { | |||
| self.backend.stubbedPostReceiptResult = .success(Self.mockCustomerInfo) | |||
|
|
|||
| let result = try self.handleTransaction(transactionData) | |||
| expect(result).to(beSuccess()) | |||
There was a problem hiding this comment.
This isn't a Result anymore, so can't use beSuccess(). But checking .value is equivalent.
| for _ in 0..<count { | ||
| _ = try self.handleTransaction(transactionData) | ||
| } | ||
|
|
||
| expect(self.backend.invokedPostReceiptDataCount) == 1 | ||
| expect(self.mockTransaction.finishInvokedCount) == count |
There was a problem hiding this comment.
Calling it 10 times to ensure that we only post it once (and finish 10 times).
|
|
||
| expect(self.backend.invokedPostReceiptData) == true | ||
| expect(self.backend.invokedPostReceiptDataParameters?.observerMode) == true | ||
| expect(self.mockTransaction.finishInvoked) == false | ||
|
|
||
| self.verifyTransactionWasCached() |
There was a problem hiding this comment.
Added these to all tests that are relevant.
| self.serverDown() | ||
| try await self.purchaseMonthlyProduct() | ||
|
|
||
| self.logger.verifyMessageWasLogged(Strings.offlineEntitlements.computing_offline_customer_info, level: .info) | ||
| self.verifyNoTransactionsWereFinished() | ||
| self.verifyNoTransactionsWereStored() |
There was a problem hiding this comment.
I actually broke this (thankfully that broke other tests), so now I'm covering it with a test.
| self.verifyTransactionWasFinished(count: 1) | ||
| self.verifyTransactionWasStored(count: 1) |
There was a problem hiding this comment.
Improved the basic "can purchase package" test with these 2 assertions.
| @@ -361,6 +374,21 @@ private extension CustomerInfoManager { | |||
| } | |||
| } | |||
|
|
|||
| private func cachedOrRequestCustomerInfoFetcher( | |||
There was a problem hiding this comment.
Hmm this seems unused now after the latest changes?
|
|
||
| /// Converts the `TransactionPosterResult` into `Result<CustomerInfo, BackendError>` | ||
| /// by using `customerInfoFetcher` if the result is `.alreadyPosted`. | ||
| func toResult( |
There was a problem hiding this comment.
The comment is clear but I'm concerned the name of the function doesn't indicate that this might initiate a fetch of the customer info... Maybe we could rename to toResultOrFetchCustomerInfo? Or something along those lines?
There was a problem hiding this comment.
Swift (and Obj-C) are different from other languages, the name of this method isn't just toResult, it's toResult(completion:customerInfoFetcher:), so the "fetch customer info" part is already part of the name.
It would have to toResultOrFetchCustomerInfo(completion:_:) which is less clear IMO.
| // MARK: - | ||
|
|
||
| private var storedTransactions: StoredTransactions { | ||
| return self.deviceCache.value(for: CacheKey.transactions) ?? [] |
There was a problem hiding this comment.
Kinda unrelated to this PR but I was wondering, do we clear the cache values at any point? In android we clear them when we have a token but not a corresponding active purchase anymore (so the purchase has expired). We perform this check on every app foreground.
There was a problem hiding this comment.
That's a fair question. There's no direct equivalent on iOS, in that a purchase never goes away from the receipt (other than in Sandbox).
We could give them a TTL based on expiration date, with the assumption that if a purchase has been posted and finished, if say, 90 days go by, you're probably not gonna find it in the queue again and should be safe to remove.
It's not perfectly true in that calling restoreCompletedTransactions could technically bring it back, but should work for the vast majority of cases. Thoughts?
There was a problem hiding this comment.
Done in #3023 and rebased this.
This was suggested in #2914 as a way to prevent the cache from growing forever. ### Other changes: - Added `PostedTransactionCache.unpostedTransactions`: this was added in #2914, so I put it here to prevent merge conflicts. - `PostedTransactionCache` now verifies that cache interaction doesn't happen in the main thread - Added debug log when saving transactions - Added debug log when pruning cache - Added `ClockType` tests
This is a performance optimization that will help avoid duplicate posts after #2612.
NachoSoto
force-pushed
the
cache-posted-transactions-2
branch
from
443f5bd
to
7abf1a5
Compare
| self.handlePostReceiptResult(result, | ||
| subscriberAttributes: unsyncedAttributes, | ||
| adServicesToken: adServicesToken) | ||
| self.operationDispatcher.dispatchOnWorkerThread { |
There was a problem hiding this comment.
Thanks to the new assertions we're avoiding this now.
|
Closing this. As shown in #3020 this won't work with that 😢 |
As explained in #2914 this won't be usable.
This is a performance optimization that will help avoid duplicate posts after #2612.
TODO: