Remove calls to invokeOnCompletion
This CL removes all calls to invokeOnCompletion in STL codebase. As
mentioned in the KDoc, there is no guarantee on which thread the
invokeOnCompletion block is run, which can cause concurrency issues that
throw an exception since ag/27342795.
The calls to invokeOnCompletion were replaced by a try/finally block.
However, it is important to note that a try block launched inside a
coroutine might never run if the coroutine is cancelled before it
starts. For this reason, we must also specify `start = ATOMIC` when
launching the coroutine to make sure that the try/finally block will
definitely run until the first suspension point.
Bug: 290930950
Bug: 340824084
Test: atest PlatformComposeSceneTransitionLayoutTests
Flag: com.android.systemui.scene_container
Change-Id: Ia822d4a2e0f424cd7a6715fb7dbeaf46affcaba3
diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/AnimateToScene.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/AnimateToScene.kt
index b5e9313..48a348b 100644
--- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/AnimateToScene.kt
+++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/AnimateToScene.kt
@@ -21,6 +21,7 @@
import androidx.compose.animation.core.SpringSpec
import kotlin.math.absoluteValue
import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.CoroutineStart
import kotlinx.coroutines.Job
import kotlinx.coroutines.launch
@@ -190,16 +191,17 @@
}
// Animate the progress to its target value.
+ // Important: We start atomically to make sure that we start the coroutine even if it is
+ // cancelled right after it is launched, so that finishTransition() is correctly called.
+ // Otherwise, this transition will never be stopped and we will never settle to Idle.
transition.job =
- launch { animatable.animateTo(targetProgress, animationSpec, initialVelocity) }
- .apply {
- invokeOnCompletion {
- // Settle the state to Idle(target). Note that this will do nothing if this
- // transition was replaced/interrupted by another one, and this also runs if
- // this coroutine is cancelled, i.e. if [this] coroutine scope is cancelled.
- layoutState.finishTransition(transition, targetScene)
- }
+ launch(start = CoroutineStart.ATOMIC) {
+ try {
+ animatable.animateTo(targetProgress, animationSpec, initialVelocity)
+ } finally {
+ layoutState.finishTransition(transition, targetScene)
}
+ }
return transition
}
diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/DraggableHandler.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/DraggableHandler.kt
index 6758990..1f81245 100644
--- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/DraggableHandler.kt
+++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/DraggableHandler.kt
@@ -33,6 +33,7 @@
import com.android.compose.nestedscroll.PriorityNestedScrollConnection
import kotlin.math.absoluteValue
import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.CoroutineStart
import kotlinx.coroutines.Job
import kotlinx.coroutines.launch
@@ -684,7 +685,11 @@
val isTargetGreater = targetOffset > animatable.value
val job =
coroutineScope
- .launch {
+ // Important: We start atomically to make sure that we start the coroutine even
+ // if it is cancelled right after it is launched, so that snapToScene() is
+ // correctly called. Otherwise, this transition will never be stopped and we
+ // will never settle to Idle.
+ .launch(start = CoroutineStart.ATOMIC) {
// TODO(b/327249191): Refactor the code so that we don't even launch a
// coroutine if we don't need to animate.
if (skipAnimation) {
@@ -726,18 +731,15 @@
}
} finally {
bouncingScene = null
+ snapToScene(targetScene)
}
}
- // Make sure that we settle to target scene at the end of the animation or if
- // the animation is cancelled.
- .apply { invokeOnCompletion { snapToScene(targetScene) } }
OffsetAnimation(animatable, job)
}
}
fun snapToScene(scene: SceneKey) {
- if (layoutState.transitionState != this) return
cancelOffsetAnimation()
layoutState.finishTransition(this, idleScene = scene)
}
diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt
index e0ec33c..a5b6d24 100644
--- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt
+++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt
@@ -497,11 +497,9 @@
transitionStates = listOf(transition)
}
is TransitionState.Transition -> {
- // Force the current transition to finish to currentScene.
- currentState.finish().invokeOnCompletion {
- // Make sure [finishTransition] is called at the end of the transition.
- finishTransition(currentState, currentState.currentScene)
- }
+ // Force the current transition to finish to currentScene. The transition will call
+ // [finishTransition] once it's finished.
+ currentState.finish()
val tooManyTransitions = transitionStates.size >= MAX_CONCURRENT_TRANSITIONS
val clearCurrentTransitions = !chain || tooManyTransitions