Skip to content

Commit

Permalink
Fix "Unsupported concurrent change during composition"
Browse files Browse the repository at this point in the history
Fixes https://youtrack.jetbrains.com/issue/CMP-6729/iOS-Runtime-crash.-Unsupported-concurrent-change-during-composition

Regression after #1574

The draw call is similar to:
```
// NodeCoordinator.drawBlock
observer.observeReads(Unit, {
  RenderNodeLayer.invalidate()
    drawState.value = Unit
}) {
  RenderNodeLayer.drawLayer()
    drawState.value
}
```

So, triggering the observer in response to the state change also changes this state, which triggers observer again.

It is sometimes manifested via this call:
```
performRecompose()
  val snapshot = Snapshot.takeMutableSnapshot()
    Snapshot.sendApplyNotifications() // triggers drawState.value = Unit
  snapshot.enter {
    ...
    Snapshot.sendApplyNotifications() // can be called anywhere, triggers the observer of `drawState` because we changed `drawState` during the previous `sendApplyNotifications`
  }
  snapshot.apply() // fails with "Unsupported concurrent change during composition"
```
(I haven't figured why sometimes and not always)

Considering this, *we shouldn't change any state we read in `draw` inside `invalidate`*
(because *we shouldn't change any state we read in `observeReads(block=)`) inside observeReads(onValueChangedForScope=)`*)

We added this state not to trigger `RenderNodeLayer.drawLayer` after `RenderNodeLayer.invalidate`, but to trigger `SkiaGraphicsLayer.invalidate` after we called `RenderNodeLayer.drawLayer` inside `SkiaGraphicsLayer.draw`.

Because:
- we still need some way to notify `SkiaGraphicsLayer.invalidate`
- only `RenderNodeLayer.invalidate` knows that `RenderNodeLayer.drawLayer` is changed

We notify via `isDirty` state and a separate check outside `invalidate`

<details>
  <summary>A synthetic reproducer of the issue for history (skip it if details are already understood)</summary>
```
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.neverEqualPolicy
import androidx.compose.runtime.snapshots.Snapshot
import androidx.compose.runtime.snapshots.SnapshotStateObserver

fun main() {
    val drawState = mutableStateOf(Unit, neverEqualPolicy())

    val observer = SnapshotStateObserver { it.invoke() }
    observer.start()

    fun draw() {
        observer.observeReads(Unit, {
            println("invalidate draw (drawState.value = Unit)")
            drawState.value = Unit
        }) {
            println("\ndraw")
            drawState.value
        }
    }

    fun externalChange() {
        println("\nexternalChange")
        drawState.value = Unit
        Snapshot.sendApplyNotifications()
    }

    fun composition() {
        println("\ncomposition BEFORE")
        Snapshot.takeMutableSnapshot().apply {
            enter {
                println("composition BEGIN")
                println("sendApplyNotifications")
                Snapshot.sendApplyNotifications()
                println("composition END")
            }
            println("applying composition state to global state")
            if (!apply().succeeded) {
                throw RuntimeException("Composition apply failed")
            }
        }
    }

    // frame 0
    draw()

    externalChange()

    // frame 1
    composition()
}
```
Output:
```
draw

externalChange
invalidate draw (drawState.value = Unit)

composition BEFORE
invalidate draw (drawState.value = Unit)
composition BEGIN
sendApplyNotifications
invalidate draw (drawState.value = Unit)
composition END
applying composition state to global state
Exception in thread "main" java.lang.RuntimeException: Composition apply failed
	at MainKt.main$composition(main.kt:39)
	at MainKt.main(main.kt:50)
	at MainKt.main(main.kt)
```

</details>

## Release Notes
### Fixes - Multiplatform
- _(prerelease fix)_ Fix "Unsupported concurrent change during composition"
  • Loading branch information
igordmn committed Sep 25, 2024
1 parent 44ed9b7 commit d9d9b3a
Show file tree
Hide file tree
Showing 3 changed files with 374 additions and 9 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
* Copyright 2024 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.ui

import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.verticalScroll
import androidx.compose.material.CircularProgressIndicator
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.setValue
import androidx.compose.runtime.snapshots.Snapshot
import androidx.compose.ui.graphics.asComposeCanvas
import androidx.compose.ui.scene.CanvasLayersComposeScene
import androidx.compose.ui.scene.ComposeScene
import kotlinx.coroutines.CoroutineExceptionHandler
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.runBlocking
import org.jetbrains.skia.Surface
import org.jetbrains.skiko.MainUIDispatcher
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.JUnit4

/**
* Collection of tests for encountered bugs
*/
@RunWith(JUnit4::class)
class ComposeSceneBugsTest {
// https://youtrack.jetbrains.com/issue/CMP-6729/iOS-Runtime-crash.-Unsupported-concurrent-change-during-composition
// sendApplyNotifications can be called anywhere. When it was called inside composition, it triggers wrongly written observers
@Test
fun `no crash when sendApplyNotifications performed in composition`() {
runLayerSceneTest { scene ->
val canvas = Surface.makeRasterN32Premul(100, 100).canvas

var triggerApplySnapshot by mutableStateOf(false)

scene.setContent {
Column(Modifier.fillMaxSize().verticalScroll(rememberScrollState())) {
CircularProgressIndicator()
}

if (triggerApplySnapshot) {
Snapshot.sendApplyNotifications()
}
}

repeat(10) {
scene.render(canvas.asComposeCanvas(), it * 100L)
}

triggerApplySnapshot = true

repeat(10) {
scene.render(canvas.asComposeCanvas(), 1000 + it * 100L)
}
}
}

private fun runLayerSceneTest(body: CoroutineScope.(ComposeScene) -> Unit) {
var coroutineException: Throwable? = null

// catching recomposition exceptions this way because of https://youtrack.jetbrains.com/issue/CMP-6734/ComposeScene-doesnt-catch-exceptions-during-recomposition
// we catch exceptions this way with real ComposeWindow and the other testing method (runComposeUiTest)
runBlocking(
MainUIDispatcher + CoroutineExceptionHandler { _, throwable ->
coroutineException = throwable
}
) {
CanvasLayersComposeScene(coroutineContext = coroutineContext).apply {
body(this)
close()
}
}

if (coroutineException != null) {
throw coroutineException!!
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,46 @@

package androidx.compose.ui.graphics

import androidx.compose.foundation.ScrollState
import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.width
import androidx.compose.foundation.verticalScroll
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.setValue
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.draw.drawWithContent
import androidx.compose.ui.geometry.Offset
import androidx.compose.ui.geometry.Size
import androidx.compose.ui.graphics.layer.drawLayer
import androidx.compose.ui.platform.LocalGraphicsContext
import androidx.compose.ui.platform.testTag
import androidx.compose.ui.test.ExperimentalTestApi
import androidx.compose.ui.test.runSkikoComposeUiTest
import androidx.compose.ui.test.InternalTestApi
import androidx.compose.ui.test.SkikoComposeUiTest
import androidx.compose.ui.test.onNodeWithTag
import androidx.compose.ui.test.runInternalSkikoComposeUiTest
import androidx.compose.ui.unit.dp
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.time.Duration.Companion.seconds
import kotlinx.coroutines.test.StandardTestDispatcher
import kotlinx.coroutines.test.runTest

@ExperimentalTestApi
class GraphicsLayerTest {

// TODO: Add test timeout once available
@Test // Bug: https://youtrack.jetbrains.com/issue/CMP-6660
fun layerDrawingWithRecording() = runSkikoComposeUiTest {
// Bug: https://youtrack.jetbrains.com/issue/CMP-6660
@Test
fun layerDrawingWithRecording() = runLayerTest {
var drawCount = 0
setContent {
val graphicsLayer = rememberGraphicsLayer()
Expand All @@ -42,12 +65,181 @@ class GraphicsLayerTest {
drawLayer(graphicsLayer)
})
}
waitForIdle()

// UnconfinedTestDispatcher might make some difference here: instead of possible infinite
// invalidation cycle, it resolves invalidations during the same render.
// But even in this case, with incorrect behaviour composition counters won't be equal 1
// shouldn't hang
awaitIdle()
}

// Bug: https://youtrack.jetbrains.com/issue/CMP-6695
@Test
fun invalidateWhenScrollChanged() = runLayerTest {
val scrollState = ScrollState(0)

setContent {
val backgroundLayer = rememberGraphicsLayer()

Box(
Modifier
.fillMaxSize()
.drawWithContent {
backgroundLayer.record {
this@drawWithContent.drawContent()
}
this@drawWithContent.drawContent()
}
) {
Column(
modifier = Modifier
.verticalScroll(scrollState)
.fillMaxWidth()
) {
repeat(100) {
Box(Modifier
.size(100.dp)
.background(if (it % 2 == 0) Color.Red else Color.Black))
}
}
}

Spacer(
modifier = Modifier
.testTag("bar")
.fillMaxWidth()
.height(80.dp)
.clip(RectangleShape)
.drawWithContent {
drawLayer(backgroundLayer)
}
)
}

awaitIdle()
assertEquals(Color.Red, onNodeWithTag("bar").captureToImage().toPixelMap()[0, 0])

scrollState.scrollTo(100)
awaitIdle()
assertEquals(Color.Black, onNodeWithTag("bar").captureToImage().toPixelMap()[0, 0])

scrollState.scrollTo(200)
awaitIdle()
assertEquals(Color.Red, onNodeWithTag("bar").captureToImage().toPixelMap()[0, 0])
}

// Bug: https://youtrack.jetbrains.com/issue/CMP-6695
@Test
fun invalidateNestedDrawWhenScrollChanged() = runLayerTest {
val scrollState = ScrollState(0)

setContent {
val graphicsContext = LocalGraphicsContext.current
val backgroundLayer = rememberGraphicsLayer()

Box(
Modifier
.fillMaxSize()
.drawWithContent {
backgroundLayer.record {
this@drawWithContent.drawContent()
}
this@drawWithContent.drawContent()
}
) {
Column(
modifier = Modifier
.verticalScroll(scrollState)
.fillMaxWidth()
) {
repeat(100) {
Box(Modifier
.size(100.dp)
.background(if (it % 2 == 0) Color.Red else Color.Black))
}
}
}

Spacer(
modifier = Modifier
.testTag("bar")
.fillMaxWidth()
.height(80.dp)
.clip(RectangleShape)
.drawWithContent {
val layer = graphicsContext.createGraphicsLayer()
layer.record { drawLayer(backgroundLayer) }
drawLayer(layer)
graphicsContext.releaseGraphicsLayer(layer)
}
)
}

awaitIdle()
assertEquals(Color.Red, onNodeWithTag("bar").captureToImage().toPixelMap()[0, 0])

scrollState.scrollTo(100)
awaitIdle()
assertEquals(Color.Black, onNodeWithTag("bar").captureToImage().toPixelMap()[0, 0])

scrollState.scrollTo(200)
awaitIdle()
assertEquals(Color.Red, onNodeWithTag("bar").captureToImage().toPixelMap()[0, 0])
}

@Test
fun invalidateWhenContentChanged() = runLayerTest {
var color by mutableStateOf(Color.Red)

setContent {
val backgroundLayer = rememberGraphicsLayer()

assertEquals(1, drawCount)
Box(
Modifier
.fillMaxSize()
.drawWithContent {
backgroundLayer.record {
this@drawWithContent.drawContent()
}
this@drawWithContent.drawContent()
}
) {
Box(Modifier.width(100.dp).height(1000.dp).drawWithContent {
drawRect(color, Offset.Zero, Size(100f, 1000f))
})
}

Spacer(
modifier = Modifier
.testTag("bar")
.fillMaxWidth()
.height(80.dp)
.clip(RectangleShape)
.drawWithContent {
drawLayer(backgroundLayer)
}
)
}

awaitIdle()
assertEquals(Color.Red, onNodeWithTag("bar").captureToImage().toPixelMap()[0, 0])

color = Color.Black
awaitIdle()
assertEquals(Color.Black, onNodeWithTag("bar").captureToImage().toPixelMap()[0, 0])

color = Color.Red
awaitIdle()
assertEquals(Color.Red, onNodeWithTag("bar").captureToImage().toPixelMap()[0, 0])
}

@OptIn(InternalTestApi::class)
private fun runLayerTest(body: suspend SkikoComposeUiTest.() -> Unit) {
runInternalSkikoComposeUiTest(
coroutineDispatcher = StandardTestDispatcher()
) {
runOnUiThread {
runTest(timeout = 10.seconds) {
body()
}
}
}
}
}
Loading

0 comments on commit d9d9b3a

Please sign in to comment.