From cae1ed5efb341f02de6d112f1b4d6f6a221486e4 Mon Sep 17 00:00:00 2001 From: Ricki Hirner Date: Thu, 22 Feb 2024 11:56:44 +0100 Subject: [PATCH] More correct usage of expedited workers (#566) * Add foreground notification type to expedited workers (required for Android 14) * Make SyncWorker a long-running worker * Don't use expedited SyncWorker for everything; handle foreground service launch restriction * AddressBookSyncer: only request expedited for sub-jobs when parent job is expedited, too * RefreshCollectionsWorker is not long-running -> no foreground service type needed * Fix tests * Don't use foreground service type in ForegroundInfo * Make SyncWorker not long-running --- .../davdroid/syncadapter/SyncWorkerTest.kt | 2 +- .../RefreshCollectionsWorker.kt | 6 +++- .../davdroid/syncadapter/AddressBookSyncer.kt | 7 +++-- .../syncadapter/PeriodicSyncWorker.kt | 6 +++- .../syncadapter/SyncAdapterServices.kt | 2 +- .../davdroid/syncadapter/SyncWorker.kt | 28 ++++++++++++++----- .../davdroid/ui/account/SettingsActivity.kt | 2 +- 7 files changed, 39 insertions(+), 14 deletions(-) diff --git a/app/src/androidTestOse/kotlin/at/bitfire/davdroid/syncadapter/SyncWorkerTest.kt b/app/src/androidTestOse/kotlin/at/bitfire/davdroid/syncadapter/SyncWorkerTest.kt index 6108bfef..7eead336 100644 --- a/app/src/androidTestOse/kotlin/at/bitfire/davdroid/syncadapter/SyncWorkerTest.kt +++ b/app/src/androidTestOse/kotlin/at/bitfire/davdroid/syncadapter/SyncWorkerTest.kt @@ -72,7 +72,7 @@ class SyncWorkerTest { @Test fun testEnqueue_enqueuesWorker() { - SyncWorker.enqueue(context, account, CalendarContract.AUTHORITY) + SyncWorker.enqueue(context, account, CalendarContract.AUTHORITY, true) val workerName = SyncWorker.workerName(account, CalendarContract.AUTHORITY) assertTrue(workScheduledOrRunningOrSuccessful(context, workerName)) } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/servicedetection/RefreshCollectionsWorker.kt b/app/src/main/kotlin/at/bitfire/davdroid/servicedetection/RefreshCollectionsWorker.kt index 2a970bcd..e7470703 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/servicedetection/RefreshCollectionsWorker.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/servicedetection/RefreshCollectionsWorker.kt @@ -84,6 +84,10 @@ import kotlin.collections.* * - adds resources if new ones are detected * - removes resources if not found 40x (delete locally) * + * Expedited: yes (always initiated by user) + * + * Long-running: no + * * @throws IllegalArgumentException when there's no service with the given service ID */ @HiltWorker @@ -577,4 +581,4 @@ class RefreshCollectionsWorker @AssistedInject constructor( } } -} +} \ No newline at end of file diff --git a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/AddressBookSyncer.kt b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/AddressBookSyncer.kt index 5796386c..3efe863b 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/AddressBookSyncer.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/AddressBookSyncer.kt @@ -30,7 +30,10 @@ import java.util.logging.Level /** * Sync logic for address books */ -class AddressBookSyncer(context: Context): Syncer(context) { +class AddressBookSyncer( + context: Context, + private val expedited: Boolean +) : Syncer(context) { @EntryPoint @InstallIn(SingletonComponent::class) @@ -53,7 +56,7 @@ class AddressBookSyncer(context: Context): Syncer(context) { if (updateLocalAddressBooks(account, syncResult)) for (addressBookAccount in LocalAddressBook.findAll(context, null, account).map { it.account }) { Logger.log.log(Level.INFO, "Running sync for address book", addressBookAccount) - SyncWorker.enqueue(context, addressBookAccount, ContactsContract.AUTHORITY) + SyncWorker.enqueue(context, addressBookAccount, ContactsContract.AUTHORITY, expedited = expedited) } } catch (e: Exception) { Logger.log.log(Level.SEVERE, "Couldn't sync address books", e) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/PeriodicSyncWorker.kt b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/PeriodicSyncWorker.kt index db211db8..ef1a4291 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/PeriodicSyncWorker.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/PeriodicSyncWorker.kt @@ -32,6 +32,10 @@ import java.util.concurrent.TimeUnit * The different periodic sync workers each carry a unique work name composed of the account and * authority which they are responsible for. For each account there will be multiple dedicated periodic * sync workers for each authority. See [PeriodicSyncWorker.workerName] for more information. + * + * Deferrable: yes (PeriodicWorkRequest) + * + * Long-running: no */ @HiltWorker class PeriodicSyncWorker @AssistedInject constructor( @@ -121,7 +125,7 @@ class PeriodicSyncWorker @AssistedInject constructor( // Just request immediate sync Logger.log.info("Requesting immediate sync") - SyncWorker.enqueue(applicationContext, account, authority) + SyncWorker.enqueue(applicationContext, account, authority, expedited = false) return Result.success() } } \ No newline at end of file diff --git a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncAdapterServices.kt b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncAdapterServices.kt index 28a34d23..2c972bb9 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncAdapterServices.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncAdapterServices.kt @@ -79,7 +79,7 @@ abstract class SyncAdapterService: Service() { } Logger.log.fine("Sync framework now starting SyncWorker") - val workerName = SyncWorker.enqueue(context, account, authority, upload = upload) + val workerName = SyncWorker.enqueue(context, account, authority, expedited = true, upload = upload) // Block the onPerformSync method to simulate an ongoing sync Logger.log.fine("Blocking sync framework until SyncWorker finishes") diff --git a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncWorker.kt b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncWorker.kt index 48ab609c..dbefc53e 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncWorker.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncWorker.kt @@ -63,6 +63,7 @@ import java.util.logging.Level * * By enqueuing this worker ([SyncWorker.enqueue]) a sync will be started immediately (as soon as * possible). Currently, there are three scenarios starting a sync: + * * 1) *manual sync*: User presses an in-app sync button and enqueues this worker directly. * 2) *periodic sync*: User defines time interval to sync in app settings. The [PeriodicSyncWorker] runs * in the background and enqueues this worker when due. @@ -70,6 +71,9 @@ import java.util.logging.Level * button in one of the responsible apps. The [SyncAdapterService] is notified of this and enqueues * this worker. * + * Expedited: when run manually + * + * Long-running: no */ @HiltWorker class SyncWorker @AssistedInject constructor( @@ -84,6 +88,9 @@ class SyncWorker @AssistedInject constructor( internal const val ARG_ACCOUNT_TYPE = "accountType" internal const val ARG_AUTHORITY = "authority" + /** Boolean. Set to `true` when the job was requested as expedited job. */ + private const val ARG_EXPEDITED = "expedited" + private const val ARG_UPLOAD = "upload" private const val ARG_RESYNC = "resync" @@ -129,7 +136,7 @@ class SyncWorker @AssistedInject constructor( upload: Boolean = false ) { for (authority in SyncUtils.syncAuthorities(context)) - enqueue(context, account, authority, resync, upload) + enqueue(context, account, authority, expedited = true, resync = resync, upload = upload) } /** @@ -146,6 +153,7 @@ class SyncWorker @AssistedInject constructor( context: Context, account: Account, authority: String, + expedited: Boolean, @ArgResync resync: Int = NO_RESYNC, upload: Boolean = false ): String { @@ -154,6 +162,8 @@ class SyncWorker @AssistedInject constructor( .putString(ARG_AUTHORITY, authority) .putString(ARG_ACCOUNT_NAME, account.name) .putString(ARG_ACCOUNT_TYPE, account.type) + if (expedited) + argumentsBuilder.putBoolean(ARG_EXPEDITED, true) if (resync != NO_RESYNC) argumentsBuilder.putInt(ARG_RESYNC, resync) argumentsBuilder.putBoolean(ARG_UPLOAD, upload) @@ -165,7 +175,6 @@ class SyncWorker @AssistedInject constructor( val workRequest = OneTimeWorkRequestBuilder() .addTag(workerName(account, authority)) .setInputData(argumentsBuilder.build()) - .setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) .setBackoffCriteria( BackoffPolicy.EXPONENTIAL, WorkRequest.DEFAULT_BACKOFF_DELAY_MILLIS, // 30 sec @@ -179,17 +188,20 @@ class SyncWorker @AssistedInject constructor( addTag(workerName(mainAccount, authority)) } } - .build() + + if (expedited) + workRequest.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) // enqueue and start syncing val name = workerName(account, authority) - Logger.log.log(Level.INFO, "Enqueueing unique worker: $name, with tags: ${workRequest.tags}") + val request = workRequest.build() + Logger.log.log(Level.INFO, "Enqueueing unique worker: $name, expedited = $expedited, tags = ${request.tags}") WorkManager.getInstance(context).enqueueUniqueWork( name, ExistingWorkPolicy.KEEP, // If sync is already running, just continue. // Existing retried work will not be replaced (for instance when // PeriodicSyncWorker enqueues another scheduled sync). - workRequest + request ) return name } @@ -305,8 +317,9 @@ class SyncWorker @AssistedInject constructor( inputData.getString(ARG_ACCOUNT_TYPE) ?: throw IllegalArgumentException("$ARG_ACCOUNT_TYPE required") ) val authority = inputData.getString(ARG_AUTHORITY) ?: throw IllegalArgumentException("$ARG_AUTHORITY required") + val expedited = inputData.getBoolean(ARG_EXPEDITED, false) - // Check internet connection + // check internet connection val ignoreVpns = AccountSettings(applicationContext, account).getIgnoreVpns() val connectivityManager = applicationContext.getSystemService()!! if (!internetAvailable(connectivityManager, ignoreVpns)) { @@ -319,7 +332,7 @@ class SyncWorker @AssistedInject constructor( // What are we going to sync? Select syncer based on authority val syncer: Syncer = when (authority) { applicationContext.getString(R.string.address_books_authority) -> - AddressBookSyncer(applicationContext) + AddressBookSyncer(applicationContext, expedited) CalendarContract.AUTHORITY -> CalendarSyncer(applicationContext) ContactsContract.AUTHORITY -> @@ -438,6 +451,7 @@ class SyncWorker @AssistedInject constructor( .setCategory(NotificationCompat.CATEGORY_STATUS) .setOngoing(true) .setPriority(NotificationCompat.PRIORITY_LOW) + .setForegroundServiceBehavior(NotificationCompat.FOREGROUND_SERVICE_DEFERRED) .build() return ForegroundInfo(NotificationUtils.NOTIFY_SYNC_EXPEDITED, notification) } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/account/SettingsActivity.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/account/SettingsActivity.kt index 0c6c7c2e..ab1ce00c 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/ui/account/SettingsActivity.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/ui/account/SettingsActivity.kt @@ -603,7 +603,7 @@ class SettingsActivity: AppCompatActivity() { */ private fun resync(authority: String, fullResync: Boolean) { val resync = if (fullResync) SyncWorker.FULL_RESYNC else SyncWorker.RESYNC - SyncWorker.enqueue(getApplication(), account, authority, resync) + SyncWorker.enqueue(getApplication(), account, authority, expedited = true, resync = resync) } }