fix mac os sandbox check slowness #3879
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
tonidero
reviewed
May 9, 2024
| @@ -73,12 +80,12 @@ extension BundleSandboxEnvironmentDetector: Sendable {} | |||
|
|
|||
| private extension BundleSandboxEnvironmentDetector { | |||
|
|
|||
| var isProductionReceipt: Bool { | |||
| var isProductionReceipt: Bool? { | |||
| do { | |||
| return try self.receiptFetcher.fetchAndParseLocalReceipt().environment == .production | |||
There was a problem hiding this comment.
Should we also check if the environment is .unknown? Seems that the catch will happen with any parsing error, not specifically about the environment so I wonder if we will be doing extra operations in those cases... Having said that, parsing errors should be fairly uncommon, so I think it should be ok?
There was a problem hiding this comment.
Only 3 months late to respond to the comment 😅
Good call, I'll add an extra check for that
There was a problem hiding this comment.
can't imagine it would happen often that we can correctly parse the receipt and it's production but it doesn't have the environment variable, but I can't discard it either since it's an undocumented field
MarkVillacampa
approved these changes
May 10, 2024
… we can't verify the environment from the receipt itself
aboedo
force-pushed
the
origin/andy/fix_macOS_sandbox_check_slowness_2
branch
from
e1df30d to
31a8ba4
Compare
aboedo
changed the title
aboedo
commented
Aug 22, 2024
| func testIsNotSandboxIfReceiptIsProductionAndMAS() throws { | ||
| func testIsNotSandboxIfReceiptIsProduction() throws { |
There was a problem hiding this comment.
we no longer look at MAS if we know what the receipt environment is
Comment on lines
-66
to
-72
| func testIsSandboxIfReceiptIsNotProductionAndNotMAS() throws { | ||
| expect( | ||
| SystemInfo.with( | ||
| macAppStore: false, | ||
| receiptEnvironment: .sandbox | ||
| ).isSandbox | ||
| ) == true |
There was a problem hiding this comment.
this test and the next one are technically still there, but now in a more complete test that also verifies that we indeed use MAS when needed and skip it when not needed
Comment on lines
-154
to
+211
| private struct MockMacAppStoreDetector: MacAppStoreDetector { | ||
| private final class MockMacAppStoreDetector: MacAppStoreDetector, @unchecked Sendable { | ||
|
|
||
| let isMacAppStore: Bool | ||
| let isMacAppStoreValue: Bool | ||
| private(set) var isMacAppStoreCalled = false | ||
|
|
||
| init(isMacAppStore: Bool) { | ||
| self.isMacAppStore = isMacAppStore | ||
| self.isMacAppStoreValue = isMacAppStore | ||
| } | ||
|
|
||
| var isMacAppStore: Bool { | ||
| isMacAppStoreCalled = true | ||
| return isMacAppStoreValue | ||
| } |
There was a problem hiding this comment.
changes here are to make it possible to check whether it was called
|
@tonidero @MarkVillacampa ready for another pass, hopefully it takes me less than 3 months this time to iterate on PR comments this time |
|
Tested on device (by forcing the environment value in the receipt) and was able to verify that I got the warning before the changes and did not get it after. Still could be optimized further by caching the value (it won't change in a single app run) but then I started fighting Sendable and figured to live to fight another day |
MarkVillacampa
approved these changes
Aug 23, 2024
|
Looking good! |
This was referenced Aug 28, 2024
vegaro
pushed a commit
that referenced
this pull request
Aug 30, 2024
**This is an automatic release.** ### Bugfixes * Fix `compatibleTopBarTrailing` in MacOS and api tests (#4226) via Cesar de la Vega (@vegaro) * [Paywall] Fix restoreStarted not being called on `presentPaywallIfNeeded` when using `requiredEntitlementIdentifier` (#4223) via Josh Holtz (@joshdholtz) * [CustomerCenter] Move sheet and restore alert creation to `ManageSubscriptionsView` (#4220) via Cesar de la Vega (@vegaro) * [EXTERNAL] `Custom Entitlements Computation`: fix support display on debug screen (#4215) by @NachoSoto (#4218) via Toni Rico (@tonidero) * [Customer Center] Add padding to `No thanks` in promotional offer screen (#4221) via Cesar de la Vega (@vegaro) * Fix version number in plist files (#4213) via Cesar de la Vega (@vegaro) * fix mac os sandbox check slowness (#3879) via Andy Boedo (@aboedo) * [Customer Center] Fix `FeedbackSurveyView` not opening (#4208) via Cesar de la Vega (@vegaro) * Remove `unneeded_override` disable to fix linter (#4209) via Cesar de la Vega (@vegaro) ### Dependency Updates * Bump rexml from 3.3.3 to 3.3.6 in /Tests/InstallationTests/CocoapodsInstallation (#4210) via dependabot[bot] (@dependabot[bot]) * Bump rexml from 3.3.3 to 3.3.6 (#4211) via dependabot[bot] (@dependabot[bot]) ### Other Changes * Update readme wording (#3914) via James Borthwick (@jamesrb1) * Set a maximum duration for iOS 15 tests (#4229) via Cesar de la Vega (@vegaro) * Paywall Components Initial Commit (#4224) via James Borthwick (@jamesrb1) * [CustomerCenter] Open App Store when the user wants to update their app (#4199) via JayShortway (@JayShortway) * [Customer Center] Shows a warning when the app is not the latest version (#4193) via JayShortway (@JayShortway) * Fix integration tests simulator version (#4219) via Cesar de la Vega (@vegaro) * Pin swift-docc-plugin to 1.3.0 (#4216) via James Borthwick (@jamesrb1)
nyeu
pushed a commit
that referenced
this pull request
Oct 2, 2024
Alternative approach to #3875, also meant to address #3871 We're currently looking at a field called Environment in the local receipt to check whether the app is running in sandbox on macOS. However, since it's an undocumented field, we can't trust that it will always be there, so if the value is production then we check the bundle's signature, which can be slow to run. This PR changes that behavior so that we only perform that check if the value for that variable isn't available, which would cause a receipt parsing failure since it's marked as non-optional. Still need to write tests for this, but it should be significantly easier than for the other PR.
nyeu
pushed a commit
that referenced
this pull request
Oct 2, 2024
**This is an automatic release.** ### Bugfixes * Fix `compatibleTopBarTrailing` in MacOS and api tests (#4226) via Cesar de la Vega (@vegaro) * [Paywall] Fix restoreStarted not being called on `presentPaywallIfNeeded` when using `requiredEntitlementIdentifier` (#4223) via Josh Holtz (@joshdholtz) * [CustomerCenter] Move sheet and restore alert creation to `ManageSubscriptionsView` (#4220) via Cesar de la Vega (@vegaro) * [EXTERNAL] `Custom Entitlements Computation`: fix support display on debug screen (#4215) by @NachoSoto (#4218) via Toni Rico (@tonidero) * [Customer Center] Add padding to `No thanks` in promotional offer screen (#4221) via Cesar de la Vega (@vegaro) * Fix version number in plist files (#4213) via Cesar de la Vega (@vegaro) * fix mac os sandbox check slowness (#3879) via Andy Boedo (@aboedo) * [Customer Center] Fix `FeedbackSurveyView` not opening (#4208) via Cesar de la Vega (@vegaro) * Remove `unneeded_override` disable to fix linter (#4209) via Cesar de la Vega (@vegaro) ### Dependency Updates * Bump rexml from 3.3.3 to 3.3.6 in /Tests/InstallationTests/CocoapodsInstallation (#4210) via dependabot[bot] (@dependabot[bot]) * Bump rexml from 3.3.3 to 3.3.6 (#4211) via dependabot[bot] (@dependabot[bot]) ### Other Changes * Update readme wording (#3914) via James Borthwick (@jamesrb1) * Set a maximum duration for iOS 15 tests (#4229) via Cesar de la Vega (@vegaro) * Paywall Components Initial Commit (#4224) via James Borthwick (@jamesrb1) * [CustomerCenter] Open App Store when the user wants to update their app (#4199) via JayShortway (@JayShortway) * [Customer Center] Shows a warning when the app is not the latest version (#4193) via JayShortway (@JayShortway) * Fix integration tests simulator version (#4219) via Cesar de la Vega (@vegaro) * Pin swift-docc-plugin to 1.3.0 (#4216) via James Borthwick (@jamesrb1)
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.
Alternative approach to #3875, also meant to address #3871
We're currently looking at a field called Environment in the local receipt to check whether the app is running in sandbox on macOS.
However, since it's an undocumented field, we can't trust that it will always be there, so if the value is production then we check the bundle's signature, which can be slow to run.
This PR changes that behavior so that we only perform that check if the value for that variable isn't available, which would cause a receipt parsing failure since it's marked as non-optional.
Still need to write tests for this, but it should be significantly easier than for the other PR.