From 3ce65b98251d22fa37d4eb78e9344c39983245d8 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Tue, 16 Nov 2021 15:37:50 +0300 Subject: [PATCH] Introduce ClassValue-based exception constructor cache to stracktrace recovery (#2997) * Use ClassValue-based cache for exception constructors with stacktrace recovery * It eliminates the classloader leak in containerized environments * Insignificantly improves the performance of exception copying * Creates a precedent of guarded use of non-Android-compliant API Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com> --- kotlinx-coroutines-core/build.gradle | 7 -- .../jvm/src/internal/ExceptionsConstructor.kt | 80 +++++++++++++------ .../test/R8ServiceLoaderOptimizationTest.kt | 1 - 3 files changed, 54 insertions(+), 34 deletions(-) diff --git a/kotlinx-coroutines-core/build.gradle b/kotlinx-coroutines-core/build.gradle index 35142854b5..70295b2909 100644 --- a/kotlinx-coroutines-core/build.gradle +++ b/kotlinx-coroutines-core/build.gradle @@ -201,12 +201,6 @@ task checkJdk16() { } } -tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinCompile) { - kotlinOptions.jdkHome = System.env.JDK_16 - // only fail when actually trying to compile, not during project setup phase - dependsOn(checkJdk16) -} - jvmTest { minHeapSize = '1g' maxHeapSize = '1g' @@ -283,7 +277,6 @@ task jdk16Test(type: Test, dependsOn: [compileTestKotlinJvm, checkJdk16]) { classpath = files { jvmTest.classpath } testClassesDirs = files { jvmTest.testClassesDirs } executable = "$System.env.JDK_16/bin/java" - exclude '**/*LFStressTest.*' // lock-freedom tests use LockFreedomTestEnvironment which needs JDK8 exclude '**/*LincheckTest.*' // Lincheck tests use LinChecker which needs JDK8 exclude '**/exceptions/**' // exceptions tests check suppressed exception which needs JDK8 exclude '**/ExceptionsGuideTest.*' diff --git a/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt b/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt index 60328ebdc0..de15225266 100644 --- a/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt +++ b/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt @@ -11,10 +11,15 @@ import java.util.concurrent.locks.* import kotlin.concurrent.* private val throwableFields = Throwable::class.java.fieldsCountOrDefault(-1) -private val cacheLock = ReentrantReadWriteLock() private typealias Ctor = (Throwable) -> Throwable? -// Replace it with ClassValue when Java 6 support is over -private val exceptionCtors: WeakHashMap, Ctor> = WeakHashMap() + +private val ctorCache = try { + if (ANDROID_DETECTED) WeakMapCtorCache + else ClassValueCtorCache +} catch (e: Throwable) { + // Fallback on Java 6 or exotic setups + WeakMapCtorCache +} @Suppress("UNCHECKED_CAST") internal fun tryCopyException(exception: E): E? { @@ -22,33 +27,26 @@ internal fun tryCopyException(exception: E): E? { if (exception is CopyableThrowable<*>) { return runCatching { exception.createCopy() as E? }.getOrNull() } - // Use cached ctor if found - cacheLock.read { exceptionCtors[exception.javaClass] }?.let { cachedCtor -> - return cachedCtor(exception) as E? - } - /* - * Skip reflective copy if an exception has additional fields (that are usually populated in user-defined constructors) - */ - if (throwableFields != exception.javaClass.fieldsCountOrDefault(0)) { - cacheLock.write { exceptionCtors[exception.javaClass] = { null } } - return null - } + return ctorCache.get(exception.javaClass).invoke(exception) as E? +} + +private fun createConstructor(clz: Class): Ctor { + val nullResult: Ctor = { null } // Pre-cache class + // Skip reflective copy if an exception has additional fields (that are usually populated in user-defined constructors) + if (throwableFields != clz.fieldsCountOrDefault(0)) return nullResult /* - * Try to reflectively find constructor(), constructor(message, cause), constructor(cause) or constructor(message). - * Exceptions are shared among coroutines, so we should copy exception before recovering current stacktrace. - */ - var ctor: Ctor? = null - val constructors = exception.javaClass.constructors.sortedByDescending { it.parameterTypes.size } + * Try to reflectively find constructor(), constructor(message, cause), constructor(cause) or constructor(message). + * Exceptions are shared among coroutines, so we should copy exception before recovering current stacktrace. + */ + val constructors = clz.constructors.sortedByDescending { it.parameterTypes.size } for (constructor in constructors) { - ctor = createConstructor(constructor) - if (ctor != null) break + val result = createSafeConstructor(constructor) + if (result != null) return result } - // Store the resulting ctor to cache - cacheLock.write { exceptionCtors[exception.javaClass] = ctor ?: { null } } - return ctor?.invoke(exception) as E? + return nullResult } -private fun createConstructor(constructor: Constructor<*>): Ctor? { +private fun createSafeConstructor(constructor: Constructor<*>): Ctor? { val p = constructor.parameterTypes return when (p.size) { 2 -> when { @@ -71,7 +69,8 @@ private fun createConstructor(constructor: Constructor<*>): Ctor? { private inline fun safeCtor(crossinline block: (Throwable) -> Throwable): Ctor = { e -> runCatching { block(e) }.getOrNull() } -private fun Class<*>.fieldsCountOrDefault(defaultValue: Int) = kotlin.runCatching { fieldsCount() }.getOrDefault(defaultValue) +private fun Class<*>.fieldsCountOrDefault(defaultValue: Int) = + kotlin.runCatching { fieldsCount() }.getOrDefault(defaultValue) private tailrec fun Class<*>.fieldsCount(accumulator: Int = 0): Int { val fieldsCount = declaredFields.count { !Modifier.isStatic(it.modifiers) } @@ -79,3 +78,32 @@ private tailrec fun Class<*>.fieldsCount(accumulator: Int = 0): Int { val superClass = superclass ?: return totalFields return superClass.fieldsCount(totalFields) } + +internal abstract class CtorCache { + abstract fun get(key: Class): Ctor +} + +private object WeakMapCtorCache : CtorCache() { + private val cacheLock = ReentrantReadWriteLock() + private val exceptionCtors: WeakHashMap, Ctor> = WeakHashMap() + + override fun get(key: Class): Ctor { + cacheLock.read { exceptionCtors[key]?.let { return it } } + cacheLock.write { + exceptionCtors[key]?.let { return it } + return createConstructor(key).also { exceptionCtors[key] = it } + } + } +} + +@IgnoreJreRequirement +private object ClassValueCtorCache : CtorCache() { + private val cache = object : ClassValue() { + override fun computeValue(type: Class<*>?): Ctor { + @Suppress("UNCHECKED_CAST") + return createConstructor(type as Class) + } + } + + override fun get(key: Class): Ctor = cache.get(key) +} diff --git a/ui/kotlinx-coroutines-android/test/R8ServiceLoaderOptimizationTest.kt b/ui/kotlinx-coroutines-android/test/R8ServiceLoaderOptimizationTest.kt index 5d60d641aa..47beb85bbf 100644 --- a/ui/kotlinx-coroutines-android/test/R8ServiceLoaderOptimizationTest.kt +++ b/ui/kotlinx-coroutines-android/test/R8ServiceLoaderOptimizationTest.kt @@ -11,7 +11,6 @@ import java.io.* import java.util.stream.* import kotlin.test.* -@Ignore class R8ServiceLoaderOptimizationTest : TestBase() { private val r8Dex = File(System.getProperty("dexPath")!!).asDexFile() private val r8DexNoOptim = File(System.getProperty("noOptimDexPath")!!).asDexFile()