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

Avoid blocking main thread for backupCache calls when leaving app #187

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

Jeehut
Copy link
Contributor

@Jeehut Jeehut commented Sep 21, 2024

I tried different approaches to fix #137, even completely converting the entire SignalCache to an actor. But this caused problems with the SignalManager initializer requiring to be async, also it was not compatible with iOS 12.

But after a bit of research I came across the beginBackgroundTask API which seems to have been invented for exactly purposes like these: We want to write a file to disk exactly when the app is left by the user (and could be killed anytime).

According to the docs:

This method requests additional background execution time for your app. Call this method when leaving a task unfinished might be detrimental to your app’s user experience. For example, call this method before writing data to a file to prevent the system from suspending your app while the operation is in progress.

I've never used this API before, so I'm not an expert. But it sounds like it fits perfectly. Let me know what you think!

@Manuel-Kehl
Copy link

Manuel-Kehl commented Sep 22, 2024

That sounds like the right idea. Only problem with the beginBackgroundTask API is that it is not supported on watchOS. A more general, cross-platform alternative that I have used in my own projects for that reason is performExpiringActivity.

💡Note: In case of cancellation by the system due to timeout, the block that's passed in is called a 2nd time with the expired: Bool set to true. I found this a little counter-intuitive at first so wanted to point it out in case you consider using it 😉

@Jeehut
Copy link
Contributor Author

Jeehut commented Sep 23, 2024

@Manuel-Kehl Thank you for pointing that out! I've read the description of the performExpiringActivity docs but didn't get the feeling it really extends the time the app gets right before termination for "finishing up tasks". It sounds more like targeted towards "app going to background". So for platforms where it's available, I would prefer to use the beginBackgroundTask API as that seems to be built particularly for the purposes we need here.

While I think it makes sense to additionally consider adding support for the performExpiringActivity API for watchOS, I believe it can be done in a separate PR. Because the way this PR is implemented, it doesn't affect the behavior on watchOS or macOS – it only improves the behavior on iOS/iPadOS/visionOS/tvOS.

@winsmith So I think Manuel's concern shouldn't block this from being merged. I suggest we create a separate task for watchOS so we can prioritize it separately from #137, which had high priority as it affects basically every customer. What do you think?

@Manuel-Kehl
Copy link

Based on the documentation here and here:

[...] To request extra execution time from your app extension, call the performExpiringActivity(withReason:using:) method of ProcessInfo instead.

If you’re working in a context where you don’t have access to UIApplication (an app extension or on watchOS) you can achieve a similar effect [to background task API] using the performExpiringActivity(withReason:using:) method on ProcessInfo.

I am pretty confident that performExpiringActivity will also extend the execution time.

How you manage this PR is of course completely up to you. Guess the benefit of only using performExpiringActivity is that it would only be a single code path for all platforms, but you can always refactor this later and go with the diff you already have for now if that's working well for iOS 😉

Just wanted to share the follow up information and pointers to the somewhat scattered documentation that I had in my own notes from back when I adopted this API, so you can possibly revisit it for future work.

On a personal note, I'd love to see this for watchOS, too, because in my experience it happens to be one of the most resource-constrained platforms where the app process tends to get killed by the watchdog most aggressively... 😅

@Jeehut
Copy link
Contributor Author

Jeehut commented Sep 25, 2024

@Manuel-Kehl Thank you for your inputs. I am still not convinced that the API guarantees additional execution time. The docs state:

When your process is in the background, the method tries to take a task assertion to ensure that your block has time to execute. If it is unable to take a task assertion, or if the time allotted for the task assertion expires, the system executes your block with the parameter set to true. If it is able to take the task assertion, it executes the block and passes false for the expired parameter.

If your block is still executing and the system need to suspend the process, the system executes your block a second time with the expired parameter set to true. Your block must be prepared to handle this case. When the expired parameter is true, stop any in-progress tasks as quickly as possible.

The first paragraph basically states that we are not guaranteed to get an expired set to false on first try. And the second paragraph states that we are guaranteed to get a second call into the block but this time expired is definitely true. But when expired is true, we are supposed to "stop any in-progress task as quickly as possible". So, it is totally possible we never get a call with expired set to false. So we have no guarantee we can execute our job. Or am I misunderstanding something? It seems like this API is a "best effort" execution time extension.

And the docs you quoted above both mention "in case you're in an app extension" or "in case you don't have access to UIApplication". So they seem to be "less recommended" when we DO have access to UIApplication and are inside an app. I have no inside knowledge though, so I might be totally wrong and your suggested API might be just as good. 🤷

@winsmith
Copy link
Contributor

winsmith commented Oct 2, 2024

@winsmith So I think Manuel's concern shouldn't block this from being merged. I suggest we create a separate task for watchOS so we can prioritize it separately from #137, which had high priority as it affects basically every customer. What do you think?

I agree, let's deal with WatchOS separately.

@Jeehut Jeehut merged commit 005d1f4 into main Oct 2, 2024
6 checks passed
@Jeehut Jeehut deleted the feature/file-io-main branch October 2, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SignalCache can perform FileIO on the main thread.
3 participants