From cbade0b54dfc8cc116433bea3c8959f0eb3817b2 Mon Sep 17 00:00:00 2001 From: Profpatsch Date: Thu, 26 Dec 2024 15:04:45 +0100 Subject: [PATCH] PlayerUiList: guard list actions with mutex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new implementation would throw `ConcurrentModificationExceptions` when destroying the UIs. So let’s play it safe and put the list behind a mutex. Adds a helper class `GuardedByMutex` that can be wrapped around a property to force all use-sites to acquire the lock before doing anything with the data. --- .../schabi/newpipe/player/ui/PlayerUiList.kt | 59 +++++++++++++------ .../org/schabi/newpipe/util/GuardedByMutex.kt | 47 +++++++++++++++ 2 files changed, 87 insertions(+), 19 deletions(-) create mode 100644 app/src/main/java/org/schabi/newpipe/util/GuardedByMutex.kt diff --git a/app/src/main/java/org/schabi/newpipe/player/ui/PlayerUiList.kt b/app/src/main/java/org/schabi/newpipe/player/ui/PlayerUiList.kt index 460dfeb0e0f..812f11ec46f 100644 --- a/app/src/main/java/org/schabi/newpipe/player/ui/PlayerUiList.kt +++ b/app/src/main/java/org/schabi/newpipe/player/ui/PlayerUiList.kt @@ -1,10 +1,10 @@ package org.schabi.newpipe.player.ui -import androidx.core.util.Consumer +import org.schabi.newpipe.util.GuardedByMutex import java.util.Optional class PlayerUiList(vararg initialPlayerUis: PlayerUi) { - val playerUis = mutableListOf() + var playerUis = GuardedByMutex(mutableListOf()) /** * Creates a [PlayerUiList] starting with the provided player uis. The provided player uis @@ -16,7 +16,9 @@ class PlayerUiList(vararg initialPlayerUis: PlayerUi) { * @param initialPlayerUis the player uis this list should start with; the order will be kept */ init { - playerUis.addAll(listOf(*initialPlayerUis)) + playerUis.runWithLockSync { + lockData.addAll(listOf(*initialPlayerUis)) + } } /** @@ -41,7 +43,9 @@ class PlayerUiList(vararg initialPlayerUis: PlayerUi) { } } - playerUis.add(playerUi) + playerUis.runWithLockSync { + lockData.add(playerUi) + } } /** @@ -52,12 +56,24 @@ class PlayerUiList(vararg initialPlayerUis: PlayerUi) { * @param T the class type parameter * */ fun destroyAll(playerUiType: Class) { - for (ui in playerUis) { - if (playerUiType.isInstance(ui)) { - ui.destroyPlayer() - ui.destroy() - playerUis.remove(ui) + val toDestroy = mutableListOf() + + // short blocking removal from class to prevent interfering from other threads + playerUis.runWithLockSync { + val new = mutableListOf() + for (ui in lockData) { + if (playerUiType.isInstance(ui)) { + toDestroy.add(ui) + } else { + new.add(ui) + } } + lockData = new + } + // then actually destroy the UIs + for (ui in toDestroy) { + ui.destroyPlayer() + ui.destroy() } } @@ -67,18 +83,19 @@ class PlayerUiList(vararg initialPlayerUis: PlayerUi) { * @param T the class type parameter * @return the first player UI of the required type found in the list, or null */ - fun get(playerUiType: Class): T? { - for (ui in playerUis) { - if (playerUiType.isInstance(ui)) { - when (val r = playerUiType.cast(ui)) { - // try all UIs before returning null - null -> continue - else -> return r + fun get(playerUiType: Class): T? = + playerUis.runWithLockSync { + for (ui in lockData) { + if (playerUiType.isInstance(ui)) { + when (val r = playerUiType.cast(ui)) { + // try all UIs before returning null + null -> continue + else -> return@runWithLockSync r + } } } + return@runWithLockSync null } - return null - } /** * @param playerUiType the class of the player UI to return; @@ -96,7 +113,11 @@ class PlayerUiList(vararg initialPlayerUis: PlayerUi) { * @param consumer the consumer to call with player UIs */ fun call(consumer: java.util.function.Consumer) { - for (ui in playerUis) { + // copy the list out of the mutex before calling the consumer which might block + val new = playerUis.runWithLockSync { + lockData.toMutableList() + } + for (ui in new) { consumer.accept(ui) } } diff --git a/app/src/main/java/org/schabi/newpipe/util/GuardedByMutex.kt b/app/src/main/java/org/schabi/newpipe/util/GuardedByMutex.kt new file mode 100644 index 00000000000..1777a8cc3c8 --- /dev/null +++ b/app/src/main/java/org/schabi/newpipe/util/GuardedByMutex.kt @@ -0,0 +1,47 @@ +package org.schabi.newpipe.util + +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock + +/** Guard the given data so that it can only be accessed by locking the mutex first. + * + * Inspired by [this blog post](https://jonnyzzz.com/blog/2017/03/01/guarded-by-lock/) + * */ +class GuardedByMutex( + private var data: T, + private val lock: Mutex = Mutex(locked = false), +) { + + /** Lock the mutex and access the data, blocking the current thread. + * @param action to run with locked mutex + * */ + fun runWithLockSync( + action: MutexData.() -> Y + ) = + runBlocking { + lock.withLock { + MutexData(data, { d -> data = d }).action() + } + } + + /** Lock the mutex and access the data, suspending the coroutine. + * @param action to run with locked mutex + * */ + suspend fun runWithLock(action: MutexData.() -> Y) = + lock.withLock { + MutexData(data, { d -> data = d }).action() + } +} + +/** The data inside a [GuardedByMutex], which can be accessed via [lockData]. + * [lockData] is a `var`, so you can `set` it as well. + * */ +class MutexData(data: T, val setFun: (T) -> Unit) { + /** The data inside this [GuardedByMutex] */ + var lockData: T = data + set(t) { + setFun(t) + field = t + } +} \ No newline at end of file