From 3cdbaf870c3c7933dccb32a5e5fec35a470056ab Mon Sep 17 00:00:00 2001 From: Roman Elizarov Date: Wed, 12 Feb 2020 11:30:50 +0300 Subject: [PATCH 1/2] Improve FieldWalker, don't access JDK classes * Works on future JDKs that forbid reflective access to JDK classes * Show human-readable path to field is something fails --- .../jvm/test/FieldWalker.kt | 181 +++++++++++------- .../ReusableCancellableContinuationTest.kt | 20 +- .../jvm/test/flow/ConsumeAsFlowLeakTest.kt | 2 +- 3 files changed, 120 insertions(+), 83 deletions(-) diff --git a/kotlinx-coroutines-core/jvm/test/FieldWalker.kt b/kotlinx-coroutines-core/jvm/test/FieldWalker.kt index bb8b855498..4c19040d9b 100644 --- a/kotlinx-coroutines-core/jvm/test/FieldWalker.kt +++ b/kotlinx-coroutines-core/jvm/test/FieldWalker.kt @@ -7,109 +7,146 @@ package kotlinx.coroutines import java.lang.reflect.* import java.util.* import java.util.Collections.* +import java.util.concurrent.atomic.* import kotlin.collections.ArrayList +import kotlin.test.* object FieldWalker { + sealed class Ref { + object RootRef : Ref() + class FieldRef(val parent: Any, val name: String) : Ref() + class ArrayRef(val parent: Any, val index: Int) : Ref() + } + + private val fieldsCache = HashMap, List>() + + init { + // excluded/terminal classes (don't walk them) + fieldsCache += listOf(Any::class, String::class, Thread::class, Throwable::class) + .map { it.java } + .associateWith { emptyList() } + } /* * Reflectively starts to walk through object graph and returns identity set of all reachable objects. + * Use [walkRefs] if you need a path from root for debugging. + */ + public fun walk(root: Any?): Set = walkRefs(root).keys + + public fun assertReachableCount(expected: Int, root: Any?, predicate: (Any) -> Boolean) { + val visited = walkRefs(root) + val actual = visited.keys.filter(predicate) + if (actual.size != expected) { + val text = actual.joinToString("\n") { "\t" + showPath(it, visited) } + assertEquals(expected, actual.size, "Unexpected number objects. Expected $expected, found ${actual.size}:\n$text") + } + + } + + /* + * Reflectively starts to walk through object graph and map to all the reached object to their path + * in from root. Use [showPath] do display a path if needed. */ - public fun walk(root: Any): Set { - val result = newSetFromMap(IdentityHashMap()) - result.add(root) + private fun walkRefs(root: Any?): Map { + val visited = IdentityHashMap() + if (root == null) return visited + visited.put(root, Ref.RootRef) val stack = ArrayDeque() stack.addLast(root) while (stack.isNotEmpty()) { val element = stack.removeLast() - val type = element.javaClass - type.visit(element, result, stack) + try { + visit(element, visited, stack) + } catch (e: Exception) { + error("Failed to visit element ${showPath(element, visited)}: $e") + } } - return result + return visited } - private fun Class<*>.visit( - element: Any, - result: MutableSet, - stack: ArrayDeque - ) { - val fields = fields() - fields.forEach { - it.isAccessible = true - val value = it.get(element) ?: return@forEach - if (result.add(value)) { - stack.addLast(value) + private fun showPath(element: Any, visited: Map): String { + val path = ArrayList() + var cur = element + while (true) { + val ref = visited[cur]!! + if (ref is Ref.RootRef) break + when (ref) { + is Ref.FieldRef -> { + cur = ref.parent + path += ".${ref.name}" + } + is Ref.ArrayRef -> { + cur = ref.parent + path += "[${ref.index}]" + } } } + path.reverse() + return path.joinToString("") + } - if (isArray && !componentType.isPrimitive) { - val array = element as Array - array.filterNotNull().forEach { - if (result.add(it)) { - stack.addLast(it) + private fun visit(element: Any, visited: IdentityHashMap, stack: ArrayDeque) { + val type = element.javaClass + when { + // Special code for arrays + type.isArray && !type.componentType.isPrimitive -> { + @Suppress("UNCHECKED_CAST") + val array = element as Array + array.forEachIndexed { index, value -> + push(value, visited, stack) { Ref.ArrayRef(element, index) } + } + } + // Special code for platform types that cannot be reflectively accessed on modern JDKs + type.name.startsWith("java.") && element is Collection<*> -> { + element.forEachIndexed { index, value -> + push(value, visited, stack) { Ref.ArrayRef(element, index) } + } + } + type.name.startsWith("java.") && element is Map<*, *> -> { + push(element.keys, visited, stack) { Ref.FieldRef(element, "keys") } + push(element.values, visited, stack) { Ref.FieldRef(element, "values") } + } + element is AtomicReference<*> -> { + push(element.get(), visited, stack) { Ref.FieldRef(element, "value") } + } + // All the other classes are reflectively scanned + else -> fields(type).forEach { field -> + push(field.get(element), visited, stack) { Ref.FieldRef(element, field.name) } + // special case to scan Throwable cause (cannot get it reflectively) + if (element is Throwable) { + push(element.cause, visited, stack) { Ref.FieldRef(element, "cause") } } } } } - private fun Class<*>.fields(): List { + private inline fun push(value: Any?, visited: IdentityHashMap, stack: ArrayDeque, ref: () -> Ref) { + if (value != null && !visited.containsKey(value)) { + visited[value] = ref() + stack.addLast(value) + } + } + + private fun fields(type0: Class<*>): List { + fieldsCache[type0]?.let { return it } val result = ArrayList() - var type = this - while (type != Any::class.java) { + var type = type0 + while (true) { val fields = type.declaredFields.filter { !it.type.isPrimitive && !Modifier.isStatic(it.modifiers) && !(it.type.isArray && it.type.componentType.isPrimitive) } + fields.forEach { it.isAccessible = true } // make them all accessible result.addAll(fields) type = type.superclass - } - - return result - } - - // Debugging-only - @Suppress("UNUSED") - fun printPath(from: Any, to: Any) { - val pathNodes = ArrayList() - val visited = newSetFromMap(IdentityHashMap()) - visited.add(from) - if (findPath(from, to, visited, pathNodes)) { - pathNodes.reverse() - println(pathNodes.joinToString(" -> ", from.javaClass.simpleName + " -> ", "-> " + to.javaClass.simpleName)) - } else { - println("Path from $from to $to not found") - } - } - - private fun findPath(from: Any, to: Any, visited: MutableSet, pathNodes: MutableList): Boolean { - if (from === to) { - return true - } - - val type = from.javaClass - if (type.isArray) { - if (type.componentType.isPrimitive) return false - val array = from as Array - array.filterNotNull().forEach { - if (findPath(it, to, visited, pathNodes)) { - return true - } + val superFields = fieldsCache[type] // will stop at Any anyway + if (superFields != null) { + result.addAll(superFields) + break } - return false } - - val fields = type.fields() - fields.forEach { - it.isAccessible = true - val value = it.get(from) ?: return@forEach - if (!visited.add(value)) return@forEach - val found = findPath(value, to, visited, pathNodes) - if (found) { - pathNodes += from.javaClass.simpleName + ":" + it.name - return true - } - } - - return false + fieldsCache[type0] = result + return result } } diff --git a/kotlinx-coroutines-core/jvm/test/ReusableCancellableContinuationTest.kt b/kotlinx-coroutines-core/jvm/test/ReusableCancellableContinuationTest.kt index b324e7ed3a..997f746118 100644 --- a/kotlinx-coroutines-core/jvm/test/ReusableCancellableContinuationTest.kt +++ b/kotlinx-coroutines-core/jvm/test/ReusableCancellableContinuationTest.kt @@ -76,7 +76,7 @@ class ReusableCancellableContinuationTest : TestBase() { expect(4) ensureActive() // Verify child was bound - assertNotNull(FieldWalker.walk(coroutineContext[Job]!!).single { it === continuation }) + FieldWalker.assertReachableCount(1, coroutineContext[Job]) { it === continuation } suspendAtomicCancellableCoroutineReusable { expect(5) coroutineContext[Job]!!.cancel() @@ -97,7 +97,7 @@ class ReusableCancellableContinuationTest : TestBase() { cont = it } ensureActive() - assertTrue { FieldWalker.walk(coroutineContext[Job]!!).contains(cont!!) } + assertTrue { FieldWalker.walk(coroutineContext[Job]).contains(cont!!) } finish(2) } @@ -112,7 +112,7 @@ class ReusableCancellableContinuationTest : TestBase() { cont = it } ensureActive() - assertFalse { FieldWalker.walk(coroutineContext[Job]!!).contains(cont!!) } + FieldWalker.assertReachableCount(0, coroutineContext[Job]) { it === cont } finish(2) } @@ -127,7 +127,7 @@ class ReusableCancellableContinuationTest : TestBase() { } expectUnreached() } catch (e: CancellationException) { - assertFalse { FieldWalker.walk(coroutineContext[Job]!!).contains(cont!!) } + FieldWalker.assertReachableCount(0, coroutineContext[Job]) { it === cont } finish(2) } } @@ -148,19 +148,19 @@ class ReusableCancellableContinuationTest : TestBase() { expect(4) ensureActive() // Verify child was bound - assertEquals(1, FieldWalker.walk(currentJob).count { it is CancellableContinuation<*> }) + FieldWalker.assertReachableCount(1, currentJob) { it is CancellableContinuation<*> } currentJob.cancel() assertFalse(isActive) // Child detached - assertEquals(0, FieldWalker.walk(currentJob).count { it is CancellableContinuation<*> }) + FieldWalker.assertReachableCount(0, currentJob) { it is CancellableContinuation<*> } suspendAtomicCancellableCoroutineReusable { it.resume(Unit) } suspendAtomicCancellableCoroutineReusable { it.resume(Unit) } - assertEquals(0, FieldWalker.walk(currentJob).count { it is CancellableContinuation<*> }) + FieldWalker.assertReachableCount(0, currentJob) { it is CancellableContinuation<*> } try { suspendAtomicCancellableCoroutineReusable {} } catch (e: CancellationException) { - assertEquals(0, FieldWalker.walk(currentJob).count { it is CancellableContinuation<*> }) + FieldWalker.assertReachableCount(0, currentJob) { it is CancellableContinuation<*> } finish(5) } } @@ -184,12 +184,12 @@ class ReusableCancellableContinuationTest : TestBase() { expect(2) val job = coroutineContext[Job]!! // 1 for reusable CC, another one for outer joiner - assertEquals(2, FieldWalker.walk(job).count { it is CancellableContinuation<*> }) + FieldWalker.assertReachableCount(2, job) { it is CancellableContinuation<*> } } expect(1) receiver.join() // Reference should be claimed at this point - assertEquals(0, FieldWalker.walk(receiver).count { it is CancellableContinuation<*> }) + FieldWalker.assertReachableCount(0, receiver) { it is CancellableContinuation<*> } finish(3) } } diff --git a/kotlinx-coroutines-core/jvm/test/flow/ConsumeAsFlowLeakTest.kt b/kotlinx-coroutines-core/jvm/test/flow/ConsumeAsFlowLeakTest.kt index 3fcceaf1fb..c037be1e6d 100644 --- a/kotlinx-coroutines-core/jvm/test/flow/ConsumeAsFlowLeakTest.kt +++ b/kotlinx-coroutines-core/jvm/test/flow/ConsumeAsFlowLeakTest.kt @@ -41,7 +41,7 @@ class ConsumeAsFlowLeakTest : TestBase() { if (shouldSuspendOnSend) yield() channel.send(second) yield() - assertEquals(0, FieldWalker.walk(channel).count { it === second }) + FieldWalker.assertReachableCount(0, channel) { it === second } finish(6) job.cancelAndJoin() } From 9ae6f572153de1222c760005ddafc9bc5860979b Mon Sep 17 00:00:00 2001 From: Roman Elizarov Date: Thu, 13 Feb 2020 15:29:27 +0300 Subject: [PATCH 2/2] ~ Review feedback --- kotlinx-coroutines-core/jvm/test/FieldWalker.kt | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/kotlinx-coroutines-core/jvm/test/FieldWalker.kt b/kotlinx-coroutines-core/jvm/test/FieldWalker.kt index 4c19040d9b..12fd4dea04 100644 --- a/kotlinx-coroutines-core/jvm/test/FieldWalker.kt +++ b/kotlinx-coroutines-core/jvm/test/FieldWalker.kt @@ -1,5 +1,5 @@ /* - * Copyright 2016-2019 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. + * Copyright 2016-2020 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. */ package kotlinx.coroutines @@ -37,10 +37,12 @@ object FieldWalker { val visited = walkRefs(root) val actual = visited.keys.filter(predicate) if (actual.size != expected) { - val text = actual.joinToString("\n") { "\t" + showPath(it, visited) } - assertEquals(expected, actual.size, "Unexpected number objects. Expected $expected, found ${actual.size}:\n$text") + val textDump = actual.joinToString("") { "\n\t" + showPath(it, visited) } + assertEquals( + expected, actual.size, + "Unexpected number objects. Expected $expected, found ${actual.size}$textDump" + ) } - } /* @@ -50,7 +52,7 @@ object FieldWalker { private fun walkRefs(root: Any?): Map { val visited = IdentityHashMap() if (root == null) return visited - visited.put(root, Ref.RootRef) + visited[root] = Ref.RootRef val stack = ArrayDeque() stack.addLast(root) while (stack.isNotEmpty()) { @@ -68,7 +70,7 @@ object FieldWalker { val path = ArrayList() var cur = element while (true) { - val ref = visited[cur]!! + val ref = visited.getValue(cur) if (ref is Ref.RootRef) break when (ref) { is Ref.FieldRef -> {