Start synchronizing with a File lock

First stab at hunting a few race conditions, I've put together some examples that try to expose race conditions but I've only found a scenario on `scheduleFrame()` for now. In any case, I've resorted to using a `ReentrantLock` on a `File` object, since everything originates from there, and trying to gate the most probable sources of resource contention, including `disposeDependencies()` on the `RiveArtboardRenderer`. I still need to take a look at the input queue. And, I'd like to remove some of the old `RiveArtboardRenderer` APIs that have been moved to the `RiveFileController`, some of which are already deprecated With this considerations, I'm pretty sure this will need a major bump. Diffs= c4a8015a8 `synchronize{}` with a ReentrantLock (#6170) Co-authored-by: Umberto Sonnino <[email protected]>
rive-app · Oct 31, 2023 · 7678d57 · 7678d57
1 parent 50edba6
commit 7678d57
Show file tree

Hide file tree

Showing 16 changed files with 189 additions and 147 deletions.
diff --git a/.rive_head b/.rive_head
@@ -1 +1 @@
-552801b54309992b18437e17ce7303ff11ac5fb8
+c4a8015a888d3b623951ee169087369bebd8fcea
diff --git a/app/src/main/java/app/rive/runtime/example/LowLevelActivity.kt b/app/src/main/java/app/rive/runtime/example/LowLevelActivity.kt
@@ -73,9 +73,7 @@ class LowLevelRiveView(context: Context) : RiveTextureView(context) {
                 artboard.let {
                     save()
                     align(Fit.COVER, Alignment.CENTER, RectF(0.0f, 0.0f, width, height), it.bounds)
-                    it.drawSkia(
-                        cppPointer
-                    )
+                    it.drawSkia(cppPointer)
                     restore()
                 }
             }

diff --git a/kotlin/src/androidTest/java/app/rive/runtime/kotlin/core/RiveViewLifecycleTest.kt b/kotlin/src/androidTest/java/app/rive/runtime/kotlin/core/RiveViewLifecycleTest.kt
@@ -220,7 +220,7 @@ class RiveViewLifecycleTest {
             // 'Open' the view - attached
             (mockView as TestUtils.MockRiveAnimationView).mockAttach()
 
-            val savedState = mockView.controller.saveControllerState()
+            val savedState = mockView.controller.saveControllerState()!!
             val file = savedState.file
             assertNotNull(file)
 
@@ -230,7 +230,7 @@ class RiveViewLifecycleTest {
             assertNull(mockView.artboardRenderer)
 
             savedState.dispose()
-            assertEquals(0, file?.refCount)
+            assertEquals(0, file.refCount)
         }
     }
 
