Skip to content
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

TransactionPoster: avoid posting transactions multiple times #2914

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions RevenueCat.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,8 @@
4FC083292A4A35FB00A97089 /* Integer+Extensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4FC083282A4A35FB00A97089 /* Integer+Extensions.swift */; };
4FC0832B2A4A361700A97089 /* IntegerExtensionsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4FC0832A2A4A361700A97089 /* IntegerExtensionsTests.swift */; };
4FC6F8892A73E445002139B2 /* PostedTransactionCacheTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4FC6F8882A73E445002139B2 /* PostedTransactionCacheTests.swift */; };
4FC6F88B2A73FBD9002139B2 /* MockPostedTransactionCache.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4FC6F88A2A73FBD9002139B2 /* MockPostedTransactionCache.swift */; };
4FC6F88C2A73FBD9002139B2 /* MockPostedTransactionCache.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4FC6F88A2A73FBD9002139B2 /* MockPostedTransactionCache.swift */; };
4FC8EE522A8D34D70010EDFF /* ClockTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4FC8EE512A8D34D70010EDFF /* ClockTests.swift */; };
4FC972172A712DCC008593DE /* CachingProductsManagerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4FC972162A712DCC008593DE /* CachingProductsManagerTests.swift */; };
4FCBA84F2A15391B004134BD /* SnapshotTesting+Extensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = 576C8A9127D27DDD0058FA6E /* SnapshotTesting+Extensions.swift */; };
Expand Down Expand Up @@ -1002,6 +1004,7 @@
4FC083282A4A35FB00A97089 /* Integer+Extensions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Integer+Extensions.swift"; sourceTree = "<group>"; };
4FC0832A2A4A361700A97089 /* IntegerExtensionsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = IntegerExtensionsTests.swift; sourceTree = "<group>"; };
4FC6F8882A73E445002139B2 /* PostedTransactionCacheTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PostedTransactionCacheTests.swift; sourceTree = "<group>"; };
4FC6F88A2A73FBD9002139B2 /* MockPostedTransactionCache.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockPostedTransactionCache.swift; sourceTree = "<group>"; };
4FC8EE512A8D34D70010EDFF /* ClockTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ClockTests.swift; sourceTree = "<group>"; };
4FC972162A712DCC008593DE /* CachingProductsManagerTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CachingProductsManagerTests.swift; sourceTree = "<group>"; };
4FCBA8522A1539D0004134BD /* __Snapshots__ */ = {isa = PBXFileReference; lastKnownFileType = folder; path = __Snapshots__; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1851,6 +1854,7 @@
5791FBD1299184EF00F1FEDA /* MockAsyncSequence.swift */,
57CB2A7B29CCC91800C91439 /* MockProductEntitlementMappingFetcher.swift */,
4F8A58162A16EE3500EF97AD /* MockOfflineCustomerInfoCreator.swift */,
4FC6F88A2A73FBD9002139B2 /* MockPostedTransactionCache.swift */,
);
path = Mocks;
sourceTree = "<group>";
Expand Down Expand Up @@ -3182,6 +3186,7 @@
571E7AD4279F2D0C003B3606 /* StoreKitTestHelpers.swift in Sources */,
2DFF6C56270CA28800ECAFAB /* MockRequestFetcher.swift in Sources */,
5738F489278CC2500096D623 /* MockTransaction.swift in Sources */,
4FC6F88C2A73FBD9002139B2 /* MockPostedTransactionCache.swift in Sources */,
576C8ABC27D2997C0058FA6E /* SnapshotTesting+Extensions.swift in Sources */,
F5355164286B7125009CA47A /* MockOfferingsFactory.swift in Sources */,
57CB2B6029CDF63200C91439 /* PurchasedProductsFetcherTests.swift in Sources */,
Expand Down Expand Up @@ -3504,6 +3509,7 @@
57554C84282AC273009A7E58 /* PeriodTypeTests.swift in Sources */,
57057FF828B0048900995F21 /* TestLogHandler.swift in Sources */,
57E415F12846997B00EA5460 /* PurchasesAttributionDataTests.swift in Sources */,
4FC6F88B2A73FBD9002139B2 /* MockPostedTransactionCache.swift in Sources */,
2DDF41CF24F6F4C3005BC22D /* ReceiptParsing+TestsWithRealReceipts.swift in Sources */,
57488C2329CB89CC0000EE7E /* OfflineEntitlementsManagerTests.swift in Sources */,
57FDAA962846BDE2009A48F1 /* PurchasesTransactionHandlingTests.swift in Sources */,
Expand Down
23 changes: 18 additions & 5 deletions Sources/Identity/CustomerInfoManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -320,10 +320,14 @@ private extension CustomerInfoManager {
if #available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *) {
_ = Task<Void, Never> {
let transactions = await self.transactionFetcher.unfinishedVerifiedTransactions
let unpostedTransactions = self.transactionPoster.unpostedTransactions(in: transactions)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a follow-up to #2954. We look for the first un-posted transaction now.


if let transactionToPost = transactions.first {
if let transactionToPost = unpostedTransactions.first {
Logger.debug(
Strings.customerInfo.posting_transactions_in_lieu_of_fetching_customerinfo(transactions)
Strings.customerInfo.posting_transactions_in_lieu_of_fetching_customerinfo(
allTransactions: transactions,
unpostedTransactions: unpostedTransactions
)
)

let transactionData = PurchasedTransactionData(
Expand All @@ -336,18 +340,27 @@ private extension CustomerInfoManager {

// Post everything but the first transaction in the background
// in parallel so they can be de-duped
let otherTransactionsToPostInParalel = Array(transactions.dropFirst())
let otherTransactionsToPostInParalel = Array(unpostedTransactions.dropFirst())
Task.detached(priority: .background) {
await self.postTransactions(otherTransactionsToPostInParalel, transactionData)
}

// Return the result of posting the first transaction.
// The posted receipt will include the content of every other transaction
// so we don't need to wait for those.
completion(await self.transactionPoster.handlePurchasedTransaction(
let result = await self.transactionPoster.handlePurchasedTransaction(
transactionToPost,
data: transactionData
))
)

result.toResult(completion: completion) {
// There is a chance due to race-conditions that even though it was initially determined that
// `transactionToPost` had not been posted,
// `handlePurchasedTransaction` returns `.alreadyPosted`. This falls back to
self.requestCustomerInfo(appUserID: appUserID,
isAppBackgrounded: isAppBackgrounded,
completion: $0)
}
} else {
self.requestCustomerInfo(appUserID: appUserID,
isAppBackgrounded: isAppBackgrounded,
Expand Down
9 changes: 5 additions & 4 deletions Sources/Logging/Strings/CustomerInfoStrings.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ enum CustomerInfoStrings {
case customerinfo_updated_from_network
case customerinfo_updated_from_network_error(BackendError)
case customerinfo_updated_offline
case posting_transactions_in_lieu_of_fetching_customerinfo([StoreTransaction])
case posting_transactions_in_lieu_of_fetching_customerinfo(allTransactions: [StoreTransaction],
unpostedTransactions: [StoreTransaction])
case updating_request_date(CustomerInfo, Date)
case sending_latest_customerinfo_to_delegate
case sending_updated_customerinfo_to_delegate
Expand Down Expand Up @@ -73,9 +74,9 @@ extension CustomerInfoStrings: LogMessage {
case .customerinfo_updated_offline:
return "There was an error communicating with RevenueCat servers. " +
"CustomerInfo was temporarily computed offline, and it will be posted again as soon as possible."
case let .posting_transactions_in_lieu_of_fetching_customerinfo(transactions):
return "Found \(transactions.count) unfinished transactions, will post receipt in lieu " +
"of fetching CustomerInfo:\n\(transactions)"
case let .posting_transactions_in_lieu_of_fetching_customerinfo(allTransactions, unpostedTransactions):
return "Found \(allTransactions.count) unfinished transactions (\(unpostedTransactions.count) unposted)," +
" will post receipt in lieu of fetching CustomerInfo:\n\(allTransactions)"
case let .updating_request_date(info, newRequestDate):
return "Updating CustomerInfo '\(info.originalAppUserId)' request date: \(newRequestDate)"
case .sending_latest_customerinfo_to_delegate:
Expand Down
8 changes: 8 additions & 0 deletions Sources/Logging/Strings/PurchaseStrings.swift
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ enum PurchaseStrings {
case missing_cached_customer_info
case sk2_transactions_update_received_transaction(productID: String)
case transaction_poster_handling_transaction(productID: String, offeringID: String?)
case transaction_poster_skipping_duplicate(productID: String, transactionID: String)
case transaction_poster_storing_posted_transaction(productID: String, transactionID: String)
case caching_presented_offering_identifier(offeringID: String, productID: String)
case payment_queue_wrapper_delegate_call_sk1_enabled
case restorepurchases_called_with_allow_sharing_appstore_account_false
Expand Down Expand Up @@ -316,6 +318,12 @@ extension PurchaseStrings: LogMessage {
return prefix
}

case let .transaction_poster_skipping_duplicate(productID, transactionID):
return "TransactionPoster: skipping already posted transaction for product '\(productID)': \(transactionID)"

case let .transaction_poster_storing_posted_transaction(productID, transactionID):
return "TransactionPoster: caching posted transaction for product '\(productID)': \(transactionID)"

case let .caching_presented_offering_identifier(offeringID, productID):
return "Caching presented offering identifier '\(offeringID)' for product '\(productID)'"

Expand Down
1 change: 1 addition & 0 deletions Sources/Purchasing/Purchases/Purchases.swift
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ public typealias StartPurchaseBlock = (@escaping PurchaseCompletedBlock) -> Void
productsManager: productsManager,
receiptFetcher: receiptFetcher,
backend: backend,
cache: PostedTransactionCache(deviceCache: deviceCache, clock: systemInfo.clock),
paymentQueueWrapper: paymentQueueWrapper,
systemInfo: systemInfo,
operationDispatcher: operationDispatcher
Expand Down
120 changes: 83 additions & 37 deletions Sources/Purchasing/Purchases/PurchasesOrchestrator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ extension PurchasesOrchestrator: StoreKit2TransactionListenerDelegate {
let subscriberAttributes = self.unsyncedAttributes
let adServicesToken = self.attribution.unsyncedAdServicesToken

let result: Result<CustomerInfo, BackendError> = await Async.call { completed in
let result: TransactionPosterResult = await Async.call { completed in
self.transactionPoster.handlePurchasedTransaction(
StoreTransaction.from(transaction: transaction),
data: .init(
Expand Down Expand Up @@ -1058,7 +1058,7 @@ private extension PurchasesOrchestrator {
source: .init(isRestore: isRestore, initiationSource: initiationSource)
),
observerMode: self.observerMode) { result in
self.handleReceiptPost(result: result,
self.handleReceiptPost(result: .init(result),
subscriberAttributes: unsyncedAttributes,
adServicesToken: adServicesToken,
completion: completion)
Expand All @@ -1068,7 +1068,7 @@ private extension PurchasesOrchestrator {
}
}

func handleReceiptPost(result: Result<CustomerInfo, BackendError>,
func handleReceiptPost(result: TransactionPosterResult,
subscriberAttributes: SubscriberAttribute.Dictionary,
adServicesToken: String?,
completion: (@Sendable (Result<CustomerInfo, PurchasesError>) -> Void)?) {
Expand All @@ -1080,21 +1080,33 @@ private extension PurchasesOrchestrator {

if let completion = completion {
self.operationDispatcher.dispatchOnMainThread {
completion(result.mapError { $0.asPurchasesError })
self.convertToResult(result) { result in
completion(result.mapError { $0.asPurchasesError })
}
}
}
}

func handlePostReceiptResult(_ result: Result<CustomerInfo, BackendError>,
func handlePostReceiptResult(_ result: TransactionPosterResult,
subscriberAttributes: SubscriberAttribute.Dictionary,
adServicesToken: String?) {
if let customerInfo = result.value {
switch result {
case .alreadyPosted:
// Nothing to do
break

case let .success(customerInfo):
self.customerInfoManager.cache(customerInfo: customerInfo, appUserID: self.appUserID)
}

self.markSyncedIfNeeded(subscriberAttributes: subscriberAttributes,
adServicesToken: adServicesToken,
error: result.error)
self.markSyncedIfNeeded(subscriberAttributes: subscriberAttributes,
adServicesToken: adServicesToken,
error: nil)

case let .failure(error):
self.markSyncedIfNeeded(subscriberAttributes: subscriberAttributes,
adServicesToken: adServicesToken,
error: error)
}
}

func handlePurchasedTransaction(_ purchasedTransaction: StoreTransaction,
Expand All @@ -1104,31 +1116,41 @@ private extension PurchasesOrchestrator {
let unsyncedAttributes = self.unsyncedAttributes
let adServicesToken = self.attribution.unsyncedAdServicesToken

self.transactionPoster.handlePurchasedTransaction(
purchasedTransaction,
data: .init(
appUserID: self.appUserID,
presentedOfferingID: offeringID,
unsyncedAttributes: unsyncedAttributes,
aadAttributionToken: adServicesToken,
storefront: storefront,
source: self.purchaseSource(for: purchasedTransaction.productIdentifier,
transaction: purchasedTransaction,
restored: restored)
)
) { result in
self.handlePostReceiptResult(result,
subscriberAttributes: unsyncedAttributes,
adServicesToken: adServicesToken)
self.operationDispatcher.dispatchOnWorkerThread {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Another change here. We were handling transactions in the main thread without this, which now made us read/write cache in the main thread.
Thanks to the new assertions we're avoiding this now.

self.transactionPoster.handlePurchasedTransaction(
purchasedTransaction,
data: .init(
appUserID: self.appUserID,
presentedOfferingID: offeringID,
unsyncedAttributes: unsyncedAttributes,
aadAttributionToken: adServicesToken,
storefront: storefront,
source: self.purchaseSource(for: purchasedTransaction.productIdentifier,
transaction: purchasedTransaction,
restored: restored)
)
) { result in
let completion: (@Sendable(Result<CustomerInfo, PurchasesError>) -> Void)? =
self.getAndRemovePurchaseCompletedCallback(forTransaction: purchasedTransaction)
.map { completion in
return { @Sendable result in
self.operationDispatcher.dispatchOnMainActor {
completion(
purchasedTransaction,
result.value,
result.error?.asPublicError,
result.error?.isCancelledError ?? false
)
}
}
}

if let completion = self.getAndRemovePurchaseCompletedCallback(forTransaction: purchasedTransaction) {
self.operationDispatcher.dispatchOnMainActor {
completion(purchasedTransaction,
result.value,
Copy link
Contributor Author

@NachoSoto NachoSoto Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@NachoSoto NachoSoto Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and tested.

result.error?.asPublicError,
result.error?.isCancelledError ?? false
)
}
self.handleReceiptPost(
result: result,
subscriberAttributes: unsyncedAttributes,
adServicesToken: adServicesToken,
completion: completion
)
}
}
}
Expand Down Expand Up @@ -1200,6 +1222,19 @@ private extension PurchasesOrchestrator {
}
}

func convertToResult(
_ transactionResult: TransactionPosterResult,
completion: @escaping @Sendable (Result<CustomerInfo, BackendError>) -> Void
) {
transactionResult.toResult(completion: completion) {
self.customerInfoManager.customerInfo(
appUserID: self.appUserID,
fetchPolicy: .cachedOrFetched,
completion: $0
)
}
}

}

private extension PurchasesOrchestrator {
Expand Down Expand Up @@ -1235,7 +1270,7 @@ extension PurchasesOrchestrator {
let unsyncedAttributes = self.unsyncedAttributes
let adServicesToken = self.attribution.unsyncedAdServicesToken

let result = await self.transactionPoster.handlePurchasedTransaction(
let postResult = await self.transactionPoster.handlePurchasedTransaction(
transaction,
data: .init(
appUserID: self.appUserID,
Expand All @@ -1248,11 +1283,12 @@ extension PurchasesOrchestrator {
)
)

self.handlePostReceiptResult(result,
self.handlePostReceiptResult(postResult,
subscriberAttributes: unsyncedAttributes,
adServicesToken: adServicesToken)

return try result
return try await self
.convertToResult(postResult)
.mapError(\.asPurchasesError)
.get()
}
Expand All @@ -1268,4 +1304,14 @@ extension PurchasesOrchestrator {
}
}

/// Converts a `TransactionPosterResult` to a `Result<CustomerInfo, BackendError>`
/// by optionally loading a customer info for `TransactionPosterResult.alreadyPosted`.
private func convertToResult(
_ transactionResult: TransactionPosterResult
) async -> Result<CustomerInfo, BackendError> {
return await Async.call { completion in
self.convertToResult(transactionResult, completion: completion)
}
}

}
Loading