Fix crash: IllegalStateException: Unsupported concurrent change during composition#267
Open
mwichro wants to merge 4 commits into
Open
Fix crash: IllegalStateException: Unsupported concurrent change during composition#267mwichro wants to merge 4 commits into
mwichro wants to merge 4 commits into
Conversation
GlobalAppSettingsConcurrencyTest, SnapshotStateMapConcurrencyTest: add synchronization to background writers to prevent SnapshotApplyConflictException while validating concurrent reader stability.
Contributor
Author
|
@copilot review this PR |
Comment on lines
+112
to
+119
| /** | ||
| * 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 <T> mutateUiState(block: () -> T): T = | ||
| Snapshot.withMutableSnapshot(block) |
Comment on lines
26
to
30
| fun update(settings: AppSettings) { | ||
| _current.value = settings | ||
| Snapshot.withMutableSnapshot { | ||
| _current.value = settings | ||
| } | ||
| } |
Comment on lines
+92
to
+98
| start.countDown() | ||
| writersDone.await(30, TimeUnit.SECONDS) | ||
| readerDone.await(30, TimeUnit.SECONDS) | ||
| pool.shutdownNow() | ||
|
|
||
| assertNull("Snapshot-guarded writes must not throw", failure.get()) | ||
| } |
Comment on lines
+139
to
+144
| start.countDown() | ||
| done.await(30, TimeUnit.SECONDS) | ||
| pool.shutdownNow() | ||
|
|
||
| assertNull("Guarded removal must not throw during reads", failure.get()) | ||
| } |
Comment on lines
+85
to
+92
| 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) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The crash — IllegalStateException: Unsupported concurrent change during composition — happens when a Compose snapshot state object is written from a non-composition (background) thread at the same moment the recomposer is applying its own snapshot touching that object. I found two such state objects being mutated from background coroutines, both reachable during page switching.
Problem 1 — PageDataManager.pageHigh / pageScroll (the page-switch crash)
PageDataManager.kt:102 declares these as mutableStateMapOf because the UI observes them — ScrollIndicator.kt:43 reads page.height and page.scroll during composition (these delegate into the maps via PageView.kt:119-135).
But the same maps were written directly from background threads:
loadPageFromDb → recomputeHeight/setPageHeight on Dispatchers.Default,
updateScroll/updateZoom → setPageScroll on the drawing thread,
removePage (cache eviction during reduceCache/cancelUnnecessaryLoading/onExit, all fired while switching pages) removed entries on IO/Default threads.
Worst of all, getPageScroll used getOrPut, so a composition read itself could write the map — a direct write/write race with the background writers.
Fix: all mutations now go through a single mutateUiState { } helper that wraps them in Snapshot.withMutableSnapshot, committing atomically instead of racing the recomposer. getPageScroll is now a pure read that computes the default without materializing the entry (entries are created only on a real setPageScroll write). Removals in removePage are wrapped too.
Problem 2 — GlobalAppSettings.current (latent, same class of bug)
AppSettings.kt:14 is a global mutableStateOf read throughout the UI, but update() is called from Kv.setAppSettings → EditorViewModel.updateScribbleToErase inside viewModelScope.launch(Dispatchers.IO) — a background write to snapshot state. Fix: update() now commits inside Snapshot.withMutableSnapshot.
What I deliberately left alone
NotableNavigator's mutableStateOf (currentPageId, isQuickNavOpen) — all writes occur in main-thread LaunchedEffect/navigation callbacks, so they're not part of this concurrency bug. SelectionState's snapshot fields are written from gesture/coroutine handlers but are a separate concern from the page-switch crash; I didn't touch them to keep the change scoped to the actual/likely cause.
Tests
GlobalAppSettingsConcurrencyTest.kt — hammers update() from 8 threads while a reader observes inside a snapshot; asserts no throw.
SnapshotStateMapConcurrencyTest.kt — reproduces the exact PageDataManager pattern (per-page writes + removals on background threads, concurrent composition-like reads) and asserts the snapshot-guarded path never throws. Mirrors mutateUiState so a revert to direct writes would resurface the failure.
I could not compile or run the tests — there's no Gradle/JDK toolchain available in this environment (the build command was declined as "we don't have gradle installed"). The changes use stable Compose 1.11.2 snapshot APIs (Snapshot.withMutableSnapshot, takeSnapshot, enter, dispose) and should compile and pass in CI, but I have not verified that here.