all: audit synchronized usages
Audit usages of Synchronized throughout the app to prevent deadlocks. This is primarily composed of not making long-running work synchronized, instead only making mutations and reads synchronized. This does cause minor issues, such as during a sanitization event the playback state could be feasibly saved, changed, and then restored back to the previous state unintentionally. However, preventing deadlocks is generally better than trying to fix those.
This commit is contained in:
parent
3bdf7b136e
commit
cd00950a5c
5 changed files with 39 additions and 48 deletions
|
@ -47,8 +47,6 @@ import org.oxycblt.auxio.util.logD
|
||||||
*
|
*
|
||||||
* TODO: Add file observing
|
* TODO: Add file observing
|
||||||
*
|
*
|
||||||
* TODO: Audit usages of synchronized
|
|
||||||
*
|
|
||||||
* TODO: Rework UI flow once again
|
* TODO: Rework UI flow once again
|
||||||
*/
|
*/
|
||||||
class IndexerService : Service(), Indexer.Controller, Settings.Callback {
|
class IndexerService : Service(), Indexer.Controller, Settings.Callback {
|
||||||
|
|
|
@ -35,12 +35,14 @@ class MusicStore private constructor() {
|
||||||
private set
|
private set
|
||||||
|
|
||||||
/** Add a callback to this instance. Make sure to remove it when done. */
|
/** Add a callback to this instance. Make sure to remove it when done. */
|
||||||
|
@Synchronized
|
||||||
fun addCallback(callback: Callback) {
|
fun addCallback(callback: Callback) {
|
||||||
callback.onLibraryChanged(library)
|
callback.onLibraryChanged(library)
|
||||||
callbacks.add(callback)
|
callbacks.add(callback)
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Remove a callback from this instance. */
|
/** Remove a callback from this instance. */
|
||||||
|
@Synchronized
|
||||||
fun removeCallback(callback: Callback) {
|
fun removeCallback(callback: Callback) {
|
||||||
callbacks.remove(callback)
|
callbacks.remove(callback)
|
||||||
}
|
}
|
||||||
|
@ -82,7 +84,6 @@ class MusicStore private constructor() {
|
||||||
}
|
}
|
||||||
|
|
||||||
fun sanitize(song: Song) = songs.find { it.id == song.id }
|
fun sanitize(song: Song) = songs.find { it.id == song.id }
|
||||||
fun sanitize(songs: List<Song>) = songs.mapNotNull { sanitize(it) }
|
|
||||||
fun sanitize(album: Album) = albums.find { it.id == album.id }
|
fun sanitize(album: Album) = albums.find { it.id == album.id }
|
||||||
fun sanitize(artist: Artist) = artists.find { it.id == artist.id }
|
fun sanitize(artist: Artist) = artists.find { it.id == artist.id }
|
||||||
fun sanitize(genre: Genre) = genres.find { it.id == genre.id }
|
fun sanitize(genre: Genre) = genres.find { it.id == genre.id }
|
||||||
|
|
|
@ -101,7 +101,6 @@ class PlaybackStateDatabase(context: Context) :
|
||||||
|
|
||||||
// --- INTERFACE FUNCTIONS ---
|
// --- INTERFACE FUNCTIONS ---
|
||||||
|
|
||||||
@Synchronized
|
|
||||||
fun read(library: MusicStore.Library): SavedState? {
|
fun read(library: MusicStore.Library): SavedState? {
|
||||||
requireBackgroundThread()
|
requireBackgroundThread()
|
||||||
|
|
||||||
|
@ -186,7 +185,6 @@ class PlaybackStateDatabase(context: Context) :
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Clear the previously written [SavedState] and write a new one. */
|
/** Clear the previously written [SavedState] and write a new one. */
|
||||||
@Synchronized
|
|
||||||
fun write(state: SavedState) {
|
fun write(state: SavedState) {
|
||||||
requireBackgroundThread()
|
requireBackgroundThread()
|
||||||
|
|
||||||
|
|
|
@ -360,48 +360,59 @@ class PlaybackStateManager private constructor() {
|
||||||
/** Restore the state from the [database] */
|
/** Restore the state from the [database] */
|
||||||
suspend fun restoreState(database: PlaybackStateDatabase) {
|
suspend fun restoreState(database: PlaybackStateDatabase) {
|
||||||
val library = musicStore.library ?: return
|
val library = musicStore.library ?: return
|
||||||
withContext(Dispatchers.IO) { readImpl(database, library) }?.let(::restoreImpl)
|
val state = withContext(Dispatchers.IO) { database.read(library) }
|
||||||
isInitialized = true
|
|
||||||
|
synchronized(this) {
|
||||||
|
if (state != null) {
|
||||||
|
applyStateImpl(state)
|
||||||
|
}
|
||||||
|
|
||||||
|
isInitialized = true
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Save the current state to the [database]. */
|
/** Save the current state to the [database]. */
|
||||||
suspend fun saveState(database: PlaybackStateDatabase) {
|
suspend fun saveState(database: PlaybackStateDatabase) {
|
||||||
logD("Saving state to DB")
|
logD("Saving state to DB")
|
||||||
|
|
||||||
// Pack the entire state and save it to the database.
|
val state = synchronized(this) { makeStateImpl() }
|
||||||
withContext(Dispatchers.IO) { saveImpl(database) }
|
|
||||||
|
withContext(Dispatchers.IO) { database.write(state) }
|
||||||
}
|
}
|
||||||
|
|
||||||
suspend fun sanitize(database: PlaybackStateDatabase, newLibrary: MusicStore.Library) {
|
suspend fun sanitize(database: PlaybackStateDatabase, newLibrary: MusicStore.Library) {
|
||||||
// Since we need to sanitize the state and re-save it for consistency, take the
|
// Since we need to sanitize the state and re-save it for consistency, take the
|
||||||
// easy way out and just write a new state and restore from it. Don't really care.
|
// easy way out and just write a new state and restore from it. Don't really care.
|
||||||
logD("Sanitizing state")
|
logD("Sanitizing state")
|
||||||
isPlaying = false
|
|
||||||
val state =
|
val state =
|
||||||
withContext(Dispatchers.IO) {
|
synchronized(this) {
|
||||||
saveImpl(database)
|
isPlaying = false
|
||||||
readImpl(database, newLibrary)
|
makeStateImpl()
|
||||||
}
|
}
|
||||||
|
|
||||||
state?.let(::restoreImpl)
|
val sanitizedState =
|
||||||
|
withContext(Dispatchers.IO) {
|
||||||
|
database.write(state)
|
||||||
|
database.read(newLibrary)
|
||||||
|
}
|
||||||
|
|
||||||
|
synchronized(this) {
|
||||||
|
if (sanitizedState != null) {
|
||||||
|
applyStateImpl(state)
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private fun readImpl(
|
private fun makeStateImpl() =
|
||||||
database: PlaybackStateDatabase,
|
PlaybackStateDatabase.SavedState(
|
||||||
library: MusicStore.Library
|
index = index,
|
||||||
): PlaybackStateDatabase.SavedState? {
|
parent = parent,
|
||||||
logD("Getting state from DB")
|
queue = _queue,
|
||||||
|
positionMs = positionMs,
|
||||||
|
isShuffled = isShuffled,
|
||||||
|
repeatMode = repeatMode)
|
||||||
|
|
||||||
val start = System.currentTimeMillis()
|
private fun applyStateImpl(state: PlaybackStateDatabase.SavedState) {
|
||||||
val state = database.read(library)
|
|
||||||
|
|
||||||
logD("State read completed successfully in ${System.currentTimeMillis() - start}ms")
|
|
||||||
|
|
||||||
return state
|
|
||||||
}
|
|
||||||
|
|
||||||
@Synchronized
|
|
||||||
private fun restoreImpl(state: PlaybackStateDatabase.SavedState) {
|
|
||||||
index = state.index
|
index = state.index
|
||||||
parent = state.parent
|
parent = state.parent
|
||||||
_queue = state.queue.toMutableList()
|
_queue = state.queue.toMutableList()
|
||||||
|
@ -414,23 +425,6 @@ class PlaybackStateManager private constructor() {
|
||||||
notifyShuffledChanged()
|
notifyShuffledChanged()
|
||||||
}
|
}
|
||||||
|
|
||||||
@Synchronized
|
|
||||||
private fun saveImpl(database: PlaybackStateDatabase) {
|
|
||||||
val start = System.currentTimeMillis()
|
|
||||||
|
|
||||||
database.write(
|
|
||||||
PlaybackStateDatabase.SavedState(
|
|
||||||
index = index,
|
|
||||||
parent = parent,
|
|
||||||
queue = _queue,
|
|
||||||
positionMs = positionMs,
|
|
||||||
isShuffled = isShuffled,
|
|
||||||
repeatMode = repeatMode))
|
|
||||||
|
|
||||||
this@PlaybackStateManager.logD(
|
|
||||||
"State save completed successfully in ${System.currentTimeMillis() - start}ms")
|
|
||||||
}
|
|
||||||
|
|
||||||
// --- CALLBACKS ---
|
// --- CALLBACKS ---
|
||||||
|
|
||||||
private fun notifyIndexMoved() {
|
private fun notifyIndexMoved() {
|
||||||
|
|
|
@ -236,8 +236,8 @@ val AndroidViewModel.application: Application
|
||||||
fun <R> SQLiteDatabase.queryAll(tableName: String, block: (Cursor) -> R) =
|
fun <R> SQLiteDatabase.queryAll(tableName: String, block: (Cursor) -> R) =
|
||||||
query(tableName, null, null, null, null, null, null)?.use(block)
|
query(tableName, null, null, null, null, null, null)?.use(block)
|
||||||
|
|
||||||
// Note: ViewCompat.setOnApplyWindowInsets is a horrible buggy mess, so we use the native method
|
// Note: WindowInsetsCompat and it's related methods are a non-functional mess that does not
|
||||||
// and convert the insets as needed to their compat forms.
|
// work for Auxio's use-case. Use our own methods instead.
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Resolve system bar insets in a version-aware manner. This can be used to apply padding to a view
|
* Resolve system bar insets in a version-aware manner. This can be used to apply padding to a view
|
||||||
|
|
Loading…
Reference in a new issue