From 24e1b4034efe24cc042404e2237f01ca1f2cd9b3 Mon Sep 17 00:00:00 2001 From: Ivan Iskandar <12537387+ivaniskandar@users.noreply.github.com> Date: Sat, 2 Dec 2023 23:46:59 +0700 Subject: [PATCH] Move workers to foreground service context a bit more safely (#10202) The system will crash the app if the worker that calls setForeground() finished before the service runner be able to call Service.startForeground(). This edge case is not handled by WorkManager and there is no way to check if the required calls are done. So here we suspend the worker by an arbitrary duration assuming the transition to foreground service is done by then. --- .../tachiyomi/data/backup/BackupCreateJob.kt | 12 ++++------ .../tachiyomi/data/backup/BackupRestoreJob.kt | 7 ++---- .../tachiyomi/data/download/DownloadJob.kt | 18 ++++++--------- .../data/library/LibraryUpdateJob.kt | 7 ++---- .../data/library/MetadataUpdateJob.kt | 7 ++---- .../data/updater/AppUpdateDownloadJob.kt | 9 ++------ .../util/system/WorkManagerExtensions.kt | 22 +++++++++++++++++++ 7 files changed, 41 insertions(+), 41 deletions(-) diff --git a/app/src/main/java/eu/kanade/tachiyomi/data/backup/BackupCreateJob.kt b/app/src/main/java/eu/kanade/tachiyomi/data/backup/BackupCreateJob.kt index bc37b8c75..f6fe367f3 100644 --- a/app/src/main/java/eu/kanade/tachiyomi/data/backup/BackupCreateJob.kt +++ b/app/src/main/java/eu/kanade/tachiyomi/data/backup/BackupCreateJob.kt @@ -17,6 +17,7 @@ import com.hippo.unifile.UniFile import eu.kanade.tachiyomi.data.notification.Notifications import eu.kanade.tachiyomi.util.system.cancelNotification import eu.kanade.tachiyomi.util.system.isRunning +import eu.kanade.tachiyomi.util.system.setForegroundSafely import eu.kanade.tachiyomi.util.system.workManager import logcat.LogPriority import tachiyomi.core.util.system.logcat @@ -39,19 +40,14 @@ class BackupCreateJob(private val context: Context, workerParams: WorkerParamete if (isAutoBackup && BackupRestoreJob.isRunning(context)) return Result.retry() - val backupPreferences = Injekt.get() - val uri = inputData.getString(LOCATION_URI_KEY)?.toUri() ?: getAutomaticBackupLocation() ?: return Result.failure() - val flags = inputData.getInt(BACKUP_FLAGS_KEY, BackupCreateFlags.AutomaticDefaults) + setForegroundSafely() - try { - setForeground(getForegroundInfo()) - } catch (e: IllegalStateException) { - logcat(LogPriority.ERROR, e) { "Not allowed to run on foreground service" } - } + val flags = inputData.getInt(BACKUP_FLAGS_KEY, BackupCreateFlags.AutomaticDefaults) + val backupPreferences = Injekt.get() return try { val location = BackupCreator(context).createBackup(uri, flags, isAutoBackup) diff --git a/app/src/main/java/eu/kanade/tachiyomi/data/backup/BackupRestoreJob.kt b/app/src/main/java/eu/kanade/tachiyomi/data/backup/BackupRestoreJob.kt index f9b41c330..e4595a4b5 100644 --- a/app/src/main/java/eu/kanade/tachiyomi/data/backup/BackupRestoreJob.kt +++ b/app/src/main/java/eu/kanade/tachiyomi/data/backup/BackupRestoreJob.kt @@ -12,6 +12,7 @@ import androidx.work.workDataOf import eu.kanade.tachiyomi.data.notification.Notifications import eu.kanade.tachiyomi.util.system.cancelNotification import eu.kanade.tachiyomi.util.system.isRunning +import eu.kanade.tachiyomi.util.system.setForegroundSafely import eu.kanade.tachiyomi.util.system.workManager import kotlinx.coroutines.CancellationException import logcat.LogPriority @@ -29,11 +30,7 @@ class BackupRestoreJob(private val context: Context, workerParams: WorkerParamet ?: return Result.failure() val sync = inputData.getBoolean(SYNC_KEY, false) - try { - setForeground(getForegroundInfo()) - } catch (e: IllegalStateException) { - logcat(LogPriority.ERROR, e) { "Not allowed to run on foreground service" } - } + setForegroundSafely() return try { val restorer = BackupRestorer(context, notifier) diff --git a/app/src/main/java/eu/kanade/tachiyomi/data/download/DownloadJob.kt b/app/src/main/java/eu/kanade/tachiyomi/data/download/DownloadJob.kt index d0d58a843..93826afe0 100644 --- a/app/src/main/java/eu/kanade/tachiyomi/data/download/DownloadJob.kt +++ b/app/src/main/java/eu/kanade/tachiyomi/data/download/DownloadJob.kt @@ -16,11 +16,10 @@ import eu.kanade.tachiyomi.data.notification.Notifications import eu.kanade.tachiyomi.util.system.isConnectedToWifi import eu.kanade.tachiyomi.util.system.isOnline import eu.kanade.tachiyomi.util.system.notificationBuilder +import eu.kanade.tachiyomi.util.system.setForegroundSafely import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map -import logcat.LogPriority -import tachiyomi.core.util.system.logcat import tachiyomi.domain.download.service.DownloadPreferences import uy.kohesive.injekt.Injekt import uy.kohesive.injekt.api.get @@ -51,21 +50,18 @@ class DownloadJob(context: Context, workerParams: WorkerParameters) : CoroutineW } override suspend fun doWork(): Result { - try { - setForeground(getForegroundInfo()) - } catch (e: IllegalStateException) { - logcat(LogPriority.ERROR, e) { "Not allowed to set foreground job" } + var active = checkConnectivity() && downloadManager.downloaderStart() + + if (!active) { + return Result.failure() } - var networkCheck = checkConnectivity() - var active = networkCheck - downloadManager.downloaderStart() + setForegroundSafely() // Keep the worker running when needed while (active) { delay(100) - networkCheck = checkConnectivity() - active = !isStopped && networkCheck && downloadManager.isRunning + active = !isStopped && downloadManager.isRunning && checkConnectivity() } return Result.success() diff --git a/app/src/main/java/eu/kanade/tachiyomi/data/library/LibraryUpdateJob.kt b/app/src/main/java/eu/kanade/tachiyomi/data/library/LibraryUpdateJob.kt index 2df3b804e..d733f176e 100644 --- a/app/src/main/java/eu/kanade/tachiyomi/data/library/LibraryUpdateJob.kt +++ b/app/src/main/java/eu/kanade/tachiyomi/data/library/LibraryUpdateJob.kt @@ -30,6 +30,7 @@ import eu.kanade.tachiyomi.util.storage.getUriCompat import eu.kanade.tachiyomi.util.system.createFileInCacheDir import eu.kanade.tachiyomi.util.system.isConnectedToWifi import eu.kanade.tachiyomi.util.system.isRunning +import eu.kanade.tachiyomi.util.system.setForegroundSafely import eu.kanade.tachiyomi.util.system.workManager import kotlinx.coroutines.CancellationException import kotlinx.coroutines.async @@ -108,11 +109,7 @@ class LibraryUpdateJob(private val context: Context, workerParams: WorkerParamet } } - try { - setForeground(getForegroundInfo()) - } catch (e: IllegalStateException) { - logcat(LogPriority.ERROR, e) { "Not allowed to set foreground job" } - } + setForegroundSafely() libraryPreferences.lastUpdatedTimestamp().set(Date().time) diff --git a/app/src/main/java/eu/kanade/tachiyomi/data/library/MetadataUpdateJob.kt b/app/src/main/java/eu/kanade/tachiyomi/data/library/MetadataUpdateJob.kt index 61178bc19..fc8de7617 100644 --- a/app/src/main/java/eu/kanade/tachiyomi/data/library/MetadataUpdateJob.kt +++ b/app/src/main/java/eu/kanade/tachiyomi/data/library/MetadataUpdateJob.kt @@ -18,6 +18,7 @@ import eu.kanade.tachiyomi.data.notification.Notifications import eu.kanade.tachiyomi.source.UnmeteredSource import eu.kanade.tachiyomi.util.prepUpdateCover import eu.kanade.tachiyomi.util.system.isRunning +import eu.kanade.tachiyomi.util.system.setForegroundSafely import eu.kanade.tachiyomi.util.system.workManager import kotlinx.coroutines.CancellationException import kotlinx.coroutines.async @@ -53,11 +54,7 @@ class MetadataUpdateJob(private val context: Context, workerParams: WorkerParame private var mangaToUpdate: List = mutableListOf() override suspend fun doWork(): Result { - try { - setForeground(getForegroundInfo()) - } catch (e: IllegalStateException) { - logcat(LogPriority.ERROR, e) { "Not allowed to set foreground job" } - } + setForegroundSafely() addMangaToQueue() diff --git a/app/src/main/java/eu/kanade/tachiyomi/data/updater/AppUpdateDownloadJob.kt b/app/src/main/java/eu/kanade/tachiyomi/data/updater/AppUpdateDownloadJob.kt index 581f15a12..a242485c4 100644 --- a/app/src/main/java/eu/kanade/tachiyomi/data/updater/AppUpdateDownloadJob.kt +++ b/app/src/main/java/eu/kanade/tachiyomi/data/updater/AppUpdateDownloadJob.kt @@ -17,13 +17,12 @@ import eu.kanade.tachiyomi.network.await import eu.kanade.tachiyomi.network.newCachelessCallWithProgress import eu.kanade.tachiyomi.util.storage.getUriCompat import eu.kanade.tachiyomi.util.storage.saveTo +import eu.kanade.tachiyomi.util.system.setForegroundSafely import eu.kanade.tachiyomi.util.system.workManager -import logcat.LogPriority import okhttp3.internal.http2.ErrorCode import okhttp3.internal.http2.StreamResetException import tachiyomi.core.i18n.stringResource import tachiyomi.core.util.lang.withIOContext -import tachiyomi.core.util.system.logcat import tachiyomi.i18n.MR import uy.kohesive.injekt.injectLazy import java.io.File @@ -43,11 +42,7 @@ class AppUpdateDownloadJob(private val context: Context, workerParams: WorkerPar return Result.failure() } - try { - setForeground(getForegroundInfo()) - } catch (e: IllegalStateException) { - logcat(LogPriority.ERROR, e) { "Not allowed to run on foreground service" } - } + setForegroundSafely() withIOContext { downloadApk(title, url) diff --git a/app/src/main/java/eu/kanade/tachiyomi/util/system/WorkManagerExtensions.kt b/app/src/main/java/eu/kanade/tachiyomi/util/system/WorkManagerExtensions.kt index 02d721725..e565921db 100644 --- a/app/src/main/java/eu/kanade/tachiyomi/util/system/WorkManagerExtensions.kt +++ b/app/src/main/java/eu/kanade/tachiyomi/util/system/WorkManagerExtensions.kt @@ -1,8 +1,12 @@ package eu.kanade.tachiyomi.util.system import android.content.Context +import androidx.work.CoroutineWorker import androidx.work.WorkInfo import androidx.work.WorkManager +import kotlinx.coroutines.delay +import logcat.LogPriority +import tachiyomi.core.util.system.logcat val Context.workManager: WorkManager get() = WorkManager.getInstance(this) @@ -11,3 +15,21 @@ fun WorkManager.isRunning(tag: String): Boolean { val list = this.getWorkInfosByTag(tag).get() return list.any { it.state == WorkInfo.State.RUNNING } } + +/** + * Makes this worker run in the context of a foreground service. + * + * Note that this function is a no-op if the process is subject to foreground + * service restrictions. + * + * Moving to foreground service context requires the worker to run a bit longer, + * allowing Service.startForeground() to be called and avoiding system crash. + */ +suspend fun CoroutineWorker.setForegroundSafely() { + try { + setForeground(getForegroundInfo()) + delay(500) + } catch (e: IllegalStateException) { + logcat(LogPriority.ERROR, e) { "Not allowed to set foreground job" } + } +}