Add Sync Worker #1255
Add Sync Worker #1255
Conversation
# Conflicts: # RELEASE-NOTES.txt # Simplenote/src/main/java/com/automattic/simplenote/Simplenote.java
# Conflicts: # RELEASE-NOTES.txt # Simplenote/src/main/java/com/automattic/simplenote/Simplenote.java
theck13
added
[status] needs code review
task
|
You can test the changes on this Pull Request by downloading the APK here. |
|
This should definitely help keep things more up to date while people have the app closed or in the background, but I know you have been concerned about battery usage and it makes me wonder why we are starting the worker when we have no changes queued up. Wouldn't we want to only run this until all local changes have been synced? That is, once the app closes we keep a background worker alive until the local queue has flushed out and then we can safely shut everything down. Any changes that come in while the app is asleep should apply cleanly since we wouldn't have had the opportunity to create new local changes. Is the goal to make sure that we never get behind on a bucket's |
|
Our theory is the high battery usage reported years ago was because the buckets remained started and tried syncing even if there wasn't a network connection. Both the active network connection constraint and the
Yeah, it would be great if there was a callback from Simperium that told us all local changes have been synced. We could use that mechanism as a constraint for the sync worker such that we only trigger the background sync worker if there are local changes. That may also help us display the last synced time per note.
That is a little tricky. If there is no active network connection, we may be keeping the background worker alive indefinitely causing the high battery usage issue reported before.
That's would be one benefit. The major reason is the user experience. Users expect that any edit made on Android will be synced across all their devices as soon as their Android device has a network connection whether the Simplenote app is open or not. Currently, syncing only occurs when there is a network connection and the Simplenote app is open. Adding background sync will help meet user expectations when they make an edit on Android and close the app before syncing completes due to the network connection or something else. |
Seems like a non-issue since I would assume we'd follow the same steps this PR follows to avoid that scenario. The only difference would be killing the background worker once local changes are sync'd.
This again points me to question the need to keep a worker alive forever beyond the point where those changes are sync'd.
Should we not make one then? I'd be happy to try and help. It seems like the benefit would be big. I guess there's limited harm merging this without it but I was remember how much you highlighted the other concerns running a worker forever. |
Ah, I thought you meant doing something different than these changes and just keeping the worker alive regardless of constraints.
Yeah, let's do it. It's on my list to do after this.
I agree. The previous concerns were more about the frequency of starting/stopping the buckets, which was once per minute. Now, they are once per fifteen minutes, which I think it fine since it's the minimum interval for |
There was a problem hiding this comment.
While I think having a periodic worker running each 15 minutes is an overkill for the sync issue, I understand the current limitation of Simperium which requires it, please just check my comment regarding the execution of the code scheduling the worker.
And as stated in our previous discussion, I don't think using WorkManager workers will solve the issue 100%, as they are not guaranteed to run at the specified constraints, but may be delayed, or not run at all when the device is in Doze mode (or in some OEM ROMs, speaking from a previous experience 🙂). I still think that firing a foreground service when the app exits and let it run for a short period, or use the new WorkManager API, is the only guaranteed way to make sure it will run. And when Simperium gets updated with a callback about sync status, it'll make using a foreground service a lot better, as it will only run if there is some sync to be done, and will be finished when the sync has been completed.
But for a short term solution, I think this PR will solve the issue for most users.
| PeriodicWorkRequest syncWorkRequest = new PeriodicWorkRequest.Builder( | ||
| SyncWorker.class, | ||
| PeriodicWorkRequest.MIN_PERIODIC_INTERVAL_MILLIS, | ||
| TimeUnit.MILLISECONDS | ||
| ) | ||
| .setConstraints(new Constraints.Builder().setRequiredNetworkType(NetworkType.CONNECTED).build()) | ||
| .setBackoffCriteria(BackoffPolicy.LINEAR, ONE_MINUTE_MILLIS, TimeUnit.MILLISECONDS) | ||
| .addTag(TAG_SYNC) | ||
| .build(); | ||
| WorkManager.getInstance(getApplicationContext()).enqueueUniquePeriodicWork( | ||
| TAG_SYNC, | ||
| ExistingPeriodicWorkPolicy.REPLACE, | ||
| syncWorkRequest | ||
| ); | ||
| Log.d("Simplenote.onTrimMemory", "Started worker"); |
There was a problem hiding this comment.
This code is not guaranteed to run, as the system may kill the app before the 10s delay finishes. You can reproduce it by swiping away the app from recents menu.
I suggest you move this block outside the delayed runnable, and use setInitialDelay on the request to have a minimum delay of 10 seconds.
There was a problem hiding this comment.
I moved the enqueue outside of the post delayed runnable and added a twenty second initial delay in 22d02da and d0c9c30. I chose to use twenty seconds as the initial delay to ensure a race condition doesn't occur when the buckets are stopped in ApplicationLifecycleMonitor.onTrimMemory after they are started in SyncWoker.startWork.
There was a problem hiding this comment.
If a device doesn't follow AOSP practices or constraints, then we don't support that device. If the sync worker is delayed, that's fine. Since background syncing does not require an active user and doesn't need to run at an exact time, a deferred task using WorkManager is the recommended implementation according to the documentation for background processing. If battery saver or doze mode are enabled and our sync worker isn't triggered, that's fine. Actually, that behavior is preferable since it respects the operating system constraints. We don't want to violate things like that.
I think using WorkManager is better than a foreground service. A foreground service is meant for performing an operation noticeable to the user. Showing an indicator on every sync is against the minimalistic design of Simplenote. Also, a foreground service requires a status bar notification. Showing a notification periodically even when the app hasn't been used can be confusing. I don't think we should use WorkManager 2.3.0-alpha02 since it is the same as a foreground service and using an alpha library in production can be buggy.
| PeriodicWorkRequest syncWorkRequest = new PeriodicWorkRequest.Builder( | ||
| SyncWorker.class, | ||
| PeriodicWorkRequest.MIN_PERIODIC_INTERVAL_MILLIS, | ||
| TimeUnit.MILLISECONDS | ||
| ) | ||
| .setConstraints(new Constraints.Builder().setRequiredNetworkType(NetworkType.CONNECTED).build()) | ||
| .setBackoffCriteria(BackoffPolicy.LINEAR, ONE_MINUTE_MILLIS, TimeUnit.MILLISECONDS) | ||
| .addTag(TAG_SYNC) | ||
| .build(); | ||
| WorkManager.getInstance(getApplicationContext()).enqueueUniquePeriodicWork( | ||
| TAG_SYNC, | ||
| ExistingPeriodicWorkPolicy.REPLACE, | ||
| syncWorkRequest | ||
| ); | ||
| Log.d("Simplenote.onTrimMemory", "Started worker"); |
There was a problem hiding this comment.
I moved the enqueue outside of the post delayed runnable and added a twenty second initial delay in 22d02da and d0c9c30. I chose to use twenty seconds as the initial delay to ensure a race condition doesn't occur when the buckets are stopped in ApplicationLifecycleMonitor.onTrimMemory after they are started in SyncWoker.startWork.
I don't think we can actually do that, as it will impact a lot of users.
It depends on the user's usage, if a user is using two devices at the same time, the delay won't be OK 🙂. I still think that a foreground service will be more battery friendly than the current solution, and it won't bother users to see the notification for some seconds until the sync is done (which is not needed in most cases, as the sync would be complete before the user exits the app), FI the same behavior is used by a lot of apps, including Youtube when checking for "downloads recommendations". But anyway, let's agree to disagree on this point 🙂 |
I don't think we can actually do that, as it will impact a lot of users, and it was your remark too in our previous discussion 🙂: Officially, we only support AOSP ROM's. Unofficially, we help users with devices that are AOSP-based, but not necessarily pure AOSP ROM's. If the sync worker is delayed, that's fine.It depends on the user's usage, if a user is using two devices at the same time, the delay won't be OK 🙂. Simplenote is not marketed as a real-time collaboration app in which two devices are used simultaneously. If the devices and networks are fast enough to do that, that's fine, but it's not our targeted behavior. Also, if the two devices are being used at the same time, there won't be a delay due to a background sync worker if the app is in the foreground. I still think that a foreground service will be more battery friendly than the current solution That's not entirely true. There are definitely situations in which a foreground service would use more battery than it won't bother users to see the notification for some seconds until the sync is done That's not always true. It's quite possible any notification for any duration will bother users. the same behavior is used by a lot of apps, including Youtube when checking for "downloads recommendations". Simplenote is not trying to copy the behavior of apps like YouTube. If you would like to discuss this further, send me a direct message. |
Fix
Add a worker to run syncing tasks in the background to avoid data loss as described in #992 among other issues. The background tasks are starting and stopping the note, tag, and preference buckets, which effectively triggers syncing. Currently, the buckets are stopped after the app has been closed for ten seconds. The worker added in these changes starts the buckets after that delay. It also has a backoff criteria of one minute and a constraint which requires an active network connection. The worker starts the buckets and stops them after ten seconds. The flow is as follows:
Effectively, the buckets are started for ten seconds every fifteen minutes when there is a network connection and the app is in the background.
Adding a sync worker was originally attempted in #1137. That pull request was closed without merging due to concerns about the implementation. These changes build on that pull request with three major differences. First, the sync worker was updated from
WorkertoListenableWorker.Workeris a class that performs work synchronously on a background thread provided byWorkManager.ListenableWorkeris a class that can perform work asynchronously inWorkManager. Since starting and stopping the buckets after ten seconds is asynchronous,ListenableWorkerwas used. Second, the original implementation started and stopped the buckets every minute. This implementation uses thePeriodicWorkRequest.MIN_PERIODIC_INTERVAL_MILLISconstant, which is fifteen minutes, for the repeat interval. Third, creating and enqueuing the sync worker was moved fromApplicationLifecycleMonitor.onActivityPausedtoApplicationLifecycleMonitor.onTrimMemoryto avoid the race condition when the buckets are stopped inApplicationLifecycleMonitor.onTrimMemoryafter they are started inSyncWoker.startWork. That would nullify the sync worker and disable syncing until the repeat interval has elapsed fifteen minutes later.Test
There aren't any user-facing changes to this pull request. The best way to see what's happening is to watch the logcat with the logs filtered to show "Debug" only and the "(Started)|(Stopped)" regular expression is used.
To test the flow described above, open the app then tap the recents/overview system navigation button to trigger starting the sync worker after a ten second delay. If you watch the logs for another fifteen minutes without opening the Simplenote app, you'll see the buckets started and stopped again. See the logs below for example.
To test the network connection constraint, enable airplane mode before tapping the recents/overview system navigation button. The sync worker will be started, but the buckets will not be started and stopped until airplane mode is disabled. See the logs below for example.
Review
Only one developer is required to review these changes, but anyone can perform the review. @malinajirka, I requested you as a reviewer since you reviewed the initial pull request for adding a sync worker. Don't feel obligated to review this pull request too, but you can if you want. I just wanted to notify you of the second attempt in case you wanted to verify your concerns have been addressed.
Release
RELEASE-NOTES.txtwas updated in 2f92028 with: