Fix potential deadlock in snapshot list and map
Adding content to a SnapshotStateList or SnapshotStateMap can
encounter a deadlock if the modification is concurrent with
a direct write to the state record. This was made signficantly
more likely to be encountered with the changes introduced by
93fcae828b that uses direct writes to release unused records.
The locks are now ordered in that a snapshot lock is never
attempted to be taken when a map or list lock is held.
Fixes: 269402895
Test: ./gradlew :composer:r:r:tDUT
(cherry picked from https://android-review.googlesource.com/q/commit:5e7f64d97283fb2be1db3f0d5c6dcd1a55c1a425)
Merged-In: Ie8b87caecabc88f6a6dd610805d964aed3489b85
Change-Id: Ie8b87caecabc88f6a6dd610805d964aed3489b85
diff --git a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/SnapshotStateList.kt b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/SnapshotStateList.kt
index cf562db..2ae56e8 100644
--- a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/SnapshotStateList.kt
+++ b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/SnapshotStateList.kt
@@ -106,8 +106,8 @@
override fun addAll(elements: Collection<T>) = conditionalUpdate { it.addAll(elements) }
override fun clear() {
- synchronized(sync) {
- writable {
+ writable {
+ synchronized(sync) {
list = persistentListOf()
modification++
}
@@ -167,8 +167,8 @@
val builder = oldList!!.builder()
result = block(builder)
val newList = builder.build()
- if (newList == oldList || synchronized(sync) {
- writable {
+ if (newList == oldList || writable {
+ synchronized(sync) {
if (modification == currentModification) {
list = newList
modification++
@@ -201,8 +201,8 @@
result = false
break
}
- if (synchronized(sync) {
- writable {
+ if (writable {
+ synchronized(sync) {
if (modification == currentModification) {
list = newList
modification++
diff --git a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/SnapshotStateMap.kt b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/SnapshotStateMap.kt
index aceccb7..9b77e7ce 100644
--- a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/SnapshotStateMap.kt
+++ b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/SnapshotStateMap.kt
@@ -137,8 +137,8 @@
val builder = oldMap!!.builder()
result = block(builder)
val newMap = builder.build()
- if (newMap == oldMap || synchronized(sync) {
- writable {
+ if (newMap == oldMap || writable {
+ synchronized(sync) {
if (modification == currentModification) {
map = newMap
modification++
@@ -153,8 +153,8 @@
private inline fun update(block: (PersistentMap<K, V>) -> PersistentMap<K, V>) = withCurrent {
val newMap = block(map)
- if (newMap !== map) synchronized(sync) {
- writable {
+ if (newMap !== map) writable {
+ synchronized(sync) {
map = newMap
modification++
}
diff --git a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/snapshots/SnapshotStateListTests.kt b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/snapshots/SnapshotStateListTests.kt
index d03fa3f..41be7ba 100644
--- a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/snapshots/SnapshotStateListTests.kt
+++ b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/snapshots/SnapshotStateListTests.kt
@@ -27,6 +27,9 @@
import kotlin.test.assertFailsWith
import kotlin.test.assertFalse
import kotlin.test.assertTrue
+import kotlinx.coroutines.channels.Channel
+import kotlinx.coroutines.channels.consumeEach
+import kotlinx.test.IgnoreJsTarget
class SnapshotStateListTests {
@Test
@@ -567,7 +570,9 @@
}
}
- @Test @OptIn(ExperimentalCoroutinesApi::class)
+ @Test(timeout = 10_000)
+ @OptIn(ExperimentalCoroutinesApi::class)
+ @IgnoreJsTarget // Not relevant in a single threaded environment
fun concurrentGlobalModifications_addAll(): Unit = runTest {
repeat(100) {
val list = mutableStateListOf<Int>()
@@ -587,6 +592,104 @@
}
}
+ @Test(timeout = 5000)
+ @OptIn(ExperimentalCoroutinesApi::class)
+ @IgnoreJsTarget // Not relevant in a single threaded environment
+ fun concurrentMixingWriteApply_add(): Unit = runTest {
+ repeat(1000) {
+ val lists = Array(100) { mutableStateListOf<Int>() }.toList()
+ val channel = Channel<Unit>(Channel.CONFLATED)
+ coroutineScope {
+ // Launch mutator
+ launch(Dispatchers.Default) {
+ repeat(100) { index ->
+ lists.fastForEach { list ->
+ list.add(index)
+ }
+
+ // Simulate the write observer
+ channel.trySend(Unit)
+ }
+ channel.close()
+ }
+
+ // Simulate the global snapshot manager
+ launch(Dispatchers.Default) {
+ channel.consumeEach {
+ Snapshot.notifyObjectsInitialized()
+ }
+ }
+ }
+ }
+ // Should only get here if the above doesn't deadlock.
+ }
+
+ @Test(timeout = 5000)
+ @OptIn(ExperimentalCoroutinesApi::class)
+ @IgnoreJsTarget // Not relevant in a single threaded environment
+ fun concurrentMixingWriteApply_addAll_clear(): Unit = runTest {
+ repeat(100) {
+ val lists = Array(100) { mutableStateListOf<Int>() }.toList()
+ val data = Array(100) { index -> index }.toList()
+ val channel = Channel<Unit>(Channel.CONFLATED)
+ coroutineScope {
+ // Launch mutator
+ launch(Dispatchers.Default) {
+ repeat(100) {
+ lists.fastForEach { list ->
+ list.addAll(data)
+ list.clear()
+ }
+ // Simulate the write observer
+ channel.trySend(Unit)
+ }
+ channel.close()
+ }
+
+ // Simulate the global snapshot manager
+ launch(Dispatchers.Default) {
+ channel.consumeEach {
+ Snapshot.notifyObjectsInitialized()
+ }
+ }
+ }
+ }
+ // Should only get here if the above doesn't deadlock.
+ }
+
+ @Test(timeout = 5000)
+ @OptIn(ExperimentalCoroutinesApi::class)
+ @IgnoreJsTarget // Not relevant in a single threaded environment
+ fun concurrentMixingWriteApply_addAll_removeRange(): Unit = runTest {
+ repeat(10) {
+ val lists = Array(100) { mutableStateListOf<Int>() }.toList()
+ val data = Array(100) { index -> index }.toList()
+ val channel = Channel<Unit>(Channel.CONFLATED)
+ coroutineScope {
+ // Launch mutator
+ launch(Dispatchers.Default) {
+ repeat(100) {
+ lists.fastForEach { list ->
+ list.addAll(data)
+ list.removeRange(0, data.size)
+ }
+ // Simulate the write observer
+ channel.trySend(Unit)
+ }
+ channel.close()
+ }
+
+ // Simulate the global snapshot manager
+ launch(Dispatchers.Default) {
+ channel.consumeEach {
+ Snapshot.notifyObjectsInitialized()
+ }
+ }
+ }
+ }
+ // Should only get here if the above doesn't deadlock.
+ }
+
@Test
fun modificationAcrossSnapshots() {
val list = mutableStateListOf<Int>()
@@ -628,8 +731,8 @@
private fun <T> expected(expected: List<T>, actual: List<T>) {
assertEquals(expected.size, actual.size)
- (0 until expected.size).forEach {
+ expected.indices.forEach {
assertEquals(expected[it], actual[it])
}
}
-}
\ No newline at end of file
+}
diff --git a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/snapshots/SnapshotStateMapTests.kt b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/snapshots/SnapshotStateMapTests.kt
index 70d93d7..c816968 100644
--- a/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/snapshots/SnapshotStateMapTests.kt
+++ b/compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/snapshots/SnapshotStateMapTests.kt
@@ -14,6 +14,8 @@
* limitations under the License.
*/
+@file:Suppress("ConvertArgumentToSet")
+
package androidx.compose.runtime.snapshots
import androidx.compose.runtime.mutableStateMapOf
@@ -27,6 +29,8 @@
import kotlin.test.assertFailsWith
import kotlin.test.assertFalse
import kotlin.test.assertTrue
+import kotlinx.coroutines.channels.Channel
+import kotlinx.coroutines.channels.consumeEach
import kotlinx.test.IgnoreJsTarget
class SnapshotStateMapTests {
@@ -565,6 +569,71 @@
}
}
+ @Test(timeout = 5000)
+ @OptIn(ExperimentalCoroutinesApi::class)
+ @IgnoreJsTarget // Not relevant in a single threaded environment
+ fun concurrentMixingWriteApply_set(): Unit = runTest {
+ repeat(100) {
+ val maps = Array(100) { mutableStateMapOf<Int, Int>() }.toList()
+ val channel = Channel<Unit>(Channel.CONFLATED)
+ coroutineScope {
+ // Launch mutator
+ launch(Dispatchers.Default) {
+ repeat(100) { index ->
+ maps.fastForEach { map ->
+ map[index] = index
+ }
+
+ // Simulate the write observer
+ channel.trySend(Unit)
+ }
+ channel.close()
+ }
+
+ // Simulate the global snapshot manager
+ launch(Dispatchers.Default) {
+ channel.consumeEach {
+ Snapshot.notifyObjectsInitialized()
+ }
+ }
+ }
+ }
+ // Should only get here if the above doesn't deadlock.
+ }
+
+ @Test(timeout = 5000)
+ @OptIn(ExperimentalCoroutinesApi::class)
+ @IgnoreJsTarget // Not relevant in a single threaded environment
+ fun concurrentMixingWriteApply_clear(): Unit = runTest {
+ repeat(100) {
+ val maps = Array(100) { mutableStateMapOf<Int, Int>() }.toList()
+ val channel = Channel<Unit>(Channel.CONFLATED)
+ coroutineScope {
+ // Launch mutator
+ launch(Dispatchers.Default) {
+ repeat(100) {
+ maps.fastForEach { map ->
+ repeat(10) { index -> map[index] = index }
+ map.clear()
+ }
+
+ // Simulate the write observer
+ channel.trySend(Unit)
+ }
+ channel.close()
+ }
+
+ // Simulate the global snapshot manager
+ launch(Dispatchers.Default) {
+ channel.consumeEach {
+ Snapshot.notifyObjectsInitialized()
+ }
+ }
+ }
+ }
+ // Should only get here if the above doesn't deadlock.
+ }
+
private fun validateRead(
initialMap: MutableMap<Int, Float> = defaultMap(),
block: (Map<Int, Float>, Map<Int, Float>) -> Unit
