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

CustomerInfoManager: post transactions in parallel to POST receipts only once #2954

Merged
merged 9 commits into from
Aug 11, 2023

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Aug 2, 2023

The existing implementation was posting them in sequence, which meant that it was always going to make N requests.
With this new implementation we can ensure that only one request is made for all transactions.

@NachoSoto NachoSoto added the perf label Aug 2, 2023
@NachoSoto NachoSoto requested a review from a team August 2, 2023 21:50
description: "Subscription never became active"
)
pollInterval: .milliseconds(200)
) { [delegate = self.purchasesDelegate] in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this a trailing closure which is simpler to use.

@@ -22,6 +22,10 @@ extension OperationQueue {
switch cacheStatus {
case .firstCallbackAddedToList:
self.addOperation(factory.create())

Logger.verbose(Strings.network.enqueing_operation(factory.operationType,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Equivalent log to the one for addedToExistingInFlightList, useful for debugging.

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.70%. Comparing base (161b04d) to head (9256434).
Report is 847 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2954   +/-   ##
=======================================
  Coverage   86.69%   86.70%           
=======================================
  Files         219      219           
  Lines       15619    15636   +17     
=======================================
+ Hits        13541    13557   +16     
- Misses       2078     2079    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NachoSoto NachoSoto force-pushed the customer-info-post-transactions-dedup branch from 39b2f8f to 8d1d751 Compare August 3, 2023 21:46
Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

This LGTM!

@NachoSoto NachoSoto force-pushed the customer-info-post-transactions-dedup branch from e214dfe to 75f5734 Compare August 11, 2023 20:46
Sources/Identity/CustomerInfoManager.swift Show resolved Hide resolved
Comment on lines +383 to +386
await withTaskGroup(of: Void.self) { group in
for transaction in transactions {
group.addTask {
_ = await self.transactionPoster.handlePurchasedTransaction(
Copy link
Member

Choose a reason for hiding this comment

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

should we have a log for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TransactionPoster already has a log in handlePurchasedTransaction.

@NachoSoto NachoSoto enabled auto-merge (squash) August 11, 2023 22:06
@NachoSoto NachoSoto merged commit 581a048 into main Aug 11, 2023
@NachoSoto NachoSoto deleted the customer-info-post-transactions-dedup branch August 11, 2023 22:39
NachoSoto added a commit that referenced this pull request Aug 14, 2023
…p launch

Possibly as a consequence of #2623 we were calling `updateAllCachesIfNeeded` on app launch together with `updateCachesIfInForeground`, meaning that we were updating all caches twice.
Thanks to our de-duping logic this is normally not a big issue, but this issue combined with what #2954 fixes, means that if there are pending transactions, opening the app would probably end up posting receipts duplicated times.

This adds a new log as well to detect this:
> [purchases] DEBUG: ℹ️ Throttling cache update, only 0.8 seconds elapsed

And a new integration test that posts `UIApplication.willEnterForegroundNotification` to test this.
This was referenced Aug 16, 2023
NachoSoto pushed a commit that referenced this pull request Aug 17, 2023
**This is an automatic release.**

### Bugfixes
* `PurchasesOrchestrator`: fixed callback not invoked regression during
downgrades (#3028) via NachoSoto (@NachoSoto)
* `TransactionPoster`: don't finish transactions for non-subscriptions
if they're not processed (#2841) via NachoSoto (@NachoSoto)
### Performance Improvements
* `StoreKit 2`: only listen to `StoreKit.Transaction.updates` when SK2
is enabled (#3032) via NachoSoto (@NachoSoto)
* `CustomerInfoManager`: post transactions in parallel to POST receipts
only once (#2954) via NachoSoto (@NachoSoto)
### Other Changes
* `PostedTransactionCache`: remove implementation (#3030) via NachoSoto
(@NachoSoto)
* `Integration Tests`: improved `testCanPurchaseMultipleSubscriptions`
(#3025) via NachoSoto (@NachoSoto)
* `GitHub`: improved `ISSUE_TEMPLATE` (#3022) via NachoSoto (@NachoSoto)
* `TransactionPoster`: added transaction ID and Date to log (#3026) via
NachoSoto (@NachoSoto)
* `TransactionPoster`: fix iOS 12 test (#3018) via NachoSoto
(@NachoSoto)
* `SystemInfo`: added `ClockType` (#3014) via NachoSoto (@NachoSoto)
* `Integration Tests`: begin tests with
`UIApplication.willEnterForegroundNotification` to simulate a real app
(#3015) via NachoSoto (@NachoSoto)
* `Integration Tests`: add tests to verify `CustomerInfo`+`Offerings`
request de-dupping (#3013) via NachoSoto (@NachoSoto)
* `SwiftLint`: disable `unneeded_synthesized_initializer` (#3010) via
NachoSoto (@NachoSoto)
* Added `internal`
`NonSubscriptionTransaction.storeTransactionIdentifier` (#3009) via
NachoSoto (@NachoSoto)
* `Integration Tests`: added tests for non-renewing and non-consumable
packages (#3008) via NachoSoto (@NachoSoto)
* Expanded `EnsureNonEmptyArrayDecodable` to
`EnsureNonEmptyCollectionDecodable` (#3002) via NachoSoto (@NachoSoto)
MarkVillacampa pushed a commit that referenced this pull request Sep 6, 2023
… only once (#2954)

The existing implementation was posting them in sequence, which meant
that it was always going to make N requests.
With this new implementation we can ensure that only one request is made
for all transactions.
MarkVillacampa pushed a commit that referenced this pull request Sep 6, 2023
**This is an automatic release.**

### Bugfixes
* `PurchasesOrchestrator`: fixed callback not invoked regression during
downgrades (#3028) via NachoSoto (@NachoSoto)
* `TransactionPoster`: don't finish transactions for non-subscriptions
if they're not processed (#2841) via NachoSoto (@NachoSoto)
### Performance Improvements
* `StoreKit 2`: only listen to `StoreKit.Transaction.updates` when SK2
is enabled (#3032) via NachoSoto (@NachoSoto)
* `CustomerInfoManager`: post transactions in parallel to POST receipts
only once (#2954) via NachoSoto (@NachoSoto)
### Other Changes
* `PostedTransactionCache`: remove implementation (#3030) via NachoSoto
(@NachoSoto)
* `Integration Tests`: improved `testCanPurchaseMultipleSubscriptions`
(#3025) via NachoSoto (@NachoSoto)
* `GitHub`: improved `ISSUE_TEMPLATE` (#3022) via NachoSoto (@NachoSoto)
* `TransactionPoster`: added transaction ID and Date to log (#3026) via
NachoSoto (@NachoSoto)
* `TransactionPoster`: fix iOS 12 test (#3018) via NachoSoto
(@NachoSoto)
* `SystemInfo`: added `ClockType` (#3014) via NachoSoto (@NachoSoto)
* `Integration Tests`: begin tests with
`UIApplication.willEnterForegroundNotification` to simulate a real app
(#3015) via NachoSoto (@NachoSoto)
* `Integration Tests`: add tests to verify `CustomerInfo`+`Offerings`
request de-dupping (#3013) via NachoSoto (@NachoSoto)
* `SwiftLint`: disable `unneeded_synthesized_initializer` (#3010) via
NachoSoto (@NachoSoto)
* Added `internal`
`NonSubscriptionTransaction.storeTransactionIdentifier` (#3009) via
NachoSoto (@NachoSoto)
* `Integration Tests`: added tests for non-renewing and non-consumable
packages (#3008) via NachoSoto (@NachoSoto)
* Expanded `EnsureNonEmptyArrayDecodable` to
`EnsureNonEmptyCollectionDecodable` (#3002) via NachoSoto (@NachoSoto)
@vegaro vegaro added pr:other and removed pr:perf labels Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants