Skip to content

Commit

Permalink
Fix 'kotlinx.coroutines.debug' split package problem (#4247)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
sellmair authored Oct 21, 2024
1 parent 1fcffbf commit 1500c83
Show file tree
Hide file tree
Showing 20 changed files with 141 additions and 21 deletions.
2 changes: 2 additions & 0 deletions integration-testing/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
.kotlin
kotlin-js-store
1 change: 1 addition & 0 deletions integration-testing/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion integration-testing/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
47 changes: 47 additions & 0 deletions integration-testing/jpmsTest/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -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<Test>("debugDynamicAgentJpmsTest") {
testClassesDirs = output.classesDirs
classpath = javaSourceSet.runtimeClasspath
}
}
}

tasks.named("check") {
dependsOn(tasks.withType<Test>())
}

dependencies {
testImplementation(kotlin("test-junit"))
}

Original file line number Diff line number Diff line change
@@ -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;
}
Original file line number Diff line number Diff line change
@@ -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<IllegalStateException> {
DebugProbes.dumpCoroutines(PrintStream(ByteArrayOutputStream()))
}
}

}
1 change: 1 addition & 0 deletions integration-testing/settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ pluginManagement {
}

include 'smokeTest'
include(":jpmsTest")

rootProject.name = "kotlinx-coroutines-integration-testing"
4 changes: 4 additions & 0 deletions kotlinx-coroutines-core/api/kotlinx-coroutines-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion kotlinx-coroutines-core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
volatile <fields>;
}

# 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
volatile <fields>;
}

# 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
volatile <fields>;
}

# 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Original file line number Diff line number Diff line change
@@ -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.*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ internal object DebugProbesImpl {

@Suppress("UNCHECKED_CAST")
private fun getDynamicAttach(): Function1<Boolean, Unit>? = 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<Boolean, Unit>
}.getOrNull()
Expand Down
3 changes: 1 addition & 2 deletions kotlinx-coroutines-core/jvm/src/module-info.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions kotlinx-coroutines-debug/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -104,7 +104,7 @@ kover {
filters {
excludes {
// Never used, safety mechanism
classes("kotlinx.coroutines.debug.internal.NoOpProbesKt")
classes("kotlinx.coroutines.debug.NoOpProbesKt")
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
@file:Suppress("unused")
package kotlinx.coroutines.debug.internal
package kotlinx.coroutines.debug

import net.bytebuddy.*
import net.bytebuddy.agent.*
Expand Down Expand Up @@ -28,7 +28,7 @@ internal class ByteBuddyDynamicAttach : Function1<Boolean, Unit> {

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)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
@file:Suppress("unused", "UNUSED_PARAMETER")

package kotlinx.coroutines.debug.internal
package kotlinx.coroutines.debug

import kotlin.coroutines.*

Expand Down
10 changes: 5 additions & 5 deletions kotlinx-coroutines-debug/src/module-info.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

0 comments on commit 1500c83

Please sign in to comment.