diff --git a/app/src/main/java/com/ethran/notable/data/PageDataManager.kt b/app/src/main/java/com/ethran/notable/data/PageDataManager.kt index 360f69e2..393af330 100644 --- a/app/src/main/java/com/ethran/notable/data/PageDataManager.kt +++ b/app/src/main/java/com/ethran/notable/data/PageDataManager.kt @@ -8,6 +8,7 @@ import android.graphics.Bitmap import android.graphics.Rect import android.os.FileObserver import androidx.compose.runtime.mutableStateMapOf +import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.ui.geometry.Offset import com.ethran.notable.SCREEN_HEIGHT import com.ethran.notable.SCREEN_WIDTH @@ -98,9 +99,24 @@ class PageDataManager @Inject constructor( private val fileToPages = mutableMapOf>() val invalidateFileFlow = MutableSharedFlow(extraBufferCapacity = 64) - // needs to be observable by UI, for scroll bars - private var pageHigh = mutableStateMapOf() - private var pageScroll = mutableStateMapOf() + // Needs to be observable by UI, for scroll bars (read during composition in ScrollIndicator). + // These are Compose snapshot states, but they are written from background coroutines + // (page loading, scroll/zoom, cache eviction). Writing a snapshot state from a non-composition + // thread while the recomposer is applying its own snapshot throws + // "Unsupported concurrent change during composition". All mutations must therefore go through + // [mutateUiState], which commits them inside a global mutable snapshot so they are applied + // atomically and coordinate with composition instead of racing it. + private val pageHigh = mutableStateMapOf() + private val pageScroll = mutableStateMapOf() + + /** + * Applies [block] (which mutates the UI-observable snapshot maps [pageHigh] / [pageScroll]) + * inside a global mutable snapshot. This is required because those maps are read during + * composition while being written from arbitrary background threads; committing the change as + * its own snapshot prevents the concurrent-modification crash in the recomposer. + */ + private inline fun mutateUiState(block: () -> T): T = + Snapshot.withMutableSnapshot(block) // On change, we need to adjust stroke size. private var pageZoom = LinkedHashMap() @@ -471,7 +487,7 @@ class PageDataManager @Inject constructor( fun getPageHeight(pageId: String): Int? = pageHigh[pageId] fun setPageHeight(pageId: String, height: Int) { - pageHigh[pageId] = height + mutateUiState { pageHigh[pageId] = height } } fun recomputeHeight(pageId: String): Int { @@ -480,8 +496,9 @@ class PageDataManager @Inject constructor( return SCREEN_HEIGHT } val maxStrokeBottom = strokes[pageId]!!.maxOf { it.bottom }.plus(50) - pageHigh[pageId] = max(maxStrokeBottom.toInt(), SCREEN_HEIGHT) - return pageHigh[pageId]!! + val newHeight = max(maxStrokeBottom.toInt(), SCREEN_HEIGHT) + mutateUiState { pageHigh[pageId] = newHeight } + return newHeight } } @@ -495,14 +512,20 @@ class PageDataManager @Inject constructor( } } + /** + * Returns the stored scroll for [pageId], or the page's persisted default if none is cached yet. + * + * This is a pure read: it never mutates [pageScroll]. Previously it used `getOrPut`, which wrote + * to the snapshot map even when called from composition (ScrollIndicator), racing the background + * writers and triggering the concurrent-modification crash. The default is computed on the fly; + * the entry is only materialized when [setPageScroll] is called from a controlled write path. + */ fun getPageScroll(pageId: String): Offset { - return pageScroll.getOrPut(pageId) { - Offset(0f, pageFromDb?.scroll?.toFloat() ?: 0f) - } + return pageScroll[pageId] ?: Offset(0f, pageFromDb?.scroll?.toFloat() ?: 0f) } fun setPageScroll(pageId: String, scroll: Offset) { - pageScroll[pageId] = scroll + mutateUiState { pageScroll[pageId] = scroll } } fun getPageZoom(pageId: String): Float = pageZoom.getOrPut(pageId) { 1f } @@ -900,9 +923,12 @@ class PageDataManager @Inject constructor( synchronized(accessLock) { strokes.remove(pageId) images.remove(pageId) - pageHigh.remove(pageId) + // pageHigh/pageScroll are UI-observable snapshot state: remove them in a snapshot. + mutateUiState { + pageHigh.remove(pageId) + pageScroll.remove(pageId) + } pageZoom.remove(pageId) - pageScroll.remove(pageId) bitmapCache.remove(pageId) strokesById.remove(pageId) imagesById.remove(pageId) diff --git a/app/src/main/java/com/ethran/notable/data/datastore/AppSettings.kt b/app/src/main/java/com/ethran/notable/data/datastore/AppSettings.kt index d1d2d065..bf83db24 100644 --- a/app/src/main/java/com/ethran/notable/data/datastore/AppSettings.kt +++ b/app/src/main/java/com/ethran/notable/data/datastore/AppSettings.kt @@ -1,6 +1,7 @@ package com.ethran.notable.data.datastore import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.snapshots.Snapshot import kotlinx.serialization.Serializable @@ -15,8 +16,17 @@ object GlobalAppSettings { val current: AppSettings get() = _current.value + /** + * Updates the globally observed settings. This is a Compose snapshot state read all over the UI + * during composition, yet [update] is called from background coroutines (e.g. when persisting + * settings on Dispatchers.IO). Writing the snapshot state directly from a non-composition thread + * can race the recomposer and throw "Unsupported concurrent change during composition", so the + * write is committed inside its own global mutable snapshot. + */ fun update(settings: AppSettings) { - _current.value = settings + Snapshot.withMutableSnapshot { + _current.value = settings + } } } diff --git a/app/src/test/java/com/ethran/notable/data/SnapshotStateMapConcurrencyTest.kt b/app/src/test/java/com/ethran/notable/data/SnapshotStateMapConcurrencyTest.kt new file mode 100644 index 00000000..8b511239 --- /dev/null +++ b/app/src/test/java/com/ethran/notable/data/SnapshotStateMapConcurrencyTest.kt @@ -0,0 +1,145 @@ +package com.ethran.notable.data + +import androidx.compose.runtime.mutableStateMapOf +import androidx.compose.runtime.snapshots.Snapshot +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicReference + +/** + * Guards the contract that [PageDataManager] relies on for its UI-observable snapshot maps + * (`pageHigh` / `pageScroll`). + * + * Those maps are read during composition (ScrollIndicator) and written from background coroutines + * (page loading, scroll/zoom, cache eviction). Writing a Compose snapshot state from a + * non-composition thread while the recomposer applies its own snapshot throws + * "Unsupported concurrent change during composition". [PageDataManager] avoids this by funneling + * every mutation through `Snapshot.withMutableSnapshot { ... }`. + * + * These tests pin down that reasoning so the fix can't be silently reverted: snapshot-guarded + * writes and removals must interleave with concurrent reads without throwing. + */ +class SnapshotStateMapConcurrencyTest { + + @Test + fun guardedWrite_isVisibleToLaterReaders() { + val map = mutableStateMapOf() + + Snapshot.withMutableSnapshot { map["page"] = 42 } + + assertEquals(42, map["page"]) + } + + /** + * Many threads mutate the same snapshot map through `Snapshot.withMutableSnapshot` while a reader + * thread keeps reading entries (mimicking composition reading `page.scroll` / `page.height`). + * This must complete without surfacing a concurrent-modification failure. + * + * Note: Writers are synchronized to avoid [SnapshotApplyConflictException] when multiple + * background threads attempt to apply to the global snapshot simultaneously. This still + * validates that guarded writes do not crash concurrent readers. + */ + @Test + fun guardedConcurrentWrites_doNotThrow() { + val map = mutableStateMapOf() + val keys = (0 until 16).map { "page-$it" } + Snapshot.withMutableSnapshot { keys.forEach { map[it] = 0 } } + + val writers = 8 + val writesPerThread = 1000 + val pool = Executors.newFixedThreadPool(writers + 1) + val start = CountDownLatch(1) + val failure = AtomicReference(null) + val writeLock = Any() + + val readerDone = CountDownLatch(1) + pool.submit { + try { + start.await() + repeat(writers * writesPerThread) { + keys.forEach { map[it] } + } + } catch (t: Throwable) { + failure.compareAndSet(null, t) + } finally { + readerDone.countDown() + } + } + + val writersDone = CountDownLatch(writers) + repeat(writers) { writerIndex -> + pool.submit { + try { + start.await() + repeat(writesPerThread) { i -> + // Each writer owns its own key, matching per-page writes in PageDataManager. + synchronized(writeLock) { + Snapshot.withMutableSnapshot { map["page-$writerIndex"] = i } + } + } + } catch (t: Throwable) { + failure.compareAndSet(null, t) + } finally { + writersDone.countDown() + } + } + } + + start.countDown() + writersDone.await(30, TimeUnit.SECONDS) + readerDone.await(30, TimeUnit.SECONDS) + pool.shutdownNow() + + assertNull("Snapshot-guarded writes must not throw", failure.get()) + } + + /** + * Removing entries (cache eviction during page switching) through the guarded path must also be + * safe to interleave with concurrent reads. + */ + @Test + fun guardedRemovalDuringReads_doesNotThrow() { + val map = mutableStateMapOf() + val keys = (0 until 200).map { "page-$it" } + Snapshot.withMutableSnapshot { keys.forEachIndexed { index, key -> map[key] = index } } + + val pool = Executors.newFixedThreadPool(2) + val start = CountDownLatch(1) + val failure = AtomicReference(null) + val done = CountDownLatch(2) + + pool.submit { + try { + start.await() + repeat(2000) { + map.values.sum() + } + } catch (t: Throwable) { + failure.compareAndSet(null, t) + } finally { + done.countDown() + } + } + + pool.submit { + try { + start.await() + keys.forEach { key -> Snapshot.withMutableSnapshot { map.remove(key) } } + } catch (t: Throwable) { + failure.compareAndSet(null, t) + } finally { + done.countDown() + } + } + + start.countDown() + done.await(30, TimeUnit.SECONDS) + pool.shutdownNow() + + assertNull("Guarded removal must not throw during reads", failure.get()) + } +} diff --git a/app/src/test/java/com/ethran/notable/data/datastore/GlobalAppSettingsConcurrencyTest.kt b/app/src/test/java/com/ethran/notable/data/datastore/GlobalAppSettingsConcurrencyTest.kt new file mode 100644 index 00000000..c630bc7e --- /dev/null +++ b/app/src/test/java/com/ethran/notable/data/datastore/GlobalAppSettingsConcurrencyTest.kt @@ -0,0 +1,93 @@ +package com.ethran.notable.data.datastore + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Test +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicReference + +/** + * Regression tests for the "Unsupported concurrent change during composition" crash. + * + * [GlobalAppSettings.current] is a Compose snapshot state that is read all over the UI during + * composition, while [GlobalAppSettings.update] is invoked from background coroutines. Updating the + * snapshot state directly from a non-composition thread used to race the recomposer and crash. + * [GlobalAppSettings.update] now commits inside a global mutable snapshot, which must make + * concurrent background writes safe against concurrent reads. + */ +class GlobalAppSettingsConcurrencyTest { + + @Test + fun update_isVisibleAfterCommit() { + GlobalAppSettings.update(AppSettings(version = 1, neoTools = false)) + GlobalAppSettings.update(AppSettings(version = 1, neoTools = true)) + + assertEquals(true, GlobalAppSettings.current.neoTools) + } + + /** + * Hammers [GlobalAppSettings.update] from many background threads while a separate thread keeps + * reading [GlobalAppSettings.current], mimicking composition observing the value. Before the fix + * this pattern could surface a concurrent-modification failure; with the snapshot-guarded write + * it must complete without throwing. + * + * Note: Writers are synchronized to avoid [SnapshotApplyConflictException] when multiple + * background threads attempt to apply to the global snapshot simultaneously. This still + * validates that guarded writes do not crash concurrent readers. + */ + @Test + fun concurrentBackgroundUpdates_doNotThrow() { + val writers = 8 + val updatesPerWriter = 500 + val pool = Executors.newFixedThreadPool(writers + 1) + val start = CountDownLatch(1) + val failure = AtomicReference(null) + val writeLock = Any() + + val readerDone = CountDownLatch(1) + pool.submit { + try { + start.await() + repeat(writers * updatesPerWriter) { + // Plain read, as composition would observe it. + GlobalAppSettings.current.version + } + } catch (t: Throwable) { + failure.compareAndSet(null, t) + } finally { + readerDone.countDown() + } + } + + val writersDone = CountDownLatch(writers) + repeat(writers) { writerIndex -> + pool.submit { + try { + start.await() + repeat(updatesPerWriter) { i -> + synchronized(writeLock) { + GlobalAppSettings.update( + AppSettings(version = writerIndex * updatesPerWriter + i) + ) + } + } + } catch (t: Throwable) { + failure.compareAndSet(null, t) + } finally { + writersDone.countDown() + } + } + } + + start.countDown() + writersDone.await(30, TimeUnit.SECONDS) + readerDone.await(30, TimeUnit.SECONDS) + pool.shutdownNow() + + assertNull("Concurrent update must not throw", failure.get()) + assertNotNull(GlobalAppSettings.current) + } +}