From 1500c83c628038631791a0a463d07a178a2edcb7 Mon Sep 17 00:00:00 2001 From: Sebastian Sellmair <34319766+sellmair@users.noreply.github.com> Date: Mon, 21 Oct 2024 12:12:26 +0200 Subject: [PATCH] Fix 'kotlinx.coroutines.debug' split package problem (#4247) * kotlinx-coroutines-debug: Fix split-package with the -core module - `kotlinx.coroutines.debug.internal` owned by coroutines-core - `kotlinx.coroutines.debug` owned by coroutines-debug (Which also reflects the nature of the debug module, which exposes _some_ public API from the .debug.internal package) This requires moving: - AgentPremain -> .debug.internal - ByteBuddyDynamicAttach -> .debug - NoOpProbes.kt -> .debug * kotlinx-coroutines-debug: Soften requirements on bytebuddy & junit As those dependencies are not *required* to be available at runtime - bytebuddy: Is shadowed and packaged into the -debug.jar - junit: Is optional * AgentInstallationType: Mark as @PublishedApi, as we want to keep the API stable ... as it has known usages in products which we do not want to break --- integration-testing/.gitignore | 2 + integration-testing/README.md | 1 + integration-testing/build.gradle | 2 +- integration-testing/jpmsTest/build.gradle.kts | 47 ++++++++++++++++ .../java/module-info.java | 7 +++ .../kotlin/DynamicAttachDebugJpmsTest.kt | 54 +++++++++++++++++++ integration-testing/settings.gradle | 1 + .../api/kotlinx-coroutines-core.api | 4 ++ kotlinx-coroutines-core/build.gradle.kts | 2 +- .../com.android.tools/proguard/coroutines.pro | 2 +- .../com.android.tools/r8/coroutines.pro | 2 +- .../META-INF/proguard/coroutines.pro | 2 +- .../debug/internal/AgentInstallationType.kt | 8 ++- .../src/debug/{ => internal}/AgentPremain.kt | 3 +- .../jvm/src/debug/internal/DebugProbesImpl.kt | 2 +- .../jvm/src/module-info.java | 3 +- kotlinx-coroutines-debug/build.gradle.kts | 4 +- .../src/{internal => }/Attach.kt | 4 +- .../src/{internal => }/NoOpProbes.kt | 2 +- kotlinx-coroutines-debug/src/module-info.java | 10 ++-- 20 files changed, 141 insertions(+), 21 deletions(-) create mode 100644 integration-testing/.gitignore create mode 100644 integration-testing/jpmsTest/build.gradle.kts create mode 100644 integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/java/module-info.java create mode 100644 integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/kotlin/DynamicAttachDebugJpmsTest.kt rename kotlinx-coroutines-core/jvm/src/debug/{ => internal}/AgentPremain.kt (97%) rename kotlinx-coroutines-debug/src/{internal => }/Attach.kt (90%) rename kotlinx-coroutines-debug/src/{internal => }/NoOpProbes.kt (92%) diff --git a/integration-testing/.gitignore b/integration-testing/.gitignore new file mode 100644 index 0000000000..24d00437ce --- /dev/null +++ b/integration-testing/.gitignore @@ -0,0 +1,2 @@ +.kotlin +kotlin-js-store \ No newline at end of file diff --git a/integration-testing/README.md b/integration-testing/README.md index 0218b23c47..deed97b35a 100644 --- a/integration-testing/README.md +++ b/integration-testing/README.md @@ -8,6 +8,7 @@ The tests are the following: * `coreAgentTest` checks that `kotlinx-coroutines-core` can be run as a Java agent. * `debugAgentTest` checks that the coroutine debugger can be run as a Java agent. * `debugDynamicAgentTest` checks that `kotlinx-coroutines-debug` agent can self-attach dynamically to JVM as a standalone dependency. +* `debugDynamicAgentJpmsTest` checks that `kotlinx-coroutines-debug` agent can self-attach dynamically to JVM as a standalone dependency (with JPMS) * `smokeTest` builds the multiplatform test project that depends on coroutines. The `integration-testing` project is expected to be in a subdirectory of the main `kotlinx.coroutines` project. diff --git a/integration-testing/build.gradle b/integration-testing/build.gradle index 64301dd9c5..9aef3ead2c 100644 --- a/integration-testing/build.gradle +++ b/integration-testing/build.gradle @@ -167,7 +167,7 @@ compileTestKotlin { } check { - dependsOn([jvmCoreTest, debugDynamicAgentTest, mavenTest, debugAgentTest, coreAgentTest, 'smokeTest:build']) + dependsOn([jvmCoreTest, debugDynamicAgentTest, mavenTest, debugAgentTest, coreAgentTest, ":jpmsTest:check", 'smokeTest:build']) } compileKotlin { kotlinOptions { diff --git a/integration-testing/jpmsTest/build.gradle.kts b/integration-testing/jpmsTest/build.gradle.kts new file mode 100644 index 0000000000..f96f99822f --- /dev/null +++ b/integration-testing/jpmsTest/build.gradle.kts @@ -0,0 +1,47 @@ +@file:Suppress("PropertyName") +plugins { + kotlin("jvm") +} + +val coroutines_version: String by project + +repositories { + if (project.properties["build_snapshot_train"]?.toString()?.toBoolean() == true) { + maven("https://maven.pkg.jetbrains.space/kotlin/p/kotlin/dev") + } + mavenLocal() + mavenCentral() +} + +java { + modularity.inferModulePath.set(true) +} + +kotlin { + jvmToolchain(17) + + val test = target.compilations.getByName("test") + target.compilations.create("debugDynamicAgentJpmsTest") { + associateWith(test) + + + defaultSourceSet.dependencies { + implementation("org.jetbrains.kotlinx:kotlinx-coroutines-core:$coroutines_version") + implementation("org.jetbrains.kotlinx:kotlinx-coroutines-debug:$coroutines_version") + } + + tasks.register("debugDynamicAgentJpmsTest") { + testClassesDirs = output.classesDirs + classpath = javaSourceSet.runtimeClasspath + } + } +} + +tasks.named("check") { + dependsOn(tasks.withType()) +} + +dependencies { + testImplementation(kotlin("test-junit")) +} + diff --git a/integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/java/module-info.java b/integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/java/module-info.java new file mode 100644 index 0000000000..180d85c36a --- /dev/null +++ b/integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/java/module-info.java @@ -0,0 +1,7 @@ +module debug.dynamic.agent.jpms.test { + requires kotlin.stdlib; + requires kotlinx.coroutines.core; + requires kotlinx.coroutines.debug; + requires junit; + requires kotlin.test; +} \ No newline at end of file diff --git a/integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/kotlin/DynamicAttachDebugJpmsTest.kt b/integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/kotlin/DynamicAttachDebugJpmsTest.kt new file mode 100644 index 0000000000..dcfcd0e106 --- /dev/null +++ b/integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/kotlin/DynamicAttachDebugJpmsTest.kt @@ -0,0 +1,54 @@ +@file:OptIn(ExperimentalCoroutinesApi::class) + +import org.junit.* +import kotlinx.coroutines.* +import kotlinx.coroutines.debug.* +import org.junit.Ignore +import org.junit.Test +import java.io.* +import java.lang.IllegalStateException +import kotlin.test.* + +class DynamicAttachDebugJpmsTest { + + /** + * This test is disabled because: + * Dynamic Attach with JPMS is not yet supported. + * + * Here is the state of experiments: + * When launching this test with additional workarounds like + * ``` + * jvmArgs("--add-exports=kotlinx.coroutines.debug/kotlinx.coroutines.repackaged.net.bytebuddy=com.sun.jna") + * jvmArgs("--add-exports=kotlinx.coroutines.debug/kotlinx.coroutines.repackaged.net.bytebuddy.agent=com.sun.jna") + *``` + * + * Then we see issues like + * + * ``` + * Caused by: java.lang.IllegalStateException: The Byte Buddy agent is not loaded or this method is not called via the system class loader + * at kotlinx.coroutines.debug/kotlinx.coroutines.repackaged.net.bytebuddy.agent.Installer.getInstrumentation(Installer.java:61) + * ... 54 more + * ``` + */ + @Ignore("shaded byte-buddy does not work with JPMS") + @Test + fun testAgentDumpsCoroutines() = + DebugProbes.withDebugProbes { + runBlocking { + val baos = ByteArrayOutputStream() + DebugProbes.dumpCoroutines(PrintStream(baos)) + // if the agent works, then dumps should contain something, + // at least the fact that this test is running. + Assert.assertTrue(baos.toString().contains("testAgentDumpsCoroutines")) + } + } + + @Test + fun testAgentIsNotInstalled() { + assertEquals(false, DebugProbes.isInstalled) + assertFailsWith { + DebugProbes.dumpCoroutines(PrintStream(ByteArrayOutputStream())) + } + } + +} diff --git a/integration-testing/settings.gradle b/integration-testing/settings.gradle index 8584c05a9a..5ff68ef809 100644 --- a/integration-testing/settings.gradle +++ b/integration-testing/settings.gradle @@ -8,5 +8,6 @@ pluginManagement { } include 'smokeTest' +include(":jpmsTest") rootProject.name = "kotlinx-coroutines-integration-testing" diff --git a/kotlinx-coroutines-core/api/kotlinx-coroutines-core.api b/kotlinx-coroutines-core/api/kotlinx-coroutines-core.api index 1f5b05a59d..91cccb7bd1 100644 --- a/kotlinx-coroutines-core/api/kotlinx-coroutines-core.api +++ b/kotlinx-coroutines-core/api/kotlinx-coroutines-core.api @@ -911,6 +911,10 @@ public final class kotlinx/coroutines/channels/TickerMode : java/lang/Enum { public static fun values ()[Lkotlinx/coroutines/channels/TickerMode; } +public final class kotlinx/coroutines/debug/internal/AgentInstallationType { + public static final field INSTANCE Lkotlinx/coroutines/debug/internal/AgentInstallationType; +} + public final class kotlinx/coroutines/debug/internal/DebugCoroutineInfo { public final fun getContext ()Lkotlin/coroutines/CoroutineContext; public final fun getCreationStackTrace ()Ljava/util/List; diff --git a/kotlinx-coroutines-core/build.gradle.kts b/kotlinx-coroutines-core/build.gradle.kts index 9941dcc1ee..d6abbd7e8a 100644 --- a/kotlinx-coroutines-core/build.gradle.kts +++ b/kotlinx-coroutines-core/build.gradle.kts @@ -188,7 +188,7 @@ val allMetadataJar by tasks.getting(Jar::class) { setupManifest(this) } fun setupManifest(jar: Jar) { jar.manifest { attributes(mapOf( - "Premain-Class" to "kotlinx.coroutines.debug.AgentPremain", + "Premain-Class" to "kotlinx.coroutines.debug.internal.AgentPremain", "Can-Retransform-Classes" to "true", )) } diff --git a/kotlinx-coroutines-core/jvm/resources/META-INF/com.android.tools/proguard/coroutines.pro b/kotlinx-coroutines-core/jvm/resources/META-INF/com.android.tools/proguard/coroutines.pro index c3911b83b5..1380396073 100644 --- a/kotlinx-coroutines-core/jvm/resources/META-INF/com.android.tools/proguard/coroutines.pro +++ b/kotlinx-coroutines-core/jvm/resources/META-INF/com.android.tools/proguard/coroutines.pro @@ -16,7 +16,7 @@ volatile ; } -# These classes are only required by kotlinx.coroutines.debug.AgentPremain, which is only loaded when +# These classes are only required by kotlinx.coroutines.debug.internal.AgentPremain, which is only loaded when # kotlinx-coroutines-core is used as a Java agent, so these are not needed in contexts where ProGuard is used. -dontwarn java.lang.instrument.ClassFileTransformer -dontwarn sun.misc.SignalHandler diff --git a/kotlinx-coroutines-core/jvm/resources/META-INF/com.android.tools/r8/coroutines.pro b/kotlinx-coroutines-core/jvm/resources/META-INF/com.android.tools/r8/coroutines.pro index 1ac5ce5710..69a28956ac 100644 --- a/kotlinx-coroutines-core/jvm/resources/META-INF/com.android.tools/r8/coroutines.pro +++ b/kotlinx-coroutines-core/jvm/resources/META-INF/com.android.tools/r8/coroutines.pro @@ -12,7 +12,7 @@ volatile ; } -# These classes are only required by kotlinx.coroutines.debug.AgentPremain, which is only loaded when +# These classes are only required by kotlinx.coroutines.debug.internal.AgentPremain, which is only loaded when # kotlinx-coroutines-core is used as a Java agent, so these are not needed in contexts where ProGuard is used. -dontwarn java.lang.instrument.ClassFileTransformer -dontwarn sun.misc.SignalHandler diff --git a/kotlinx-coroutines-core/jvm/resources/META-INF/proguard/coroutines.pro b/kotlinx-coroutines-core/jvm/resources/META-INF/proguard/coroutines.pro index 6d29ed25ce..874b097457 100644 --- a/kotlinx-coroutines-core/jvm/resources/META-INF/proguard/coroutines.pro +++ b/kotlinx-coroutines-core/jvm/resources/META-INF/proguard/coroutines.pro @@ -16,7 +16,7 @@ volatile ; } -# These classes are only required by kotlinx.coroutines.debug.AgentPremain, which is only loaded when +# These classes are only required by kotlinx.coroutines.debug.internal.AgentPremain, which is only loaded when # kotlinx-coroutines-core is used as a Java agent, so these are not needed in contexts where ProGuard is used. -dontwarn java.lang.instrument.ClassFileTransformer -dontwarn sun.misc.SignalHandler diff --git a/kotlinx-coroutines-core/jvm/src/debug/internal/AgentInstallationType.kt b/kotlinx-coroutines-core/jvm/src/debug/internal/AgentInstallationType.kt index 15eead7fc3..be37ff36e7 100644 --- a/kotlinx-coroutines-core/jvm/src/debug/internal/AgentInstallationType.kt +++ b/kotlinx-coroutines-core/jvm/src/debug/internal/AgentInstallationType.kt @@ -3,10 +3,16 @@ package kotlinx.coroutines.debug.internal /** * Object used to differentiate between agent installed statically or dynamically. * This is done in a separate object so [DebugProbesImpl] can check for static installation - * without having to depend on [kotlinx.coroutines.debug.AgentPremain], which is not compatible with Android. + * without having to depend on [AgentPremain], which is not compatible with Android. * Otherwise, access to `AgentPremain.isInstalledStatically` triggers the load of its internal `ClassFileTransformer` * that is not available on Android. + * + * The entity (despite being internal) has usages in the following products + * - Fleet (Reflection): FleetDebugProbes + * - Android (Hard Coded, ignored for Leak Detection) + * - IntelliJ (Suppress KotlinInternalInJava): CoroutineDumpState */ +@PublishedApi internal object AgentInstallationType { internal var isInstalledStatically = false } diff --git a/kotlinx-coroutines-core/jvm/src/debug/AgentPremain.kt b/kotlinx-coroutines-core/jvm/src/debug/internal/AgentPremain.kt similarity index 97% rename from kotlinx-coroutines-core/jvm/src/debug/AgentPremain.kt rename to kotlinx-coroutines-core/jvm/src/debug/internal/AgentPremain.kt index 4f8abb8700..8d0c557ed2 100644 --- a/kotlinx-coroutines-core/jvm/src/debug/AgentPremain.kt +++ b/kotlinx-coroutines-core/jvm/src/debug/internal/AgentPremain.kt @@ -1,7 +1,6 @@ -package kotlinx.coroutines.debug +package kotlinx.coroutines.debug.internal import android.annotation.* -import kotlinx.coroutines.debug.internal.* import org.codehaus.mojo.animal_sniffer.* import sun.misc.* import java.lang.instrument.* diff --git a/kotlinx-coroutines-core/jvm/src/debug/internal/DebugProbesImpl.kt b/kotlinx-coroutines-core/jvm/src/debug/internal/DebugProbesImpl.kt index 2ab03a770c..25594ad01a 100644 --- a/kotlinx-coroutines-core/jvm/src/debug/internal/DebugProbesImpl.kt +++ b/kotlinx-coroutines-core/jvm/src/debug/internal/DebugProbesImpl.kt @@ -51,7 +51,7 @@ internal object DebugProbesImpl { @Suppress("UNCHECKED_CAST") private fun getDynamicAttach(): Function1? = runCatching { - val clz = Class.forName("kotlinx.coroutines.debug.internal.ByteBuddyDynamicAttach") + val clz = Class.forName("kotlinx.coroutines.debug.ByteBuddyDynamicAttach") val ctor = clz.constructors[0] ctor.newInstance() as Function1 }.getOrNull() diff --git a/kotlinx-coroutines-core/jvm/src/module-info.java b/kotlinx-coroutines-core/jvm/src/module-info.java index 2759a34296..ad4c2ee066 100644 --- a/kotlinx-coroutines-core/jvm/src/module-info.java +++ b/kotlinx-coroutines-core/jvm/src/module-info.java @@ -5,13 +5,12 @@ requires transitive kotlin.stdlib; requires kotlinx.atomicfu; - // these are used by kotlinx.coroutines.debug.AgentPremain + // these are used by kotlinx.coroutines.debug.internal.AgentPremain requires static java.instrument; // contains java.lang.instrument.* requires static jdk.unsupported; // contains sun.misc.Signal exports kotlinx.coroutines; exports kotlinx.coroutines.channels; - exports kotlinx.coroutines.debug; exports kotlinx.coroutines.debug.internal; exports kotlinx.coroutines.flow; exports kotlinx.coroutines.flow.internal; diff --git a/kotlinx-coroutines-debug/build.gradle.kts b/kotlinx-coroutines-debug/build.gradle.kts index a9b2d6730d..1d0be22bfd 100644 --- a/kotlinx-coroutines-debug/build.gradle.kts +++ b/kotlinx-coroutines-debug/build.gradle.kts @@ -68,7 +68,7 @@ val shadowJar by tasks.existing(ShadowJar::class) { manifest { attributes( mapOf( - "Premain-Class" to "kotlinx.coroutines.debug.AgentPremain", + "Premain-Class" to "kotlinx.coroutines.debug.internal.AgentPremain", "Can-Redefine-Classes" to "true", "Multi-Release" to "true" ) @@ -104,7 +104,7 @@ kover { filters { excludes { // Never used, safety mechanism - classes("kotlinx.coroutines.debug.internal.NoOpProbesKt") + classes("kotlinx.coroutines.debug.NoOpProbesKt") } } } diff --git a/kotlinx-coroutines-debug/src/internal/Attach.kt b/kotlinx-coroutines-debug/src/Attach.kt similarity index 90% rename from kotlinx-coroutines-debug/src/internal/Attach.kt rename to kotlinx-coroutines-debug/src/Attach.kt index 63bfe8eafe..291d913f3e 100644 --- a/kotlinx-coroutines-debug/src/internal/Attach.kt +++ b/kotlinx-coroutines-debug/src/Attach.kt @@ -1,5 +1,5 @@ @file:Suppress("unused") -package kotlinx.coroutines.debug.internal +package kotlinx.coroutines.debug import net.bytebuddy.* import net.bytebuddy.agent.* @@ -28,7 +28,7 @@ internal class ByteBuddyDynamicAttach : Function1 { private fun detach() { val cl = Class.forName("kotlin.coroutines.jvm.internal.DebugProbesKt") - val cl2 = Class.forName("kotlinx.coroutines.debug.internal.NoOpProbesKt") + val cl2 = Class.forName("kotlinx.coroutines.debug.NoOpProbesKt") ByteBuddy() .redefine(cl2) .name(cl.name) diff --git a/kotlinx-coroutines-debug/src/internal/NoOpProbes.kt b/kotlinx-coroutines-debug/src/NoOpProbes.kt similarity index 92% rename from kotlinx-coroutines-debug/src/internal/NoOpProbes.kt rename to kotlinx-coroutines-debug/src/NoOpProbes.kt index b7b8bf50d4..927936eaa9 100644 --- a/kotlinx-coroutines-debug/src/internal/NoOpProbes.kt +++ b/kotlinx-coroutines-debug/src/NoOpProbes.kt @@ -1,6 +1,6 @@ @file:Suppress("unused", "UNUSED_PARAMETER") -package kotlinx.coroutines.debug.internal +package kotlinx.coroutines.debug import kotlin.coroutines.* diff --git a/kotlinx-coroutines-debug/src/module-info.java b/kotlinx-coroutines-debug/src/module-info.java index 2c7571ec0d..04d321c7c1 100644 --- a/kotlinx-coroutines-debug/src/module-info.java +++ b/kotlinx-coroutines-debug/src/module-info.java @@ -3,12 +3,12 @@ requires java.instrument; requires kotlin.stdlib; requires kotlinx.coroutines.core; - requires net.bytebuddy; - requires net.bytebuddy.agent; - requires org.junit.jupiter.api; - requires org.junit.platform.commons; + requires static net.bytebuddy; + requires static net.bytebuddy.agent; + requires static org.junit.jupiter.api; + requires static org.junit.platform.commons; -// exports kotlinx.coroutines.debug; // already exported by kotlinx.coroutines.core + exports kotlinx.coroutines.debug; exports kotlinx.coroutines.debug.junit4; exports kotlinx.coroutines.debug.junit5; }