@@ -243,7 +243,7 @@ class RiveViewLifecycleTest {
             // 'Open' the view - attached
             (mockView as TestUtils.MockRiveAnimationView).mockAttach()
 
-            val savedState = mockView.saveControllerState()
+            val savedState = mockView.saveControllerState()!!
             val file = savedState.file
             assertNotNull(file)
             // Make sure we cleaned up the old resource to avoid loading it twice.
@@ -252,7 +252,7 @@ class RiveViewLifecycleTest {
             // Let's 'close' this view - detached
             (mockView as TestUtils.MockRiveAnimationView).mockDetach(false)
             assertFalse(mockView.controller.isActive)
-            assertEquals(2, file?.refCount)
+            assertEquals(2, file.refCount)
 
             assertEquals(1, savedState.playingAnimations.size)
 
@@ -266,7 +266,7 @@ class RiveViewLifecycleTest {
 
             // Let's 'close' this view - detached
             (mockView as TestUtils.MockRiveAnimationView).mockDetach()
-            assertEquals(0, file?.refCount)
+            assertEquals(0, file.refCount)
             // No need to call `savedState.dispose()` because the state has already been restored.
         }
     }

diff --git a/kotlin/src/main/java/app/rive/runtime/kotlin/RiveAnimationView.kt b/kotlin/src/main/java/app/rive/runtime/kotlin/RiveAnimationView.kt
@@ -812,7 +812,6 @@ open class RiveAnimationView(context: Context, attrs: AttributeSet? = null) :
      * Called from TextureView.onAttachedToWindow() - override for implementing a custom renderer.
      */
     override fun createRenderer(): Renderer {
-        // Make the Renderer again every time this is visible and reset its state.
         return RiveArtboardRenderer(
             trace = rendererAttributes.riveTraceAnimations,
             controller = controller,
@@ -875,7 +874,7 @@ open class RiveAnimationView(context: Context, attrs: AttributeSet? = null) :
     }
 
     @ControllerStateManagement
-    fun saveControllerState(): ControllerState {
+    fun saveControllerState(): ControllerState? {
         // Invalidate the old resource to prevent it from loading again.
         rendererAttributes.resource = null
         return controller.saveControllerState()
@@ -1042,9 +1041,9 @@ class RiveViewLifecycleObserver(private val dependencies: List<RefCount>) :
     override fun onStop(owner: LifecycleOwner) {}
 
     /**
-     * DefaultLifecycleObserver.onDestroy() is called when the LifecycleOwner's onDestroy() method
+     * [DefaultLifecycleObserver] [onDestroy()] is called when the [LifecycleOwner]'s [onDestroy()] method
      * is called.
-     * This typically happens when the Activity or Fragment is in the process of being permanently
+     * This typically happens when the [Activity] or [Fragment] is in the process of being permanently
      * destroyed.
      */
     @CallSuper

diff --git a/kotlin/src/main/java/app/rive/runtime/kotlin/RiveArtboardRenderer.kt b/kotlin/src/main/java/app/rive/runtime/kotlin/RiveArtboardRenderer.kt
@@ -4,11 +4,9 @@ import android.graphics.PointF
 import android.graphics.RectF
 import androidx.annotation.WorkerThread
 import app.rive.runtime.kotlin.controllers.RiveFileController
-import app.rive.runtime.kotlin.core.Alignment
 import app.rive.runtime.kotlin.core.Artboard
 import app.rive.runtime.kotlin.core.Direction
 import app.rive.runtime.kotlin.core.File
-import app.rive.runtime.kotlin.core.Fit
 import app.rive.runtime.kotlin.core.Helpers
 import app.rive.runtime.kotlin.core.Loop
 import app.rive.runtime.kotlin.core.PlayableInstance
@@ -23,11 +21,6 @@ enum class PointerEvents {
 }
 
 open class RiveArtboardRenderer(
-    // PUBLIC
-    fit: Fit = Fit.CONTAIN,
-    alignment: Alignment = Alignment.CENTER,
-    loop: Loop = Loop.AUTO,
-    autoplay: Boolean = true,
     // TODO: would love to get rid of these three fields here.
     var artboardName: String? = null,
     var animationName: String? = null,
@@ -39,8 +32,8 @@ open class RiveArtboardRenderer(
     private var controller: RiveFileController = controller.also {
         it.onStart = ::start
         it.acquire()
-        // Add controller and its file to dependencies.
-        // This guarantees that when the renderer is disposed, it will `.release()` them.
+        // Add controller to the Renderer dependencies.
+        // When the renderer is disposed, it'll `release()` `it`
         dependencies.add(it)
     }
 
@@ -88,9 +81,7 @@ open class RiveArtboardRenderer(
             return
         }
         if (controller.isActive) {
-            activeArtboard?.drawSkia(
-                cppPointer, fit, alignment
-            )
+            activeArtboard?.drawSkia(cppPointer, fit, alignment)
         }
     }
 
@@ -339,11 +330,17 @@ open class RiveArtboardRenderer(
 
             }
         } else {
+            // (umberto) pretty sure this API is unused, but it might need deprecation and removal
             this.activeArtboard?.advance(0f)
             start()
         }
     }
 
+    override fun disposeDependencies() {
+        // Lock to make sure things are disposed in an orderly manner.
+        synchronized(file?.lock ?: this) { super.disposeDependencies() }
+    }
+
     @Deprecated(
         "Use RiveFileController.Listener",
         level = WARNING,

diff --git a/kotlin/src/main/java/app/rive/runtime/kotlin/controllers/RiveFileController.kt b/kotlin/src/main/java/app/rive/runtime/kotlin/controllers/RiveFileController.kt
@@ -34,17 +34,17 @@ annotation class ControllerStateManagement
  */
 @ControllerStateManagement
 class ControllerState internal constructor(
-    val file: File?,
-    val activeArtboard: Artboard?,
+    val file: File,
+    val activeArtboard: Artboard,
     val animations: List<LinearAnimationInstance>,
     val playingAnimations: HashSet<LinearAnimationInstance>,
     val stateMachines: List<StateMachineInstance>,
     val playingStateMachines: HashSet<StateMachineInstance>,
     val isActive: Boolean,
 ) {
     fun dispose() {
-        file?.release()
-        activeArtboard?.release()
+        file.release()
+        activeArtboard.release()
     }
 }
 
@@ -82,15 +82,18 @@ class RiveFileController(
             if (value == field) {
                 return
             }
-            // If we have an old file remove all the old values.
-            field?.let {
-                reset()
-                it.release()
+
+            synchronized(field?.lock ?: this) {
+                // If we have an old file remove all the old values.
+                field?.let {
+                    reset()
+                    it.release()
+                }
+                field = value
+                // We only need to acquire the reference to the [file] since all the other components
+                // will be fetched from this file (and will, in fact, become a dependency of [file])
+                field?.acquire()
             }
-            field = value
-            // We only need to acquire the reference to the [file] since all the other components
-            // will be fetched from this file (and will, in fact, become a dependency of [file])
-            field?.acquire()
         }
 
     var activeArtboard: Artboard? = activeArtboard
@@ -162,15 +165,27 @@ class RiveFileController(
     /**
      * Get a copy of the State of this Controller and acquire a reference to the File to prevent
      * it being released from memory.
+     *
+     * Returns a [ControllerState] object with everything the controller was using.
+     * If the controller is pointing to stale data, it'll return null
      */
     @ControllerStateManagement
-    fun saveControllerState(): ControllerState {
-        // Acquire the file & artboard to prevent it from being released.
-        this.file?.acquire()
-        this.activeArtboard?.acquire()
+    fun saveControllerState(): ControllerState? {
+        val mFile = this.file ?: return null
+        val mArtboard = this.activeArtboard ?: return null
+        synchronized(mFile.lock) {
+            // This resource had already been released.
+            if (!mFile.hasCppObject) {
+                return null
+            }
+            // Acquire the file & artboard to prevent dispose().
+            mFile.acquire()
+            mArtboard.acquire()
+        }
+
         return ControllerState(
-            file,
-            activeArtboard,
+            mFile,
+            mArtboard,
             // Duplicate contents to grab a reference to the instances.
             animations = animationList.toList(),
             playingAnimations = playingAnimations.toHashSet(),
@@ -181,8 +196,10 @@ class RiveFileController(
     }
 
     /**
-     * Restore a copy of the state on this Controller.
-     * It also releases the File reference that was acquired when creating this file.
+     * Restore a copy of the state to this Controller.
+     *
+     * It also [release()]s any resources currently associated with this Controller in favor of the
+     * ones stored on the [state]
      */
     @ControllerStateManagement
     fun restoreControllerState(state: ControllerState) {
@@ -204,34 +221,37 @@ class RiveFileController(
     /// be aware of thread safety!
     @WorkerThread
     fun advance(elapsed: Float) {
-        activeArtboard?.let { ab ->
-            // animations could change, lets cut a list.
-            // order of animations is important.....
-            animations.forEach { animationInstance ->
-                if (playingAnimations.contains(animationInstance)) {
-                    val looped = animationInstance.advance(elapsed)
-                    animationInstance.apply()
-
-                    if (looped == Loop.ONESHOT) {
-                        stop(animationInstance)
-                    } else if (looped != null) {
-                        notifyLoop(animationInstance)
+        val mLock = this.file?.lock ?: return
+        synchronized(mLock) {
+            activeArtboard?.let { ab ->
+                // animations could change, lets cut a list.
+                // order of animations is important.....
+                animations.forEach { animationInstance ->
+                    if (playingAnimations.contains(animationInstance)) {
+                        val looped = animationInstance.advance(elapsed)
+                        animationInstance.apply()
+
+                        if (looped == Loop.ONESHOT) {
+                            stop(animationInstance)
+                        } else if (looped != null) {
+                            notifyLoop(animationInstance)
+                        }
                     }
                 }
-            }
 
-            stateMachines.forEach { stateMachineInstance ->
-                if (playingStateMachines.contains(stateMachineInstance)) {
-                    val stillPlaying =
-                        resolveStateMachineAdvance(stateMachineInstance, elapsed)
+                stateMachines.forEach { stateMachineInstance ->
+                    if (playingStateMachines.contains(stateMachineInstance)) {
+                        val stillPlaying =
+                            resolveStateMachineAdvance(stateMachineInstance, elapsed)
 
-                    if (!stillPlaying) {
-                        pause(stateMachineInstance)
+                        if (!stillPlaying) {
+                            pause(stateMachineInstance)
+                        }
                     }
                 }
+                ab.advance(elapsed)
+                notifyAdvance(elapsed)
             }
-            ab.advance(elapsed)
-            notifyAdvance(elapsed)
         }
     }
 
@@ -277,30 +297,30 @@ class RiveFileController(
     }
 
     /**
-     * Use the parameters in [attrs] to initialize [activeArtboard] and any animation/state machine
-     * specified by [attrs] animationName or stateMachine name.
+     * Use the parameters in [rendererAttributes] to initialize [activeArtboard] and any animation/state machine
+     * specified by [rendererAttributes] animationName or stateMachine name.
      *
      */
-    internal fun setupScene(attrs: RiveAnimationView.RendererAttributes) {
+    internal fun setupScene(rendererAttributes: RiveAnimationView.RendererAttributes) {
         val mFile = file
         if (mFile == null) {
             Log.w(TAG, "Cannot init without a file")
             return
         }
         // If anything has been previously set up, remove it.
         reset()
-        autoplay = attrs.autoplay
-        alignment = attrs.alignment
-        fit = attrs.fit
-        loop = attrs.loop
+        autoplay = rendererAttributes.autoplay
+        alignment = rendererAttributes.alignment
+        fit = rendererAttributes.fit
+        loop = rendererAttributes.loop
 
-        val abName = attrs.artboardName
+        val abName = rendererAttributes.artboardName
 
         this.activeArtboard = if (abName != null) mFile.artboard(abName) else mFile.firstArtboard
 
         if (autoplay) {
-            val animName = attrs.animationName
-            val smName = attrs.stateMachineName
+            val animName = rendererAttributes.animationName
+            val smName = rendererAttributes.stateMachineName
 
             if (animName != null) {
                 play(animName)
@@ -520,9 +540,6 @@ class RiveFileController(
         }
     }
 
-    // Method is synchronized because it's used by play() from the UI thread as well as
-    // advance() on the Worker thread, so we need to avoid race conditions.
-    @Synchronized
     private fun resolveStateMachineAdvance(
         stateMachineInstance: StateMachineInstance,
         elapsed: Float
@@ -695,7 +712,6 @@ class RiveFileController(
      * Release a reference associated with this Controller.
      * If [refs] == 0, then give up all resources and [release] the file
      */
-    @Synchronized
     override fun release(): Int {
         val count = super.release()
         require(count >= 0)
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		552801b54309992b18437e17ce7303ff11ac5fb8
		c4a8015a888d3b623951ee169087369bebd8fcea