Fixed race condition when reading state from the global snapshot
There was a race condition when reading state from the global
snapshot that could produce a spurious read error containing the
text "Reading a state that was created after the snapshot was taken..."
when the global snapshot was advanced one thread and being read
on another. Now, if the state record cannot be found, the records
are rescaned with the global snapshot lock to prevent records from
being reused while they are being searched for.
Fixes: b/233779717
Test: ./gradlew :compose:r:r:tDUT
Change-Id: I63e17996494dcda9fc3c4f62ffad735c551e9e64
diff --git a/compose/runtime/runtime/api/restricted_1.3.0-beta03.txt b/compose/runtime/runtime/api/restricted_1.3.0-beta03.txt
index f82be3b..b50e6f8 100644
--- a/compose/runtime/runtime/api/restricted_1.3.0-beta03.txt
+++ b/compose/runtime/runtime/api/restricted_1.3.0-beta03.txt
@@ -902,6 +902,7 @@
public final class SnapshotKt {
method @kotlin.PublishedApi internal static <T extends androidx.compose.runtime.snapshots.StateRecord> T current(T r, androidx.compose.runtime.snapshots.Snapshot snapshot);
+ method @kotlin.PublishedApi internal static <T extends androidx.compose.runtime.snapshots.StateRecord> T current(T r);
method @kotlin.PublishedApi internal static void notifyWrite(androidx.compose.runtime.snapshots.Snapshot snapshot, androidx.compose.runtime.snapshots.StateObject state);
method public static <T extends androidx.compose.runtime.snapshots.StateRecord> T readable(T, androidx.compose.runtime.snapshots.StateObject state);
method public static <T extends androidx.compose.runtime.snapshots.StateRecord> T readable(T, androidx.compose.runtime.snapshots.StateObject state, androidx.compose.runtime.snapshots.Snapshot snapshot);
diff --git a/compose/runtime/runtime/api/restricted_current.txt b/compose/runtime/runtime/api/restricted_current.txt
index f82be3b..b50e6f8 100644
--- a/compose/runtime/runtime/api/restricted_current.txt
+++ b/compose/runtime/runtime/api/restricted_current.txt
@@ -902,6 +902,7 @@
public final class SnapshotKt {
method @kotlin.PublishedApi internal static <T extends androidx.compose.runtime.snapshots.StateRecord> T current(T r, androidx.compose.runtime.snapshots.Snapshot snapshot);
+ method @kotlin.PublishedApi internal static <T extends androidx.compose.runtime.snapshots.StateRecord> T current(T r);
method @kotlin.PublishedApi internal static void notifyWrite(androidx.compose.runtime.snapshots.Snapshot snapshot, androidx.compose.runtime.snapshots.StateObject state);
method public static <T extends androidx.compose.runtime.snapshots.StateRecord> T readable(T, androidx.compose.runtime.snapshots.StateObject state);
method public static <T extends androidx.compose.runtime.snapshots.StateRecord> T readable(T, androidx.compose.runtime.snapshots.StateObject state, androidx.compose.runtime.snapshots.Snapshot snapshot);
diff --git a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/Snapshot.kt b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/Snapshot.kt
index 489920f..25e87d6 100644
--- a/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/Snapshot.kt
+++ b/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/Snapshot.kt
@@ -1828,8 +1828,19 @@
* Return the current readable state record for the current snapshot. It is assumed that [this]
* is the first record of [state]
*/
-fun <T : StateRecord> T.readable(state: StateObject): T =
- readable(state, currentSnapshot())
+fun <T : StateRecord> T.readable(state: StateObject): T {
+ val snapshot = Snapshot.current
+ snapshot.readObserver?.invoke(state)
+ return readable(this, snapshot.id, snapshot.invalid) ?: sync {
+ // Readable can return null when the global snapshot has been advanced by another thread
+ // and state written to the object was overwritten while this thread was paused. Repeating
+ // the read is valid here as either this will return the same result as the previous call
+ // or will find a valid record. Being in a sync block prevents other threads from writing
+ // to this state object until the read completes.
+ val syncSnapshot = Snapshot.current
+ readable(this, syncSnapshot.id, syncSnapshot.invalid)
+ } ?: readError()
+}
/**
* Return the current readable state record for the [snapshot]. It is assumed that [this]
@@ -2089,13 +2100,23 @@
internal fun <T : StateRecord> current(r: T, snapshot: Snapshot) =
readable(r, snapshot.id, snapshot.invalid) ?: readError()
+@PublishedApi
+internal fun <T : StateRecord> current(r: T) =
+ Snapshot.current.let { snapshot ->
+ readable(r, snapshot.id, snapshot.invalid) ?: sync {
+ Snapshot.current.let { syncSnapshot ->
+ readable(r, syncSnapshot.id, syncSnapshot.invalid)
+ }
+ } ?: readError()
+ }
+
/**
* Provides a [block] with the current record, without notifying any read observers.
*
* @see readable
*/
inline fun <T : StateRecord, R> T.withCurrent(block: (r: T) -> R): R =
- block(current(this, Snapshot.current))
+ block(current(this))
/**
* Helper routine to add a range of values ot a snapshot set
diff --git a/compose/runtime/runtime/src/jvmTest/kotlin/androidx/compose/runtime/snapshots/SnapshotTestsJvm.kt b/compose/runtime/runtime/src/jvmTest/kotlin/androidx/compose/runtime/snapshots/SnapshotTestsJvm.kt
new file mode 100644
index 0000000..307a8eb
--- /dev/null
+++ b/compose/runtime/runtime/src/jvmTest/kotlin/androidx/compose/runtime/snapshots/SnapshotTestsJvm.kt
@@ -0,0 +1,77 @@
+/*
+ * Copyright 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package androidx.compose.runtime.snapshots
+
+import androidx.compose.runtime.AtomicInt
+import androidx.compose.runtime.AtomicReference
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.postIncrement
+import java.util.concurrent.atomic.AtomicBoolean
+import kotlin.concurrent.thread
+import kotlin.random.Random
+import kotlin.test.Test
+import kotlin.test.assertNull
+
+class SnapshotTestsJvm {
+
+ @Test
+ fun testMultiThreadedReadingAndWritingOfGlobalScope() {
+ val running = AtomicBoolean(true)
+ val reads = AtomicInt(0)
+ val writes = AtomicInt(0)
+ val lowNumberSeen = AtomicInt(0)
+ val exception = AtomicReference<Throwable?>(null)
+ try {
+ val state = mutableStateOf(0)
+ Snapshot.notifyObjectsInitialized()
+
+ // Create 100 reader threads of state
+ repeat(100) {
+ thread {
+ try {
+ while (running.get()) {
+ reads.postIncrement()
+ if (state.value < 1000) lowNumberSeen.postIncrement()
+ }
+ } catch (e: Throwable) {
+ exception.set(e)
+ running.set(false)
+ }
+ }
+ }
+
+ // Create 10 writer threads
+ repeat(10) {
+ thread {
+ while (running.get()) {
+ writes.postIncrement()
+ state.value = Random.nextInt(10000)
+ Snapshot.sendApplyNotifications()
+ }
+ }
+ }
+
+ while (running.get() && writes.get() < 10000) {
+ Thread.sleep(0)
+ }
+ } finally {
+ running.set(false)
+ }
+
+ assertNull(exception.get())
+ }
+}
\ No newline at end of file
