upload new photos in background with a service #382
Conversation
| @@ -314,7 +314,7 @@ class BackupNotifier extends StateNotifier<BackUpState> { | |||
|
|
|||
| // Perform Backup | |||
| state = state.copyWith(cancelToken: CancellationToken()); | |||
| _backupService.backupAsset( | |||
| await _backupService.backupAsset( | |||
There was a problem hiding this comment.
is this to allow multiple assets to be uploaded at the same time?
There was a problem hiding this comment.
for error handling, I had the most strange debugging issues (and crashes) without all async functions returning a Future and awaiting other async functions
There was a problem hiding this comment.
Is it only when running as a background service?
There was a problem hiding this comment.
Have the issues only when running as background service. The change would of course affect both. I have just re-checked if this await is still necessary for running as a service: Yes. If I remove it, running startBackupProcess fails/stops the executing thread (in an asynchronous error? withour any stacktrace)
There was a problem hiding this comment.
finally figured out what's causing the different behavior (background service vs in foreground app):
The background process is started by the Android WorkManager and executes the new Java/Kotlin BackupWorker. This starts the Dart/Flutter runtime in a new thread. I Had to make the worker aware of running another async thread by not immediately stopping after finishing the code on the Java/Kotlin side. Instead a ResolvableFuture is used to tell the Android system that the process completed (successfully or failed). This signal is send by the dart/flutter thread once it finishes executing (i.e. return Future.value(true); in callbackDispatcher method handler). If the executed dart code calls an async function without await this function is executed in yet another thread and the main dart code continues, finished, and in turn signals Android that the background work is finished. The Android system then kills the entire process. Yet, this non-main async dart function (e.g. backupAsset) was still running and is killed along with its process.
tl;dr: all async functions need to be awaited else the thread/process is killed while executing them in the background.
Not await an async function is also a problem in the foreground app: If an error occurs in that function, it is silently ignored!
|
@alextran1502 I've made some improvements to the service and its handling. However, before continuing further, I'd rather like to plan with you how to integrate the background service nicely. Both code/architecture and user experience/interface aspects. |
|
@zoodyy Agree, I haven't had a chance to go through the code fully. I will let you know as soon as I've done that. I've noticed a few things we might be able to make this simpler on the implementation side. |
| Hive.box<HiveSavedLoginInfo>(hiveLoginInfoBox).get(savedLoginInfoKey); | ||
|
|
||
| var isAuthenticated = | ||
| await container.read(authenticationProvider.notifier).login( |
There was a problem hiding this comment.
Maybe we don't need to use riverpod in the background service since Riverpod is for state management. We can use the generated OpenAPI SDK to make request directly to the server.
There was a problem hiding this comment.
You can take a look at ApiService class at /mobile/lib/shared/services/api.services.dart
There was a problem hiding this comment.
that could be an option. Could lead to some code duplication (backup/uploading in app foreground vs service would take separate paths?).
There was a problem hiding this comment.
This is a fair point. Let's keep the current backup code flow
There was a problem hiding this comment.
keeping the current backup code contradicts with the following, or does it not?
- How to make the background service as lean as possible
With the current ApiService, we can do this very easily.
There was a problem hiding this comment.
I'm not familiar with the app code at all - can you not just call into the existing backup code? (Or refactor it a bit to make that possible)
There was a problem hiding this comment.
it's possible, that's currently what this PR does. but it has its issues (see 5.)
|
What would trigger the background service to start running? What are some points you would like to discuss? What is the recommended testing method for this feature? |
| @@ -412,6 +412,7 @@ class BackupControllerPage extends HookConsumerWidget { | |||
|
|
|||
| void startBackup() { | |||
| ref.watch(errorBackupListProvider.notifier).empty(); | |||
| // TODO Android: Make sure background backup service is disabled & currently not running! | |||
There was a problem hiding this comment.
Should we disable the background service as soon as the app is resumed? It that the mechanism "baked" into the background service implementation on the native side, such that when the app is in foreground, the background service would pause or stop running?
Do you have any good resouce to read on this?
There was a problem hiding this comment.
Whether to disable the service once the app is opened is good question. This is mostly a concurrency issue. What we must prevent is that the background service is uploading something, user opens the app and the foreground app also performs an upload (of potentially the same files). There are multiple options to consider:
- When user opens the app: Stop the service (and kill any currently happening uploads), wait until stopped, check which assets to backup and perform upload as usual in the foreground app. Start service once app is closed ("inactive" / "paused" state)
- Remove foreground backup/upload (on Android) in the app entirely
- Let the user decide whether to use either only foreground upload or use only the background service
Of these options, I think 2. is cleanest (and simplest), 1. is very difficult to get right and potentially wasteful regarding battery as uploads might be canceled and performed twice, 3. is nice as a safeguard if the background upload has issues (so manual foreground upload still works as is) and difficulty is somewhere between the other two options
It that the mechanism "baked" into the background service implementation on the native side, such that when the app is in foreground, the background service would pause or stop running?
There is no automatic stopping (or pausing) of the background service. It runs entirely independent of the app.
Sorry, no good single resource to read (the Android documentation on services might even be confusing due to using the word "foreground" for services..)
There was a problem hiding this comment.
I like option 1 the best because it makes the most sense to the flow of the app.
I'd like to retain the ability to let the user perform the backup as they like and have to choose to opt-in to run background backup so that we don't force users to a fixed workflow.
There was a problem hiding this comment.
I think we can set a flag in the persistent storage of the app (Hive) that the users are using the app and the app is in foreground, background service can check that before performing any action. And vice versa when the users invoke the backup function from the app
There was a problem hiding this comment.
if Hive works fine with concurrent access by different processes, this is good. However, it seems boxes need to be closed before opening in another process.
There was a problem hiding this comment.
Is it possible to have an RPC connection to the background service? You could maybe implement uploading entirely in there and only tell the service to actually do work when the app is open. (Or freely when you enable background upload). You could still have it look like the current foreground upload that way when needed.
Edit: I just read the comment below about when the background service is run, based on that I guess my suggestion is probably not feasible.
There was a problem hiding this comment.
It turns out, this might actually be possible. I've figured out a way to setup message passing between the two threads (foreground and background service)!
I think should be done in a follow-up PR once the background service is well tested and has a counter part on iOS.
Current setup is the following: User activates automatic backup in the app, this enqueues the BackupWorker in the Android WorkManager. Whenever a photo/video is changed on the device, the Android OS starts the BackupWorker (and provides a list of the changed photos/videos). I further added configurable constraints such as battery not low, device is charging (not required by default currently), device has unmetered network connection (WIFI). Only when all constraints are met, the BackupWorker is executed by the system. It has some builtin waiting period (several seconds, I could check if its configurable in the WorkManager API if we wanted to increase it) before starting the worker after a new picture is added so that it is not executed for every single file when taking multiple pictures in a row. Once the service is active, it automatically enqueues itself again with the same constraints. This is necessary because the file-changed constraints are not available to periodic tasks but only to one-time tasks. An alternative (simpler) approach would be to let the system execute the service every x > 15 minutes. This is the minimum interval for background services on Android. However, this is quite wasteful regarding battery usage when not taking any pictures AND backups are delayed longer after taking a photo.
Start the app, click "Turn on backup" in the backup settings, switch to another app, e.g. camera, take a photo, wait ~10 seconds (might be device depended), (optionally look at app debug log, currently there should be |
|
1. How to handle app and background service potentially performing backup at the same time 2. Is communication between background service and foreground app necessary?: Currently not implemented and I am honestly not sure if it is possible (it is somehow possible between native Android app/service and between Dart/Flutter isolates... but might be very challenging in combination) 3. Does the background service need update any persistent state of the app? Is there persisted state e.g. which local files are already backed up to the server, ..) I've read that opening the same hive store concurrently is not supported. 4. How to let the user decide/configure the background service and its constraints (mostly UI) 5. How to make the background service as lean as possible: Not loading riverpod and all services etc. (no login, no checking all assets on device etc.) Simply load auth Bearer token from hive/sharedpreferences etc., make API calls to upload all new photos (as given by files changed info from Android WorkManager), optionally update local persistent state (which files are backed up), exit. This is very important to preserve battery life because currently it is too costly to upload a single new photo. With the current 6. BackgroundService is currently a util with only static methods and two top-level methods. Not sure whether this fits in well. Maybe change the service to a singleton or put it somewhere else (currently modules/backup/services)? That one top-level method (as stated in its documentation comment) needs to stay, however. I can imagine that we will eventually have some more background services or another background service with different logic for iOS, so we can put them in 7. Error handling in the background service: First, let the WorkManager retry the task after some time. If an unrecoverable errors occurs / number of retries exhausted, stop the service & show a native notification to the user: Clicking on it opens the app to possible show more info and let the app enqueue the service again? 8. Edge case handling: If the service uploads only changed files notified by the system, some files will be missed (e.g. after stopping the app by removing it from recent apps, after errors in background upload, etc.). Thus, I feel it is sometimes necessary to check all assets (as it is currently done) and upload all files that have been missed. This could be done in foreground when the user opens the app - this has issues as discussed in #382 (comment). Maybe schedule a periodic task or execute it on app start once per day: This task pauses the normal BackupWorker, performs the full backup with finding any missed files, reenabled the normal BackupWorker, exits. Why don't we always check for all assets with the server before uploading in background service as it is being done in the foreground? This ensures that the most recent information is up to date. |
|
Thanks for the extensive answers!
|
|
Did some performance testing to check if the current backup preparation (code in method All measurements are in milliseconds (3 runs in total). Using the existing backup logic burns (when already optimizing the duplicate call to |
This is a very good assessment and testing, thank you. Another direction I think we can try to mitigate the overhead of using our traditional backup method is to check if the photos/videos are already on the server by directly asking the server. We currently have that API/Endpoint (
With the minimal assets will be back up each time. I think most cases will be under 10, and I think this won't sweat the server too much even if it has 100,000 records in the database Edited: Those two are not indexes. They are unique columns, we can also make them indexes. |
Good idea! I'll absolutely use that API to check for duplicates. Pairs extremely well with the short list of changed files the background service receives. This should reduce the Might even work for the currently used full check (i.e. uploading 6000 photo IDs to server and let it check if they are already there instead of downloading 6000 IDs and checking on device). Thus, the server does the comparison by looping over all ids.. or one could maybe write a single, optimized SQL query. |
I like this idea, any return IDs mean that they are good to go. |
fyfrey
force-pushed
the
feature/backgroundBackup
branch
from
45a40e2 to
cd6c8fd
Compare
| // foreground services are allowed to run indefinitely | ||
| // requires battery optimizations to be disabled (either manually by the user | ||
| // or by the system learning that immich is important to the user) | ||
| setForegroundAsync(createForegroundInfo()) |
There was a problem hiding this comment.
If a new asset is added to an excluded album, onPhotosChanged is never called and a blank notification is shown for a few seconds. Maybe you could add default content (something like "Checking for new assets...").
There was a problem hiding this comment.
That's a good idea!
On my virtual devices, Android shows the foreground notification only after ~10 seconds, so this should not happen usually. However, some older devices might show it sooner.
Note: onPhotosChanged is called regardless of which albums were changed
|
I used this for some hours now and it seems to work very well. There could always be situations where some power saving logic kicks in (my phone is very strict with that) but we have to see how this works on the long term. My suggestion would be to merge this (it does not seem to break anything existing) and do potential optimizations in a second PR once we received some user feedback about this. This PR in its current state already is a huge improvement IMO. LGTM! |
|
Hey, Just catching up on this PR and the work you've done is great! I concur with @matthinc that we could merge this and then make performance improvements in the second iteration. I do have some suggestions though that might be worth taking onboard before we do so:
Let me know what you think about all of this and feel free to reply here or ping me on the discord if you want to discuss further. Cheers for the work on this PR! |
Thanks! And you are right about the power saving logic. Android (in somewhat recent versions) is very strict about background battery usage. Thus, the system decides when to run the backup worker (usually scheduled together with other tasks to only wake up the CPU from sleep once and use battery expensive network in one batch).
Great! I've still got some TODOs in my code that I want to resolve beforehand (mainly internationalization) |
I agree. Currently, WiFi is always required, but not charging. It's already configurable in Dart code. I could add toggles for these two settings in the backup screen as well, likely below the button to start/stop the background service.
It uploads almost everything that has not yet been uploaded. There is one exception: The user could move a photo from an excluded album to an included album. This would currently not be detected. I'd rather solve this issue in a follow-up PR.
Agreed. I'd do that in another PR as well once the background service has been tested to work well in general.
True, I would also advocate for a local client state to keep track which assets have already been backed up. It also allows to mitigate the issue 2. However, as it would severely change the current foreground backup, I'd rather tackle this in a separate PR.
Thanks! I'll reach out to you on discord to discuss some of the details |
|
Thanks, @zoodyy, for an amazing PR. I just did a quick test, and there are some bugs that I want to put the steps to reproduce here. Tested on Samsung S9 release version Case 1 - App's states aren't updating
Case 2 - Disable background backup not working
I would like to sort out these issues before merging the PR Some info about the bug Here is the log when I toggle the enable background backup button Turning on Turning off The methods in question are |
| ), | ||
| ); | ||
| } | ||
|
|
||
| /// | ||
| /// Invoke backup process | ||
| /// | ||
| void startBackupProcess() async { | ||
| Future<void> startBackupProcess() async { | ||
| assert(state.backupProgress == BackUpProgressEnum.idle); |
There was a problem hiding this comment.
Would this crash the app?
There was a problem hiding this comment.
in debug mode, yes.
In release mode, no as all asserts are disabled
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| Future<void> resumeBackup() async { |
There was a problem hiding this comment.
I understand this is called from the external context of the file, let's put in some comments so late comers understand why there are two resumeBackup functions here
| @@ -0,0 +1,379 @@ | |||
| import 'dart:async'; | |||
There was a problem hiding this comment.
probably rename this file to android_background_service I think we will have the same counterpart for iOS as well
There was a problem hiding this comment.
I've read about dart/flutter platform specific plugins and usually you use the same dart file/class to communicate with both Android and iOS.
|
@zoodyy I've just done some testing. One of the behaviors that are worth reporting is
This is not a major issue, this can be treated as expected behavior if it simplifies the code. The user simply needs to take a new photo to Thanks for the amazing work; I know a lot of Android users will be very happy with this feature. |
| onPressed: () { | ||
| AutoRouter.of(context).push(const BackupAlbumSelectionRoute()); | ||
| }, | ||
| onPressed: hasExclusiveAccess |
There was a problem hiding this comment.
Why do we have this condition?
There was a problem hiding this comment.
The user must not change the selected albums while the background service is running as both would fight over the Hive box (and a Hive box can only be opened by one thread at a time)
This is exactly the expected behavior. Everything works as intended: The background backup is only triggered to run whenever assets change (are added/deleted/modified). Since the photo was taken and thus the background service ran before it should backup the photo (only Screenshot album was allowed at that time), the service found that it must not upload that photo. Later, you changed the settings to upload everything (Recent), the next background backup run adds the old photo. However, changing the settings does not directly trigger it, only changing assets (in this case adding a new photo) In a future PR, we can improve upon this by replacing the foreground upload with the background upload and schedule it to run immediately whenever the user changes which albums to backup etc. In theory, I could easily add this to the current PR, however would rather not force my luck that the foreground and background backup do not interfere :-)
Some documentation on the WIKI is certainly handy
Indeed! Feels a lot better knowing that photos are automatically backed up without needing to regularly start the app to perform the backup |
fyfrey
force-pushed
the
feature/backgroundBackup
branch
from
da44cf9 to
0a73749
Compare
fix communication erros with Kotlin plugin on start/stop service methods better error handling for BackgroundService public methods Add default notification message when service is running
fyfrey
force-pushed
the
feature/backgroundBackup
branch
from
0a73749 to
da14908
Compare
|
This PR should now be ready to be merged 🎉
|
fyfrey
force-pushed
the
feature/backgroundBackup
branch
from
da14908 to
941fc82
Compare
Background backup service
This is more proper attempt at automatically uploading new photos in a background service even if the app is not open. Partly fixes issue #235. To enable this feature, click the new
enable background backupbutton on theBackupscreen.Only works on Android!
Overview: How does this work
Androids
WorkManageris used to run a Kotlin/Java service whenever a new photo/video is stored on the device (by taking a picture or receiving an image via some messaging app) or modified (by editing an existing image). This classBackupWorkerstarts the Dart engine which then performs the actual backup in Dart (withBackgroundServiceandBackupService).Battery efficiency
The backup logic in the foreground app currently scans all photos/videos on the device to perform, the backup. Doing this every time a photo is taken, would eat up the users battery very quickly. To solve this issue, the background backup considers only assets newer (modified date) than the last time the corresponding album has successfully been backed up.
Error handling
The background service is only executed by Android if all constraints are met (e.g. battery not low, wifi connection etc., this is already configurable in Dart code, but not in the UI). Whenever these constraints no longer hold (e.g. by losing wifi connection), Android stops the
BackupWorker. The backup process is correctly shut down and newly enqueued (without waiting for a new photo to be taken!) to run once the other constraints are fulfilled.Errors during the background backup (e.g. server becoming unavailable) produce a native notification: Failed to upload file ... etc.
Concurrency issues
Since HiveDB does not support concurrent access, manual locking is implemented via message passing between the foreground and background thread. Which thread first takes the lock (gains exclusive access) proceeds. The background service simply waits until the foreground app is no longer active. In the foreground app, certain operations are not available while the service is running / has the lock: Changing the albums to back up and starting/continuing the backup process in the foreground. These operations become automatically available as soon as the background service finishes / releases the lock.