fix mac os sandbox check slowness #3875

aboedo · 2024-05-07T19:02:44Z

Addresses #3871

Solves the issue by ensuring that the calls to isSandbox are performed on a worker thread, and that we're pre-loading the values.

I'm not a huge fan of this solution since it isn't technically perfectly safe, meaning that it's possible (albeit very unlikely) for us to still check sandbox values in the main thread.

Additionally I don't love how hard it is to actually write tests for it.

The ideal solutions would be:

change the check so that it loads instantly, which would fix the issue without further changes. But... I don't know of a faster way to check on macOS, sadly.
update isSandbox to make it asynchronous... which would be a BIG refactor, affecting a large chunk of the codebase. It doesn't feel worth the trouble.

aboedo · 2024-05-07T19:03:03Z

tagging @MarkVillacampa here too since he's worked on these checks quite a bit

RevenueCat-Danger-Bot · 2024-05-07T19:03:11Z

	1 Message
📖	Size increase: 1.67 KB

Generated by 🚫 Danger

aboedo · 2024-05-07T19:03:50Z

Sources/Identity/CustomerInfoManager.swift

@@ -99,23 +99,26 @@ class CustomerInfoManager {
    func fetchAndCacheCustomerInfoIfStale(appUserID: String,
                                          isAppBackgrounded: Bool,
                                          completion: CustomerInfoCompletion?) {


Note: I'd recommend setting the diff to hide whitespace, since it makes it much clearer that all this change does is to add a single line to run the code in a worker thread

aboedo · 2024-05-07T19:04:58Z

Sources/Misc/MapAppStoreDetector.swift

@@ -32,6 +32,8 @@ final class DefaultMacAppStoreDetector: MacAppStoreDetector {
    /// it checked the common name but was changed to an extension check to make it more
    /// future-proof.
    ///
+    /// - warning: this method may take a long time to run on macOS. It should not be called on the main thread. 


these warnings should ideally be fixed by the solutions described in the PR description, but I couldn't find a great way to make them work, sadly

aboedo · 2024-05-07T19:06:36Z

Sources/Misc/SystemInfo.swift

+        // eager-load the isSandbox value from a worker thread so that's immediately available
+        // this is useful on macOS where it may take long for the value to compute
+        self.operationDispatcher.dispatchOnWorkerThread {
+            _ = self._isSandbox
+        }


this pre-loading makes the changes quite safe. After all, for the most part we'll only be loading once, as soon as SystemInfo is created, and from a worker thread.
So other accesses will likely not hit this from a different thread. Ideally we'd modify the property to be asynchronous though so that we're completely covered.

Hmm I imagine we could potentially be calculating the isSandbox property multiple times at the same time right? One here on initialization in a worker thread, then another in the main thread, if that property gets accessed from the main thread (Or even more if accessed from multiple threads). I think this might be ok, but I wanted to mention it.

I also see that we are accessing the sandboxEnvironmentDetector.isSandbox in multiple places, like the DeviceCache or the StoreKit1Wrapper. Do we need to handle those as well? (That can happen in different PRs though)

we could, but it shouldn't be a problem other than it being inefficient the first time. But we were already doing that before this PR, the changes here just ensure that it happens on a worker thread instead

aboedo · 2024-05-07T19:07:00Z

Sources/Purchasing/Purchases/Purchases.swift

@@ -1743,21 +1743,23 @@ private extension Purchases {
        // Note: it's important that we observe "will enter foreground" instead of
        // "did become active" so that we don't trigger cache updates in the middle
        // of purchases due to pop-ups stealing focus from the app.
-        self.updateAllCachesIfNeeded(isAppBackgrounded: false)
-        self.dispatchSyncSubscriberAttributes()
+        self.operationDispatcher.dispatchOnWorkerThread {


I really dislike how indirect this is, but it's not a bad idea to dispatch this work on the worker thread in any case

I'm slightly concerned about any unknown side-effects... but nothing comes to mind right now. I think this should be safe yeah.

I believe it should be okay, all this code is already meant to be working on its own without user interaction (other than the user opening the SDK, that is)

aboedo · 2024-05-07T19:08:11Z

Tests/UnitTests/Identity/CustomerInfoManagerTests.swift

+        }
+
+        expect(invokedCompletion).toEventually(beTrue())
+        expect(self.mockOperationDispatcher.invokedDispatchOnWorkerThread) == true


Not a fan of this since it's not a very accurate check, other things could be invoking this method.
I don't know of a much better way to test when using this implementation, though.

throwing something crazy in here, and not sure it will work, but maybe modifying dispatchOnWorkerThread to this:

func dispatchOnWorkerThread(delay: Delay = .none, caller: String = #function, file: String = #file, line: Int = #line, block: @escaping @Sendable () -> Void) {

would let you check what has called dispatchOnWorkerThread in the mock?

definitely an interesting approach, I'll take a look

aboedo · 2024-05-07T19:14:56Z

Note: this was only an issue in production, because of this check which would early-return true in sandbox

tonidero

I'm worrying that this solution might be a bit brittle, as you said... but I believe it should be safe to merge this.

tonidero · 2024-05-08T07:18:41Z

Sources/Misc/SystemInfo.swift

+        // eager-load the isSandbox value from a worker thread so that's immediately available
+        // this is useful on macOS where it may take long for the value to compute
+        self.operationDispatcher.dispatchOnWorkerThread {
+            _ = self._isSandbox
+        }


Hmm I imagine we could potentially be calculating the isSandbox property multiple times at the same time right? One here on initialization in a worker thread, then another in the main thread, if that property gets accessed from the main thread (Or even more if accessed from multiple threads). I think this might be ok, but I wanted to mention it.

I also see that we are accessing the sandboxEnvironmentDetector.isSandbox in multiple places, like the DeviceCache or the StoreKit1Wrapper. Do we need to handle those as well? (That can happen in different PRs though)

tonidero · 2024-05-08T07:20:38Z

Sources/Purchasing/Purchases/Purchases.swift

@@ -1743,21 +1743,23 @@ private extension Purchases {
        // Note: it's important that we observe "will enter foreground" instead of
        // "did become active" so that we don't trigger cache updates in the middle
        // of purchases due to pop-ups stealing focus from the app.
-        self.updateAllCachesIfNeeded(isAppBackgrounded: false)
-        self.dispatchSyncSubscriberAttributes()
+        self.operationDispatcher.dispatchOnWorkerThread {


I'm slightly concerned about any unknown side-effects... but nothing comes to mind right now. I think this should be safe yeah.

aboedo · 2024-05-08T14:07:41Z

@tonidero yeah I definitely still don't love this solution. Maybe it makes sense to limit its effects to macOS for the sake of minimizing impact... Or to give the asynchronous isSandbox approach another try, even though that one feels a lot more impactful in terms of the changes

aboedo · 2024-08-22T22:14:03Z

closing in favor of #3875

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.

aboedo added 3 commits May 7, 2024 15:13

fixed an issue where macOS sandbox checks would take long to run

5ba3406

added one test

8c166ef

added tests

98b19be

aboedo requested a review from a team May 7, 2024 19:02

aboedo self-assigned this May 7, 2024

aboedo requested a review from MarkVillacampa May 7, 2024 19:02

fix test

1425798

aboedo commented May 7, 2024

View reviewed changes

aboedo added the pr:fix A bug fix label May 7, 2024

lint

04b86be

jamesrb1 approved these changes May 7, 2024

View reviewed changes

tonidero approved these changes May 8, 2024

View reviewed changes

aboedo mentioned this pull request May 8, 2024

fix mac os sandbox check slowness #3879

Merged

aboedo closed this Aug 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix mac os sandbox check slowness #3875

fix mac os sandbox check slowness #3875

aboedo commented May 7, 2024

aboedo commented May 7, 2024

RevenueCat-Danger-Bot commented May 7, 2024 •

edited

Loading

aboedo May 7, 2024

aboedo May 7, 2024

aboedo May 7, 2024

tonidero May 8, 2024

aboedo May 8, 2024

aboedo May 7, 2024

tonidero May 8, 2024

aboedo May 8, 2024

aboedo May 7, 2024

vegaro May 7, 2024

aboedo May 8, 2024

aboedo commented May 7, 2024 •

edited

Loading

tonidero left a comment

tonidero May 8, 2024

tonidero May 8, 2024

aboedo commented May 8, 2024

aboedo commented Aug 22, 2024

fix mac os sandbox check slowness #3875

fix mac os sandbox check slowness #3875

Conversation

aboedo commented May 7, 2024

aboedo commented May 7, 2024

RevenueCat-Danger-Bot commented May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo commented May 7, 2024 • edited Loading

tonidero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo commented May 8, 2024

aboedo commented Aug 22, 2024

RevenueCat-Danger-Bot commented May 7, 2024 •

edited

Loading

aboedo commented May 7, 2024 •

edited

Loading