Skip to content

Fix 'kotlinx.coroutines.debug' & 'kotlinx.coroutines.debug.internal' split package problem #4247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You see that this test is Ignored: I could not get JPMS and Dynamic Debug Probes to be happy with each other. I suspect that the shading of byte-buddy is a problem here.

This feature was not working previously, but I thought it's nice to keep the test and comment the

  • conditions under which this was tested
  • the issue we were facing

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me double-check my understanding: do you mean that, if these JVM runtime arguments are added, the test will pass, but without them, it won't? Why not introduce a separate source set where these arguments are passed, so that we know this workaround works reliably?

Also, there could be a separate test that shows that there still is this problem; something like assertFailsWith<IllegalStateException> { test body }. With the additional comment like "we want this test to fail", we'll be able to immediately learn if something changes and this usage becomes possible without workarounds.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct, I updated the comment.
It would be potentially nice to support Dynamic Attach with JPMS.
However, it seems like its blocked by the shaded byte-buddy.

The closest I could get it to working was adding this experiment, adding the jvmargs, but then still observing the issue. I would check in this test so that it is logged and can be easily picked up again, however I also totally accept if we would rather remove the test altogether

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment that you added is now gone.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Your comments didn't display before, so I didn't see them)

The closest I could get it to working was adding this experiment, adding the jvmargs, but then still observing the issue. I would check in this test so that it is logged and can be easily picked up again, however I also totally accept if we would rather remove the test altogether

Alright, thanks! This should be reflected in the in-code comment in any case. I saw that you added a more elaborate comment already, but this was in the file that's now deleted.

Removing the test is also fine by me, as it doesn't check anything and also can become outdated, leading to confusion down the line.

Alternatively, we can reformulate the test a bit: run it with the specified JVM arguments, assert that it fails, and drop a comment: "It would be nice for this not to fail; with this test, we check that it still fails. If the test stops passing, it means we succeeded and can remove the assertFailsWith and make it a proper test". Up to you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I am aware, Titouan Bion is already working on finding ways of making this test pass!

*```
*
* 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above: This is the reason why the case is not working yet

* ```
*/
@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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sense to mark this as internal, as we want this in the BCV dump.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it public however would invite more issues?
I think @PublishedApi + comment is better, some usages can be migrated (Fleet and IntelliJ)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isInstalledStatically is used from the listed products, rights? Then it, too, should be in the API dump, so not internal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Also checked it in ✅

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Your comments didn't display before, so I didn't see them)

Making it public however would invite more issues?
I think @PublishedApi + comment is better, some usages can be migrated (Fleet and IntelliJ)

@PublishedApi
internal object A {
  val x: Int = 3
}

would make both A and A.x inaccessible to those not abusing reflection/error suppressions, but makes x available to anyone who already accessed A. Both A and A.x are included in the API dump.
@PublishedApi internal val x has the same effect, it's just needlessly verbose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PublishedApi
internal object AgentInstallationType {
    var isInstalledStatically = false
}

Results in

public final class kotlinx.coroutines.debug.internal.AgentInstallationType {
  public static final kotlinx.coroutines.debug.internal.AgentInstallationType INSTANCE;
  public final boolean isInstalledStatically();
  public final void setInstalledStatically(boolean);
  static {};
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However the original code

@PublishedApi
internal object AgentInstallationType {
    internal var isInstalledStatically = false
}

Results in

public final class kotlinx.coroutines.debug.internal.AgentInstallationType {
  public static final kotlinx.coroutines.debug.internal.AgentInstallationType INSTANCE;
  public final boolean isInstalledStatically$kotlinx_coroutines_core();
  public final void setInstalledStatically$kotlinx_coroutines_core(boolean);
  static {};
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore, making it public will break current reflection users
Code example from Fleet

  override fun setInstalledStatically(installed: Boolean) {
    c.getDeclaredMethod("setInstalledStatically\$kotlinx_coroutines_core", Boolean::class.java).invoke(instance, installed)
  }
  override fun isInstalledStatically(): Boolean {
    return c.getDeclaredMethod("isInstalledStatically\$kotlinx_coroutines_core").invoke(instance) as Boolean
  }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly even

@PublishedApi
internal object AgentInstallationType {
    @PublishedApi
    internal var isInstalledStatically = false
}

would break the binary signature

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok, you are right. Looks like @PublishedApi internal is the way to go.

}
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;
}