Skip to content

Commit

Permalink
LeaksTest (#2051)
Browse files Browse the repository at this point in the history
* LeaksTest

I recently worked on explicitly breaking reference cycles to
prevent objects from leaking when we mix garbage-collected
Kotlin objects with reference-counted Swift objects.

But this work was likely to regress because we didn't have a
mechanism to prevent these reference cycles from recurring.

This PR introduces a bunch of machinery to explicitly test
for reference cycles. It works on the JVM because that's a
capable platform for this kind of dynamic analysis, and
because it's consistent with the Kotlin/Native platform that
is actually where we need to defend against for leaks.

The first new class is JvmHeap, which lazily inspects an object
for its outbound references. This correctly captures the Kotlin
compiler-inserted indirect references. It also includes lots
of special cases to avoid traversing into implemnetation details
of kotlinx.serialization and coroutines.

The second new class is CycleFinder, which is a basic Dijkstra
breadth-first search. It's one neat feature is that it can
reconstruct the list of properties that participate in a cycle.
The cycle in LeaksTest (before the widget is removed) is:
     * callHandler
       * endpoint
         * inboundServices[3]
           * value
             * service
               * viewOrNull
                 * mutableListChildren
                   * container[0]
                     * onChange
                       * receiver
                         * eventSink
                           * delegate

This PR adds a test for widgets leaking. In a follow-up PR I intend
to add tests for other interfaces and extension points in Redwood that
can be implemented in Swift. This includes event listeners, Zipline services,
TreehouseContentSource, and CodeListener.

* Simplify JvmHeap stuff
  • Loading branch information
squarejesse authored May 29, 2024
1 parent df7b5bf commit 3988a9b
Show file tree
Hide file tree
Showing 10 changed files with 568 additions and 15 deletions.
5 changes: 5 additions & 0 deletions redwood-treehouse-host/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ kotlin {
implementation libs.robolectric
}
}
jvmTest {
if (!rootProject.hasProperty('redwoodNoApps')) {
kotlin.srcDir('src/appsJvmTest/kotlin')
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* 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 app.cash.redwood.treehouse

import app.cash.redwood.treehouse.leaks.LeakWatcher
import assertk.assertThat
import assertk.assertions.isEmpty
import assertk.assertions.isEqualTo
import com.example.redwood.testapp.testing.TextInputValue
import kotlin.test.Test
import kotlinx.coroutines.test.runTest

class LeaksTest {
@Test
fun widgetNotLeaked() = runTest {
val tester = TreehouseTester(this)
val treehouseApp = tester.loadApp()
val content = tester.content(treehouseApp)
val view = tester.view()

content.bind(view)

content.awaitContent(1)
val textInputValue = view.views.single() as TextInputValue
assertThat(textInputValue.text).isEqualTo("what would you like to see?")

val widgetLeakWatcher = LeakWatcher {
view.children.widgets.single()
}

// While the widget is in the UI, it's expected to be in a reference cycle.
widgetLeakWatcher.assertObjectInReferenceCycle()

textInputValue.onChange!!.invoke("Empty")

tester.sendFrame()
content.awaitContent(2)
assertThat(view.views).isEmpty()

// Once the widget is removed, the cycle must be broken and the widget must be unreachable.
widgetLeakWatcher.assertNotLeaked()

treehouseApp.stop()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* 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 app.cash.redwood.treehouse.leaks

import java.util.IdentityHashMap

/** Returns the shortest cycle involving [start], or null if it participates in no cycle. */
internal fun Heap.findCycle(start: Any): List<String>? {
val queue = ArrayDeque<Node>()
val nodes = IdentityHashMap<Any, Node>()

nodes[start] = Node(
targetEdges = references(start),
sourceEdge = null,
sourceNode = null,
).also {
queue += it
}

for (node in generateSequence { queue.removeFirstOrNull() }) {
for (edge in node.targetEdges) {
val instance = edge.instance ?: continue

if (instance === start) {
val result = ArrayDeque<String>()
result.addFirst(edge.name)
for (sourceNode in generateSequence(node) { it.sourceNode }) {
result.addFirst(sourceNode.sourceEdge?.name ?: break)
}
return result
}

nodes.getOrPut(instance) {
Node(
targetEdges = references(instance),
sourceEdge = edge,
sourceNode = node,
).also {
queue += it
}
}
}
}

return null
}

internal interface Heap {
fun references(instance: Any): List<Edge>
}

internal class Node(
val targetEdges: List<Edge>,
val sourceEdge: Edge?,
val sourceNode: Node?,
)

internal data class Edge(
val name: String,
val instance: Any?,
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* 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 app.cash.redwood.treehouse.leaks

import assertk.assertThat
import assertk.assertions.containsExactly
import assertk.assertions.isNotNull
import assertk.assertions.isNull
import org.junit.Test

internal class FindCycleTest {
@Test
fun cycle() {
val heap = object : Heap {
override fun references(instance: Any) = when (instance) {
"A" -> listOf(
Edge("b", "B"),
Edge("c", "C"),
)

"B" -> listOf(
Edge("d", "D"),
)

"C" -> listOf(
Edge("d", "D"),
)

"D" -> listOf(
Edge("a", "A"),
)

else -> listOf()
}
}

assertThat(heap.findCycle("A"))
.isNotNull()
.containsExactly("b", "d", "a")
assertThat(heap.findCycle("B"))
.isNotNull()
.containsExactly("d", "a", "b")
assertThat(heap.findCycle("C"))
.isNotNull()
.containsExactly("d", "a", "c")
assertThat(heap.findCycle("D"))
.isNotNull()
.containsExactly("a", "b", "d")
}

@Test
fun happyPathWithNoCycle() {
val heap = object : Heap {
override fun references(instance: Any) = when (instance) {
"A" -> listOf(
Edge("b", "B"),
Edge("c", "C"),
)

"B" -> listOf(
Edge("d", "D"),
)

"C" -> listOf(
Edge("d", "D"),
)

else -> listOf()
}
}

assertThat(heap.findCycle("A")).isNull()
assertThat(heap.findCycle("B")).isNull()
assertThat(heap.findCycle("C")).isNull()
}

@Test
fun directCycle() {
val heap = object : Heap {
override fun references(instance: Any) = when (instance) {
"A" -> listOf(
Edge("a", "A"),
)

else -> listOf()
}
}

assertThat(heap.findCycle("A"))
.isNotNull()
.containsExactly("a")
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* 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 app.cash.redwood.treehouse.leaks

import app.cash.redwood.treehouse.EventLog
import java.lang.reflect.Field
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.Job
import kotlinx.coroutines.flow.StateFlow
import kotlinx.serialization.KSerializer
import kotlinx.serialization.json.Json
import kotlinx.serialization.modules.SerializersModule

/**
* Inspect the current process heap using reflection.
*
* This is not a general-purpose heap API. It is specifically targeted to finding reference cycles
* and ignores types that do not participate in these.
*
* This attempts to avoid traversing into implementation details of platform types and library
* types that are typically reachable in the tests that use it. Keeping the list of known types
* up-to-date is simple and manual and potentially toilsome.
*/
internal object JvmHeap : Heap {
/** Attempt to avoid computing each type's declared fields on every single instance. */
private val classToFields = mutableMapOf<Class<*>, List<Field>>()

override fun references(instance: Any): List<Edge> {
val javaClass = instance::class.java
val javaPackageName = javaClass.`package`?.name ?: "?"

return when {
// Collection-like types reference their contents. Note that this doesn't consider Redwood's
// own collection implementations like Widget.Children, as these may have additional fields
// that we must include.
javaClass.isArray -> {
when (instance) {
is Array<*> -> references(instance.toList())
else -> listOf() // Primitive array.
}
}
instance is Collection<*> && javaPackageName.isDescendant("java", "kotlin") -> {
instance.mapIndexed { index, value -> Edge("[$index]", value) }
}
instance is Map<*, *> && javaPackageName.isDescendant("java", "kotlin") -> {
references(instance.entries)
}
instance is Map.Entry<*, *> && javaPackageName.isDescendant("java", "kotlin") -> listOf(
Edge("key", instance.key),
Edge("value", instance.value),
)
instance is StateFlow<*> -> listOf(
Edge("value", instance.value),
)

// Don't traverse further on types that are unlikely to contain application-scoped data.
// We want to avoid loading the entire application heap into memory!
instance is Class<*> -> listOf()
instance is CoroutineDispatcher -> listOf()
instance is Enum<*> -> listOf()
instance is EventLog -> listOf()
instance is Int -> listOf()
instance is Job -> listOf()
instance is Json -> listOf()
instance is KSerializer<*> -> listOf()
instance is SerializersModule -> listOf()
instance is String -> listOf()

// Explore everything else by reflecting on its fields.
javaPackageName.isDescendant(
"app.cash",
"com.example",
"kotlin",
"kotlinx.coroutines",
"okio",
) -> {
fields(javaClass).map { field -> Edge(field.name, field.get(instance)) }
}

else -> error("unexpected class needs to be added to JvmHeap.kt: $javaClass")
}
}

/**
* Returns true if this package is a descendant of any prefix. For example, the descendants of
* 'kotlin' are 'kotlin.collections' and 'kotlin' itself, but not 'kotlinx.coroutines'.
*/
private fun String.isDescendant(vararg prefixes: String) =
prefixes.any { prefix ->
startsWith(prefix) && (length == prefix.length || this[prefix.length] == '.')
}

private fun fields(type: Class<*>): List<Field> {
return classToFields.getOrPut(type) {
buildList {
for (supertype in type.supertypes) {
for (field in supertype.declaredFields) {
if (field.type.isPrimitive) continue // Ignore primitive fields.
try {
field.isAccessible = true
} catch (e: Exception) {
throw Exception("failed to set $type.${field.name} accessible", e)
}
add(field)
}
}
}
}
}

private val Class<*>.supertypes: Sequence<Class<*>>
get() = generateSequence(this) { it.superclass }
}
Loading

0 comments on commit 3988a9b

Please sign in to comment.