-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
.kotlin | ||
kotlin-js-store |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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())) | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No sense to mark this as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making it public however would invite more issues? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it! Also checked it in ✅ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Your comments didn't display before, so I didn't see them)
@PublishedApi
internal object A {
val x: Int = 3
} would make both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @PublishedApi
internal object AgentInstallationType {
var isInstalledStatically = false
} Results in
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Therefore, making it public will break current reflection users
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, ok, you are right. Looks like |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
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.There was a problem hiding this comment.
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!