Skip to content

Commit d9a8030

Browse files
ErikUggeldahlErikUggeldahl
committed
fix: Backport legacy VMI read/write lock
Selectively backport the legacy runtime locking fix while excluding unrelated new API, Vulkan, capped-FPS, sample, and C++ runtime submodule changes bundled in the downstream mirror commit. (cherry picked from commit 7cc8317)
1 parent 0ec7f12 commit d9a8030

12 files changed

Lines changed: 605 additions & 153 deletions

File tree

‎kotlin/src/androidTest/kotlin/app/rive/runtime/kotlin/core/RiveArtboardRendererTest.kt‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ class RiveArtboardRendererTest {
7272
val afterDeleteLatch = CountDownLatch(1)
7373

7474
// The renderer needs a valid artboard to proceed to accessing the cppPointer
75-
val dummyArtboard = object : Artboard(unsafeCppPointer = 1L, lock = ReentrantLock()) {
75+
val dummyArtboard = object : Artboard(unsafeCppPointer = 1L, fileLock = ReentrantLock()) {
7676
// Override draw to do nothing, avoiding the thread affinity checks in the real draw().
7777
override fun draw(
7878
rendererAddress: Long,
@@ -138,12 +138,12 @@ class RiveArtboardRendererTest {
138138
// Signals that the main thread has released the artboard, and draw() can continue.
139139
val afterRelease = CountDownLatch(1)
140140

141-
// A lock we can reference, unlike the default private Artboard lock.
141+
// A lock we can reference, unlike the default private Artboard file lock.
142142
val artboardLock = ReentrantLock()
143143

144144
// An artboard that synchronizes with the main thread to simulate being released while
145145
// being drawn.
146-
val latchingArtboard = object : Artboard(unsafeCppPointer = 1L, lock = artboardLock) {
146+
val latchingArtboard = object : Artboard(unsafeCppPointer = 1L, fileLock = artboardLock) {
147147
override fun draw(
148148
rendererAddress: Long,
149149
fit: Fit,
@@ -209,12 +209,12 @@ class RiveArtboardRendererTest {
209209
// Signals that the main thread has released the artboard, and draw() can continue.
210210
val afterRelease = CountDownLatch(1)
211211

212-
// A lock we can reference, unlike the default private Artboard lock.
212+
// A lock we can reference, unlike the default private Artboard file lock.
213213
val artboardLock = ReentrantLock()
214214

215215
// An artboard that synchronizes with the main thread to simulate being released right
216216
// before being drawn.
217-
val latchingArtboard = object : Artboard(unsafeCppPointer = 1L, lock = artboardLock) {
217+
val latchingArtboard = object : Artboard(unsafeCppPointer = 1L, fileLock = artboardLock) {
218218
override fun draw(
219219
rendererAddress: Long,
220220
fit: Fit,

‎kotlin/src/androidTest/kotlin/app/rive/runtime/kotlin/core/RiveControllerTest.kt‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -182,8 +182,8 @@ class RiveControllerTest {
182182
// No assertions. The intent is to run without throwing a NoSuchElementException.
183183
}
184184

185-
private class ArtboardSpy(unsafeCppPointer: Long, lock: ReentrantLock) :
186-
Artboard(unsafeCppPointer, lock) {
185+
private class ArtboardSpy(unsafeCppPointer: Long, fileLock: ReentrantLock) :
186+
Artboard(unsafeCppPointer, fileLock) {
187187
final var advanceCount = 0
188188
private set
189189

@@ -204,7 +204,7 @@ class RiveControllerTest {
204204
if (artboardPointer == NULL_POINTER) {
205205
throw ArtboardException("No Artboard found at index $index.")
206206
}
207-
val ab = ArtboardSpy(artboardPointer, lock) // Instantiate the spy
207+
val ab = ArtboardSpy(artboardPointer, fileLock) // Instantiate the spy
208208
dependencies.add(ab)
209209
return ab
210210
}

‎kotlin/src/androidTest/kotlin/app/rive/runtime/kotlin/core/RiveDataBindingTest.kt‎

Lines changed: 118 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import java.util.concurrent.CountDownLatch
1616
import java.util.concurrent.TimeUnit
1717
import java.util.concurrent.atomic.AtomicBoolean
1818
import java.util.concurrent.atomic.AtomicReference
19+
import java.util.concurrent.locks.ReentrantLock
1920
import kotlin.concurrent.thread
2021
import kotlin.io.encoding.Base64
2122
import kotlin.io.encoding.ExperimentalEncodingApi
@@ -344,7 +345,8 @@ class RiveDataBindingTest {
344345
@Test
345346
fun empty_vm() {
346347
val vm = view.controller.file?.getViewModelByName("Empty VM")!!
347-
assertEquals(0, vm.instanceCount)
348+
// All VMs have a default, undeletable instance, so instance count is 1 even for an empty VM
349+
assertEquals(1, vm.instanceCount)
348350
assertEquals(0, vm.propertyCount)
349351
}
350352

@@ -1356,8 +1358,9 @@ class RiveDataBindingTest {
13561358
class LatchedViewModelInstance(
13571359
private val iterationStarted: CountDownLatch,
13581360
private val mapMutated: CountDownLatch,
1359-
unsafeCppPointer: Long
1360-
) : ViewModelInstance(unsafeCppPointer) {
1361+
unsafeCppPointer: Long,
1362+
fileLock: ReentrantLock
1363+
) : ViewModelInstance(unsafeCppPointer, fileLock) {
13611364
/**
13621365
* This version creates an iterator over the map and holds it. If the operation
13631366
* is unsafe, any structural change while this iterator is in use will cause a
@@ -1388,14 +1391,16 @@ class RiveDataBindingTest {
13881391
class LatchedViewModel(
13891392
private val iterationStarted: CountDownLatch,
13901393
private val mapMutated: CountDownLatch,
1391-
unsafeCppPointer: Long
1392-
) : ViewModel(unsafeCppPointer) {
1394+
unsafeCppPointer: Long,
1395+
fileLock: ReentrantLock
1396+
) : ViewModel(unsafeCppPointer, fileLock) {
13931397
override fun createBlankInstance(): ViewModelInstance {
13941398
val instancePointer = cppCreateBlankInstance(cppPointer)
13951399
return LatchedViewModelInstance(
13961400
iterationStarted,
13971401
mapMutated,
1398-
instancePointer
1402+
instancePointer,
1403+
fileLock
13991404
).also {
14001405
dependencies.add(it)
14011406
}
@@ -1412,7 +1417,8 @@ class RiveDataBindingTest {
14121417
return LatchedViewModel(
14131418
iterationStarted,
14141419
mapMutated,
1415-
vm
1420+
vm,
1421+
fileLock
14161422
).also { dependencies.add(it) }
14171423
}
14181424
}
@@ -1522,4 +1528,109 @@ class RiveDataBindingTest {
15221528
// Rethrow any exception from either thread.
15231529
failure.get()?.let { throw it }
15241530
}
1531+
1532+
@Test
1533+
fun set_view_model_property_while_advancing() {
1534+
view.setRiveResource(R.raw.data_bind_test_impl, "Test Observation", autoBind = true)
1535+
view.play()
1536+
1537+
val vmi = view.controller.stateMachines.first().viewModelInstance!!
1538+
val numberProperty = vmi.getNumberProperty("Test Num")
1539+
val stringProperty = vmi.getStringProperty("Test String")
1540+
val booleanProperty = vmi.getBooleanProperty("Test Bool")
1541+
val enumProperty = vmi.getEnumProperty("Test Enum")
1542+
val colorProperty = vmi.getColorProperty("Test Color")
1543+
val triggerProperty = vmi.getTriggerProperty("Test Trigger")
1544+
val nestedNumberProperty = vmi.getNumberProperty("Test Nested/Nested Number")
1545+
1546+
// Latch to start the worker thread at the same time as the property mutation loop.
1547+
val start = CountDownLatch(1)
1548+
// Stop marker - first to reach their iteration limit or exception will set this to stop the
1549+
// other.
1550+
val stop = AtomicBoolean(false)
1551+
// Capture any exception from either thread to rethrow on the main thread after joining.
1552+
val failure = AtomicReference<Throwable?>(null)
1553+
1554+
// Simulate renderer's worker thread advances. MockRiveAnimationView does not use the real
1555+
// renderer frame loop, so the test drives controller.advance() directly.
1556+
val advanceThread = thread(name = "rive-advance-worker") {
1557+
try {
1558+
start.await()
1559+
for (i in 0 until 500_000) {
1560+
if (stop.get()) {
1561+
break
1562+
}
1563+
if (i % 3 == 0) {
1564+
// Cycle the state machine between VM-change states so worker advance
1565+
// repeatedly applies bound output changes while writer threads mutate VMI
1566+
// properties.
1567+
view.controller.fireState("Output", "Advance")
1568+
}
1569+
view.controller.advance(0f)
1570+
view.controller.advance(1f / 240f)
1571+
if (i % 8 == 0) {
1572+
// Give writer threads more chances to interleave with worker advancement.
1573+
Thread.yield()
1574+
}
1575+
}
1576+
} catch (throwable: Throwable) {
1577+
failure.compareAndSet(null, throwable)
1578+
} finally {
1579+
stop.set(true)
1580+
}
1581+
}
1582+
1583+
val writerThreads = (0 until 4).map { writerIndex ->
1584+
thread(name = "rive-vmi-writer-$writerIndex") {
1585+
try {
1586+
start.await()
1587+
for (i in 0 until 500_000) {
1588+
if (stop.get()) {
1589+
break
1590+
}
1591+
val value = (i + writerIndex * 1000).toFloat()
1592+
numberProperty.value = value
1593+
stringProperty.value = "writer-$writerIndex-$i"
1594+
booleanProperty.value = i % 2 == 0
1595+
enumProperty.value = if (i % 2 == 0) "Value 1" else "Value 2"
1596+
colorProperty.value = 0xFF000000.toInt() or (i and 0x00FFFFFF)
1597+
nestedNumberProperty.value = value + 1f
1598+
triggerProperty.trigger()
1599+
if (i % 8 == 0) {
1600+
Thread.yield()
1601+
}
1602+
}
1603+
} catch (throwable: Throwable) {
1604+
failure.compareAndSet(null, throwable)
1605+
} finally {
1606+
stop.set(true)
1607+
}
1608+
}
1609+
}
1610+
1611+
// Allow all threads to begin together.
1612+
start.countDown()
1613+
1614+
val joinTimeout = TimeUnit.SECONDS.toMillis(30)
1615+
advanceThread.join(joinTimeout)
1616+
if (advanceThread.isAlive) {
1617+
stop.set(true)
1618+
advanceThread.interrupt()
1619+
advanceThread.join(joinTimeout)
1620+
}
1621+
writerThreads.forEach { writerThread ->
1622+
writerThread.join(joinTimeout)
1623+
if (writerThread.isAlive) {
1624+
stop.set(true)
1625+
writerThread.interrupt()
1626+
writerThread.join(joinTimeout)
1627+
}
1628+
}
1629+
assertFalse(advanceThread.isAlive, "advance thread timed out")
1630+
writerThreads.forEach { writerThread ->
1631+
assertFalse(writerThread.isAlive, "${writerThread.name} timed out")
1632+
}
1633+
// Rethrow any exception from either thread.
1634+
failure.get()?.let { throw it }
1635+
}
15251636
}

‎kotlin/src/androidTest/kotlin/app/rive/runtime/kotlin/core/RiveMemoryTests.kt‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -399,9 +399,9 @@ class RiveMemoryTests {
399399
// Allows override of the draw method to synchronize with the main thread
400400
class PhasedArtboard(
401401
unsafeCppPointer: Long,
402-
lock: ReentrantLock,
402+
fileLock: ReentrantLock,
403403
private val phaser: Phaser,
404-
) : Artboard(unsafeCppPointer, lock) {
404+
) : Artboard(unsafeCppPointer, fileLock) {
405405
override fun draw(
406406
rendererAddress: Long,
407407
fit: Fit,
@@ -434,7 +434,7 @@ class RiveMemoryTests {
434434
class PhasedFile(bytes: ByteArray, private val phaser: Phaser) : File(bytes) {
435435
override fun artboard(index: Int): Artboard {
436436
val artboardPointer = cppArtboardByIndex(cppPointer, index)
437-
val ab = PhasedArtboard(artboardPointer, lock, phaser)
437+
val ab = PhasedArtboard(artboardPointer, fileLock, phaser)
438438
dependencies.add(ab)
439439
return ab
440440
}

‎kotlin/src/androidTest/res/raw/data_bind_test_impl.riv‎

74 Bytes
Binary file not shown.

‎kotlin/src/main/java/app/rive/runtime/kotlin/controllers/RiveFileController.kt‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ class RiveFileController internal constructor(
165165
return
166166
}
167167

168-
synchronized(field?.lock ?: this) {
168+
synchronized(field?.fileLock ?: this) {
169169
// If we have an old file remove all the old values.
170170
field?.let {
171171
RiveLog.d(TAG) { "File set; releasing old file: $it" }
@@ -185,7 +185,7 @@ class RiveFileController internal constructor(
185185
if (value == field) {
186186
return
187187
}
188-
synchronized(file?.lock ?: this) {
188+
synchronized(file?.fileLock ?: this) {
189189
RiveLog.d(TAG) { "Artboard set; releasing old artboard (if it exists): $field" }
190190
field?.release()
191191
field = value
@@ -268,7 +268,7 @@ class RiveFileController internal constructor(
268268
fun saveControllerState(): ControllerState? {
269269
val mFile = this.file ?: return null
270270
val mArtboard = this.activeArtboard ?: return null
271-
synchronized(mFile.lock) {
271+
synchronized(mFile.fileLock) {
272272
// This resource had already been released.
273273
if (!mFile.hasCppObject) {
274274
return null
@@ -298,7 +298,7 @@ class RiveFileController internal constructor(
298298
*/
299299
@ControllerStateManagement
300300
fun restoreControllerState(state: ControllerState) {
301-
synchronized(file?.lock ?: this) {
301+
synchronized(file?.fileLock ?: this) {
302302
// Remove all old values.
303303
reset()
304304
// Restore all the previous values.
@@ -323,7 +323,7 @@ class RiveFileController internal constructor(
323323
@WorkerThread
324324
fun advance(elapsed: Float) {
325325
// We need a file to advance.
326-
val mLock = this.file?.lock ?: return
326+
val mLock = this.file?.fileLock ?: return
327327
synchronized(mLock) {
328328
activeArtboard?.let { ab ->
329329
// Process all the inputs right away.
@@ -684,7 +684,7 @@ class RiveFileController internal constructor(
684684
private fun processAllInputs() {
685685
// Gather all state machines that need playing and do that only once.
686686
val playableSet = mutableSetOf<StateMachineInstance>()
687-
// No need to lock this: this is being called from `advance()` which is `synchronized(file)`
687+
// No need to lock this: this is being called from `advance()` which holds the file lock.
688688
while (changedInputs.isNotEmpty()) {
689689
// There is a small chance that the queue will be emptied by another thread before removing.
690690
// Null checking the removed item protects against that scenario.

‎kotlin/src/main/java/app/rive/runtime/kotlin/core/Artboard.kt‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,14 @@ import java.util.concurrent.locks.ReentrantLock
1919
* canvas.
2020
*
2121
* @param unsafeCppPointer Pointer to the C++ counterpart.
22-
* @param lock A lock that is used to synchronize access to the underlying C++ object.
22+
* @param fileLock Lock shared by the [File] and native graph this artboard belongs to.
2323
* @param file The [File] that created this artboard. Used only to promote an artboard to a bindable
2424
* artboard.
2525
*/
2626
@OpenForTesting
2727
class Artboard(
2828
unsafeCppPointer: Long,
29-
private val lock: ReentrantLock,
29+
private val fileLock: ReentrantLock,
3030
internal val file: File? = null
3131
) :
3232
NativeObject(unsafeCppPointer) {
@@ -97,7 +97,7 @@ class Artboard(
9797

9898
external override fun cppDelete(pointer: Long)
9999

100-
override fun release(): Int = synchronized(lock) {
100+
override fun release(): Int = synchronized(fileLock) {
101101
super.release()
102102
}
103103

@@ -127,7 +127,7 @@ class Artboard(
127127
if (animationPointer == NULL_POINTER) {
128128
throw AnimationException("No Animation found at index $index.")
129129
}
130-
val lai = LinearAnimationInstance(animationPointer, lock)
130+
val lai = LinearAnimationInstance(animationPointer, fileLock)
131131
dependencies.add(lai)
132132
return lai
133133
}
@@ -146,7 +146,7 @@ class Artboard(
146146
"Available Animations: ${animationNames.map { "\"$it\"" }}\""
147147
)
148148
}
149-
val lai = LinearAnimationInstance(animationPointer, lock)
149+
val lai = LinearAnimationInstance(animationPointer, fileLock)
150150
dependencies.add(lai)
151151
return lai
152152
}
@@ -173,7 +173,7 @@ class Artboard(
173173
if (stateMachinePointer == NULL_POINTER) {
174174
throw StateMachineException("No StateMachine found at index $index.")
175175
}
176-
val smi = StateMachineInstance(stateMachinePointer, lock)
176+
val smi = StateMachineInstance(stateMachinePointer, fileLock)
177177
dependencies.add(smi)
178178
return smi
179179
}
@@ -189,7 +189,7 @@ class Artboard(
189189
if (stateMachinePointer == NULL_POINTER) {
190190
throw StateMachineException("No StateMachine found with name $name.")
191191
}
192-
val smi = StateMachineInstance(stateMachinePointer, lock)
192+
val smi = StateMachineInstance(stateMachinePointer, fileLock)
193193
dependencies.add(smi)
194194
return smi
195195
}
@@ -311,7 +311,8 @@ class Artboard(
311311
var viewModelInstance: ViewModelInstance? = null
312312
set(value) {
313313
value?.let {
314-
synchronized(lock) {
314+
it.updateFileLock(fileLock)
315+
synchronized(fileLock) {
315316
// Binding mutates the native graph and must not overlap with advance().
316317
cppSetViewModelInstance(cppPointer, it.cppPointer)
317318
field = value
@@ -347,11 +348,11 @@ class Artboard(
347348
* [elapsedTime] is currently not taken into account.
348349
*/
349350
fun advance(elapsedTime: Float): Boolean =
350-
synchronized(lock) { cppAdvance(cppPointer, elapsedTime) }
351+
synchronized(fileLock) { cppAdvance(cppPointer, elapsedTime) }
351352

352353
/** Draw the the artboard to the [renderer][app.rive.runtime.kotlin.renderers.Renderer]. */
353354
@WorkerThread
354-
fun draw(rendererAddress: Long) = synchronized(lock) {
355+
fun draw(rendererAddress: Long) = synchronized(fileLock) {
355356
if (!hasCppObject) return
356357
cppDraw(cppPointer, rendererAddress)
357358
}
@@ -362,7 +363,7 @@ class Artboard(
362363
*/
363364
@WorkerThread
364365
fun draw(rendererAddress: Long, fit: Fit, alignment: Alignment, scaleFactor: Float = 1.0f) =
365-
synchronized(lock) {
366+
synchronized(fileLock) {
366367
if (!hasCppObject) return
367368

368369
cppDrawAligned(

0 commit comments

Comments
 (0)