From e9ab544b34b2a8d2906d8e11e04d5ee8166dc062 Mon Sep 17 00:00:00 2001 From: Alex Baker Date: Fri, 31 Jul 2020 12:16:15 -0500 Subject: [PATCH] Initiate work requests from background thread --- .../main/java/org/tasks/jobs/WorkManager.kt | 27 ++-- .../java/org/tasks/jobs/WorkManagerImpl.kt | 145 +++++++++--------- .../java/org/tasks/notifications/Throttle.kt | 15 +- .../org/tasks/notifications/ThrottleTest.kt | 5 +- 4 files changed, 102 insertions(+), 90 deletions(-) diff --git a/app/src/main/java/org/tasks/jobs/WorkManager.kt b/app/src/main/java/org/tasks/jobs/WorkManager.kt index 2fbc988cc..6d2dd54a3 100644 --- a/app/src/main/java/org/tasks/jobs/WorkManager.kt +++ b/app/src/main/java/org/tasks/jobs/WorkManager.kt @@ -7,34 +7,35 @@ import org.tasks.data.Place interface WorkManager { - suspend fun afterComplete(task: Task) + fun afterComplete(task: Task) - suspend fun cleanup(ids: Iterable) + fun cleanup(ids: Iterable) - suspend fun googleTaskSync(immediate: Boolean) + fun googleTaskSync(immediate: Boolean) - suspend fun caldavSync(immediate: Boolean) + fun caldavSync(immediate: Boolean) - suspend fun eteSync(immediate: Boolean) + fun eteSync(immediate: Boolean) - suspend fun reverseGeocode(place: Place) - suspend fun updateBackgroundSync() + fun reverseGeocode(place: Place) - suspend fun updateBackgroundSync( + fun updateBackgroundSync() + + fun updateBackgroundSync( forceBackgroundEnabled: Boolean?, forceOnlyOnUnmetered: Boolean?) - suspend fun scheduleRefresh(time: Long) + fun scheduleRefresh(time: Long) - suspend fun scheduleMidnightRefresh() + fun scheduleMidnightRefresh() fun scheduleNotification(scheduledTime: Long) - suspend fun scheduleBackup() + fun scheduleBackup() - suspend fun scheduleConfigRefresh() + fun scheduleConfigRefresh() - suspend fun scheduleDriveUpload(uri: Uri, purge: Boolean) + fun scheduleDriveUpload(uri: Uri, purge: Boolean) fun cancelNotifications() diff --git a/app/src/main/java/org/tasks/jobs/WorkManagerImpl.kt b/app/src/main/java/org/tasks/jobs/WorkManagerImpl.kt index b948adb1b..5d08ed264 100644 --- a/app/src/main/java/org/tasks/jobs/WorkManagerImpl.kt +++ b/app/src/main/java/org/tasks/jobs/WorkManagerImpl.kt @@ -19,7 +19,6 @@ import org.tasks.data.GoogleTaskListDao import org.tasks.data.Place import org.tasks.date.DateTimeUtils.midnight import org.tasks.date.DateTimeUtils.newDateTime -import org.tasks.db.SuspendDbUtils.eachChunk import org.tasks.jobs.WorkManager.Companion.MAX_CLEANUP_LENGTH import org.tasks.jobs.WorkManager.Companion.REMOTE_CONFIG_INTERVAL_HOURS import org.tasks.jobs.WorkManager.Companion.TAG_BACKGROUND_SYNC_CALDAV @@ -49,19 +48,7 @@ class WorkManagerImpl constructor( private val alarmManager: AlarmManager = context.getSystemService(Context.ALARM_SERVICE) as AlarmManager private val workManager = androidx.work.WorkManager.getInstance(context) - private suspend fun enqueue(builder: WorkRequest.Builder<*, *>) { - throttle.run { - workManager.enqueue(builder.build()) - } - } - - private suspend fun enqueue(continuation: WorkContinuation) { - throttle.run { - continuation.enqueue() - } - } - - override suspend fun afterComplete(task: Task) { + override fun afterComplete(task: Task) { enqueue( OneTimeWorkRequest.Builder(AfterSaveWork::class.java) .setInputData(Data.Builder() @@ -69,8 +56,8 @@ class WorkManagerImpl constructor( .build())) } - override suspend fun cleanup(ids: Iterable) { - ids.eachChunk(MAX_CLEANUP_LENGTH) { + override fun cleanup(ids: Iterable) { + ids.chunked(MAX_CLEANUP_LENGTH) { enqueue( OneTimeWorkRequest.Builder(CleanupWork::class.java) .setInputData( @@ -80,36 +67,36 @@ class WorkManagerImpl constructor( } } - override suspend fun googleTaskSync(immediate: Boolean) = + override fun googleTaskSync(immediate: Boolean) = sync(immediate, TAG_SYNC_GOOGLE_TASKS, SyncGoogleTasksWork::class.java) - override suspend fun caldavSync(immediate: Boolean) = + override fun caldavSync(immediate: Boolean) = sync(immediate, TAG_SYNC_CALDAV, SyncCaldavWork::class.java) - override suspend fun eteSync(immediate: Boolean) = + override fun eteSync(immediate: Boolean) = sync(immediate, TAG_SYNC_ETESYNC, SyncEteSyncWork::class.java) - private suspend fun sync(immediate: Boolean, tag: String, c: Class) { - val constraints = Constraints.Builder() - .setRequiredNetworkType( - if (!immediate && preferences.getBoolean(R.string.p_background_sync_unmetered_only, false)) { - NetworkType.UNMETERED - } else { - NetworkType.CONNECTED - }) - .build() - val builder = OneTimeWorkRequest.Builder(c).setConstraints(constraints) + @SuppressLint("EnqueueWork") + private fun sync(immediate: Boolean, tag: String, c: Class, requireNetwork: Boolean = true) { + Timber.d("sync(immediate = $immediate, $tag, $c, requireNetwork = $requireNetwork)") + val builder = OneTimeWorkRequest.Builder(c) + if (requireNetwork) { + builder.setConstraints(Constraints.Builder() + .setRequiredNetworkType( + if (!immediate && preferences.getBoolean(R.string.p_background_sync_unmetered_only, false)) { + NetworkType.UNMETERED + } else { + NetworkType.CONNECTED + }) + .build()) + } if (!immediate) { builder.setInitialDelay(1, TimeUnit.MINUTES) } - throttle.run { - workManager - .beginUniqueWork(tag, ExistingWorkPolicy.REPLACE, builder.build()) - .enqueue() - } + enqueue(workManager.beginUniqueWork(tag, ExistingWorkPolicy.REPLACE, builder.build())) } - override suspend fun reverseGeocode(place: Place) { + override fun reverseGeocode(place: Place) { if (BuildConfig.DEBUG && place.id == 0L) { throw RuntimeException("Missing id") } @@ -120,53 +107,56 @@ class WorkManagerImpl constructor( Constraints.Builder().setRequiredNetworkType(NetworkType.CONNECTED).build())) } - override suspend fun updateBackgroundSync() { + override fun updateBackgroundSync() { updateBackgroundSync(null, null) } @SuppressLint("CheckResult") - override suspend fun updateBackgroundSync( + override fun updateBackgroundSync( forceBackgroundEnabled: Boolean?, forceOnlyOnUnmetered: Boolean?) { val enabled = forceBackgroundEnabled ?: preferences.getBoolean(R.string.p_background_sync, true) val unmetered = forceOnlyOnUnmetered ?: preferences.getBoolean(R.string.p_background_sync_unmetered_only, false) - - scheduleBackgroundSync( - TAG_BACKGROUND_SYNC_GOOGLE_TASKS, - SyncGoogleTasksWork::class.java, - enabled && googleTaskListDao.accountCount() > 0, - unmetered) - scheduleBackgroundSync( - TAG_BACKGROUND_SYNC_CALDAV, - SyncCaldavWork::class.java, - enabled && caldavDao.getAccounts(TYPE_CALDAV).isNotEmpty(), - unmetered) - scheduleBackgroundSync( - TAG_BACKGROUND_SYNC_ETESYNC, - SyncEteSyncWork::class.java, - enabled && caldavDao.getAccounts(TYPE_ETESYNC).isNotEmpty(), - unmetered) + throttle.run { + scheduleBackgroundSync( + TAG_BACKGROUND_SYNC_GOOGLE_TASKS, + SyncGoogleTasksWork::class.java, + enabled && googleTaskListDao.accountCount() > 0, + unmetered) + } + throttle.run { + scheduleBackgroundSync( + TAG_BACKGROUND_SYNC_CALDAV, + SyncCaldavWork::class.java, + enabled && caldavDao.getAccounts(TYPE_CALDAV).isNotEmpty(), + unmetered) + } + throttle.run { + scheduleBackgroundSync( + TAG_BACKGROUND_SYNC_ETESYNC, + SyncEteSyncWork::class.java, + enabled && caldavDao.getAccounts(TYPE_ETESYNC).isNotEmpty(), + unmetered) + } } - private suspend fun scheduleBackgroundSync( + private fun scheduleBackgroundSync( tag: String, c: Class, enabled: Boolean, unmetered: Boolean? = null) { Timber.d("scheduleBackgroundSync($tag, $c, enabled = $enabled, unmetered = $unmetered)") - throttle.run { - if (enabled) { - val builder = PeriodicWorkRequest.Builder(c, 1, TimeUnit.HOURS) - unmetered?.let { builder.setConstraints(getNetworkConstraints(it)) } - workManager.enqueueUniquePeriodicWork( - tag, ExistingPeriodicWorkPolicy.KEEP, builder.build()) - } else { - workManager.cancelUniqueWork(tag) - } + if (enabled) { + val builder = PeriodicWorkRequest.Builder(c, 1, TimeUnit.HOURS) + unmetered?.let { builder.setConstraints(getNetworkConstraints(it)) } + workManager.enqueueUniquePeriodicWork( + tag, ExistingPeriodicWorkPolicy.KEEP, builder.build()) + } else { + workManager.cancelUniqueWork(tag) } } - override suspend fun scheduleRefresh(time: Long) = enqueueUnique(TAG_REFRESH, RefreshWork::class.java, time) + override fun scheduleRefresh(time: Long) = enqueueUnique(TAG_REFRESH, RefreshWork::class.java, time) - override suspend fun scheduleMidnightRefresh() = + override fun scheduleMidnightRefresh() = enqueueUnique(TAG_MIDNIGHT_REFRESH, MidnightRefreshWork::class.java, midnight()) override fun scheduleNotification(scheduledTime: Long) { @@ -184,7 +174,7 @@ class WorkManagerImpl constructor( } } - override suspend fun scheduleBackup() = + override fun scheduleBackup() = enqueueUnique( TAG_BACKUP, BackupWork::class.java, @@ -193,7 +183,7 @@ class WorkManagerImpl constructor( .millis .coerceAtMost(midnight())) - override suspend fun scheduleConfigRefresh() { + override fun scheduleConfigRefresh() { throttle.run { workManager.enqueueUniquePeriodicWork( TAG_REMOTE_CONFIG, @@ -206,7 +196,7 @@ class WorkManagerImpl constructor( } } - override suspend fun scheduleDriveUpload(uri: Uri, purge: Boolean) { + override fun scheduleDriveUpload(uri: Uri, purge: Boolean) { if (!preferences.getBoolean(R.string.p_google_drive_backup, false)) { return } @@ -228,8 +218,12 @@ class WorkManagerImpl constructor( .setRequiredNetworkType(if (unmeteredOnly) NetworkType.UNMETERED else NetworkType.CONNECTED) .build() + override fun cancelNotifications() { + alarmManager.cancel(notificationPendingIntent) + } + @SuppressLint("EnqueueWork") - private suspend fun enqueueUnique(key: String, c: Class, time: Long) { + private fun enqueueUnique(key: String, c: Class, time: Long) { val delay = time - DateUtilities.now() val builder = OneTimeWorkRequest.Builder(c) if (delay > 0) { @@ -239,9 +233,16 @@ class WorkManagerImpl constructor( enqueue(workManager.beginUniqueWork(key, ExistingWorkPolicy.REPLACE, builder.build())) } - override fun cancelNotifications() { - Timber.d("cancelNotifications") - alarmManager.cancel(notificationPendingIntent) + private fun enqueue(builder: WorkRequest.Builder<*, *>) { + throttle.run { + workManager.enqueue(builder.build()) + } + } + + private fun enqueue(continuation: WorkContinuation) { + throttle.run { + continuation.enqueue() + } } private val notificationIntent: Intent diff --git a/app/src/main/java/org/tasks/notifications/Throttle.kt b/app/src/main/java/org/tasks/notifications/Throttle.kt index 77aed5ca2..8ae75c74a 100644 --- a/app/src/main/java/org/tasks/notifications/Throttle.kt +++ b/app/src/main/java/org/tasks/notifications/Throttle.kt @@ -1,25 +1,32 @@ package org.tasks.notifications -import kotlinx.coroutines.delay +import kotlinx.coroutines.* import org.tasks.time.DateTimeUtils.currentTimeMillis import timber.log.Timber +import java.util.concurrent.Executors.newSingleThreadExecutor internal class Throttle constructor( ratePerPeriod: Int, private val periodMillis: Long = 1000, private val tag: String = "", + private val scope: CoroutineScope = + CoroutineScope(newSingleThreadExecutor().asCoroutineDispatcher() + SupervisorJob()), private val sleeper: suspend (Long) -> Unit = { delay(it) }) { private val throttle: LongArray = LongArray(ratePerPeriod) private var oldest = 0 @Synchronized - suspend fun run(runnable: suspend () -> Unit) { + fun run(runnable: suspend () -> Unit): Job = scope.launch { val sleep = throttle[oldest] - (currentTimeMillis() - periodMillis) if (sleep > 0) { - Timber.d("$tag: Throttled for ${sleep}ms") + Timber.v("$tag: Throttled for ${sleep}ms") sleeper.invoke(sleep) } - runnable.invoke() + try { + runnable.invoke() + } catch (e: Exception) { + Timber.e(e) + } throttle[oldest] = currentTimeMillis() oldest = (oldest + 1) % throttle.size } diff --git a/app/src/test/java/org/tasks/notifications/ThrottleTest.kt b/app/src/test/java/org/tasks/notifications/ThrottleTest.kt index ce8f72527..bfea969e5 100644 --- a/app/src/test/java/org/tasks/notifications/ThrottleTest.kt +++ b/app/src/test/java/org/tasks/notifications/ThrottleTest.kt @@ -17,11 +17,11 @@ class ThrottleTest { @Before fun setUp() { sleep = ArrayList() - throttle = Throttle(3) { sleep.add(it) } } @Test fun dontThrottle() = runBlockingTest { + throttle = Throttle(3, scope = this) { sleep.add(it) } val now = DateTimeUtils.currentTimeMillis() runAt(now) runAt(now) @@ -32,6 +32,7 @@ class ThrottleTest { @Test fun throttleForOneMillisecond() = runBlockingTest { + throttle = Throttle(3, scope = this) { sleep.add(it) } val now = DateTimeUtils.currentTimeMillis() runAt(now) runAt(now) @@ -42,6 +43,7 @@ class ThrottleTest { @Test fun throttleForOneSecond() = runBlockingTest { + throttle = Throttle(3, scope = this) { sleep.add(it) } val now = DateTimeUtils.currentTimeMillis() runAt(now) runAt(now) @@ -52,6 +54,7 @@ class ThrottleTest { @Test fun throttleMultiple() = runBlockingTest { + throttle = Throttle(3, scope = this) { sleep.add(it) } val now = DateTimeUtils.currentTimeMillis() runAt(now) runAt(now + 200)