From a63b3791d205a17c2c52818e0db4ccdea63624f4 Mon Sep 17 00:00:00 2001 From: OxygenCobalt Date: Sat, 9 Jul 2022 11:15:52 -0600 Subject: [PATCH] util: add guard abstraction Add an abstraction over the generation system called GenerationGuard. This allows cheap cooperative threading to be implemented consistently in many places. --- .../oxycblt/auxio/detail/DetailViewModel.kt | 13 +++--- .../org/oxycblt/auxio/image/BaseFetcher.kt | 2 +- .../org/oxycblt/auxio/image/BitmapProvider.kt | 11 +++-- .../org/oxycblt/auxio/music/system/Indexer.kt | 43 ++++++++----------- .../auxio/music/system/MediaStoreBackend.kt | 3 -- .../org/oxycblt/auxio/util/PrimitiveUtil.kt | 32 ++++++++++++++ 6 files changed, 61 insertions(+), 43 deletions(-) diff --git a/app/src/main/java/org/oxycblt/auxio/detail/DetailViewModel.kt b/app/src/main/java/org/oxycblt/auxio/detail/DetailViewModel.kt index ce0097910..842ac6d99 100644 --- a/app/src/main/java/org/oxycblt/auxio/detail/DetailViewModel.kt +++ b/app/src/main/java/org/oxycblt/auxio/detail/DetailViewModel.kt @@ -39,6 +39,7 @@ import org.oxycblt.auxio.settings.Settings import org.oxycblt.auxio.ui.Sort import org.oxycblt.auxio.ui.recycler.Header import org.oxycblt.auxio.ui.recycler.Item +import org.oxycblt.auxio.util.GenerationGuard import org.oxycblt.auxio.util.application import org.oxycblt.auxio.util.logD import org.oxycblt.auxio.util.logW @@ -112,6 +113,8 @@ class DetailViewModel(application: Application) : currentGenre.value?.let(::refreshGenreData) } + private val songGuard = GenerationGuard() + fun setSongId(id: Long) { if (_currentSong.value?.run { song.id } == id) return val library = unlikelyToBeNull(musicStore.library) @@ -158,14 +161,10 @@ class DetailViewModel(application: Application) : private fun generateDetailSong(song: Song) { _currentSong.value = DetailSong(song, null) viewModelScope.launch(Dispatchers.IO) { + val generation = songGuard.newHandle() val info = generateDetailSongInfo(song) - - // Theoretically, the song could have been changed again while we were - // extracting song information, so make sure that we can update the song - // in the first place. - if (_currentSong.value?.run { this.song.id } == song.id) { - _currentSong.value = DetailSong(song, info) - } + songGuard.yield(generation) + _currentSong.value = DetailSong(song, info) } } diff --git a/app/src/main/java/org/oxycblt/auxio/image/BaseFetcher.kt b/app/src/main/java/org/oxycblt/auxio/image/BaseFetcher.kt index e938f99df..6a020344f 100644 --- a/app/src/main/java/org/oxycblt/auxio/image/BaseFetcher.kt +++ b/app/src/main/java/org/oxycblt/auxio/image/BaseFetcher.kt @@ -53,7 +53,7 @@ import org.oxycblt.auxio.util.logW * The base implementation for all image fetchers in Auxio. * @author OxygenCobalt * - * TODO: Artist images + * TODO: File-system derived images [cover.jpg, Artist Images] */ abstract class BaseFetcher : Fetcher { /** diff --git a/app/src/main/java/org/oxycblt/auxio/image/BitmapProvider.kt b/app/src/main/java/org/oxycblt/auxio/image/BitmapProvider.kt index c81c3f3e5..937bf15ad 100644 --- a/app/src/main/java/org/oxycblt/auxio/image/BitmapProvider.kt +++ b/app/src/main/java/org/oxycblt/auxio/image/BitmapProvider.kt @@ -25,6 +25,7 @@ import coil.request.Disposable import coil.request.ImageRequest import coil.size.Size import org.oxycblt.auxio.music.Song +import org.oxycblt.auxio.util.GenerationGuard /** * A utility to provide bitmaps in a manner less prone to race conditions. @@ -39,7 +40,7 @@ import org.oxycblt.auxio.music.Song */ class BitmapProvider(private val context: Context) { private var currentRequest: Request? = null - private var currentGeneration = 0L + private var guard = GenerationGuard() /** If this provider is currently attempting to load something. */ val isBusy: Boolean @@ -51,9 +52,7 @@ class BitmapProvider(private val context: Context) { */ @Synchronized fun load(song: Song, target: Target) { - // Increment the generation value so that previous requests are invalidated. - // This is a second safeguard to mitigate instruction-by-instruction race conditions. - val generation = ++currentGeneration + val generation = guard.newHandle() currentRequest?.run { disposable.dispose() } currentRequest = null @@ -65,12 +64,12 @@ class BitmapProvider(private val context: Context) { .size(Size.ORIGINAL) .target( onSuccess = { - if (generation == currentGeneration) { + if (guard.check(generation)) { target.onCompleted(it.toBitmap()) } }, onError = { - if (generation == currentGeneration) { + if (guard.check(generation)) { target.onCompleted(null) } }) diff --git a/app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt b/app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt index 128e3444b..41dc994f7 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt @@ -23,7 +23,6 @@ import android.content.pm.PackageManager import android.database.Cursor import android.os.Build import androidx.core.content.ContextCompat -import kotlin.coroutines.cancellation.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import org.oxycblt.auxio.BuildConfig @@ -34,6 +33,7 @@ import org.oxycblt.auxio.music.MusicStore import org.oxycblt.auxio.music.Song import org.oxycblt.auxio.settings.Settings import org.oxycblt.auxio.ui.Sort +import org.oxycblt.auxio.util.GenerationGuard import org.oxycblt.auxio.util.logD import org.oxycblt.auxio.util.logE import org.oxycblt.auxio.util.logW @@ -64,7 +64,7 @@ class Indexer { private var lastResponse: Response? = null private var indexingState: Indexing? = null - private var currentGeneration = 0L + private var guard = GenerationGuard() private var controller: Controller? = null private var callback: Callback? = null @@ -133,7 +133,7 @@ class Indexer { suspend fun index(context: Context) { requireBackgroundThread() - val generation = synchronized(this) { ++currentGeneration } + val generation = guard.newHandle() val notGranted = ContextCompat.checkSelfPermission(context, Manifest.permission.READ_EXTERNAL_STORAGE) == @@ -184,13 +184,13 @@ class Indexer { @Synchronized fun cancelLast() { logD("Cancelling last job") - currentGeneration++ - emitIndexing(null, currentGeneration) + val generation = guard.newHandle() + emitIndexing(null, generation) } @Synchronized private fun emitIndexing(indexing: Indexing?, generation: Long) { - checkGeneration(generation) + guard.yield(generation) if (indexing == indexingState) { // Ignore redundant states used when the backends just want to check for @@ -210,31 +210,22 @@ class Indexer { } private suspend fun emitCompletion(response: Response, generation: Long) { - synchronized(this) { - checkGeneration(generation) - - // Do not check for redundancy here, as we actually need to notify a switch - // from Indexing -> Complete and not Indexing -> Indexing or Complete -> Complete. - - lastResponse = response - indexingState = null - } - - val state = State.Complete(response) + guard.yield(generation) // Swap to the Main thread so that downstream callbacks don't crash from being on // a background thread. Does not occur in emitIndexing due to efficiency reasons. withContext(Dispatchers.Main) { - controller?.onIndexerStateChanged(state) - callback?.onIndexerStateChanged(state) - } - } + synchronized(this) { + // Do not check for redundancy here, as we actually need to notify a switch + // from Indexing -> Complete and not Indexing -> Indexing or Complete -> Complete. + lastResponse = response + indexingState = null - private fun checkGeneration(generation: Long) { - if (currentGeneration != generation) { - // Not the running task anymore, cancel this co-routine. This allows a yield-like - // behavior to be implemented in a far cheaper manner for each backend. - throw CancellationException() + val state = State.Complete(response) + + controller?.onIndexerStateChanged(state) + callback?.onIndexerStateChanged(state) + } } } diff --git a/app/src/main/java/org/oxycblt/auxio/music/system/MediaStoreBackend.kt b/app/src/main/java/org/oxycblt/auxio/music/system/MediaStoreBackend.kt index 2409c91ca..e56815c32 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/system/MediaStoreBackend.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/system/MediaStoreBackend.kt @@ -471,7 +471,6 @@ class Api21MediaStoreBackend : MediaStoreBackend() { open class BaseApi29MediaStoreBackend : MediaStoreBackend() { private var volumeIndex = -1 private var relativePathIndex = -1 - private var dateTakenIndex = -1 override val projection: Array get() = @@ -499,7 +498,6 @@ open class BaseApi29MediaStoreBackend : MediaStoreBackend() { volumeIndex = cursor.getColumnIndexOrThrow(MediaStore.Audio.AudioColumns.VOLUME_NAME) relativePathIndex = cursor.getColumnIndexOrThrow(MediaStore.Audio.AudioColumns.RELATIVE_PATH) - dateTakenIndex = cursor.getColumnIndexOrThrow(MediaStore.Audio.AudioColumns.DATE_TAKEN) } val volumeName = cursor.getString(volumeIndex) @@ -512,7 +510,6 @@ open class BaseApi29MediaStoreBackend : MediaStoreBackend() { audio.dir = Directory(volume, relativePath.removeSuffix(File.separator)) } - return audio } } diff --git a/app/src/main/java/org/oxycblt/auxio/util/PrimitiveUtil.kt b/app/src/main/java/org/oxycblt/auxio/util/PrimitiveUtil.kt index d26ae2b3b..d06a3b1ce 100644 --- a/app/src/main/java/org/oxycblt/auxio/util/PrimitiveUtil.kt +++ b/app/src/main/java/org/oxycblt/auxio/util/PrimitiveUtil.kt @@ -22,6 +22,7 @@ import android.text.format.DateUtils import androidx.core.math.MathUtils import java.lang.reflect.Field import java.lang.reflect.Method +import java.util.concurrent.CancellationException import kotlin.reflect.KClass import org.oxycblt.auxio.BuildConfig @@ -77,3 +78,34 @@ fun lazyReflectedField(clazz: KClass<*>, field: String) = lazy { fun lazyReflectedMethod(clazz: KClass<*>, method: String) = lazy { clazz.java.getDeclaredMethod(method).also { it.isAccessible = true } } + +/** + * A generation-based abstraction that allows cheap cooperative multi-threading in shared object + * contexts. Every new task should call [newHandle], while every running task should call [check] or + * [yield] depending on the context. + * + * @author OxygenCobalt + */ +class GenerationGuard { + private var currentHandle = 0L + + /** + * Returns a new handle to the calling task while invalidating the generations of the previous + * task. + */ + @Synchronized fun newHandle() = ++currentHandle + + /** Check if the given [handle] is still the one represented by this class. */ + @Synchronized fun check(handle: Long) = handle == currentHandle + + /** + * Alternative to [kotlinx.coroutines.yield], that achieves the same behavior but in a much + * cheaper manner. + */ + @Synchronized + fun yield(handle: Long) { + if (!check(handle)) { + throw CancellationException() + } + } +}