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

sellmair · 2024-10-10T08:26:50Z

https://youtrack.jetbrains.com/issue/IDEA-359879/coroutines-debugging-Investigate-split-package-problem
#4124

dkhalanskyjb · 2024-10-11T12:34:38Z

I've looked through https://grep.app/search?q=kotlinx.coroutines.debug.AgentPremain, searching for places where people maybe rely on the current fully qualified name programmatically, and this is the only thing I've found: https://github.com/JetBrains/intellij-platform-gradle-plugin/blob/b6916aa28808b31d5cda83da5da4e381595bd0b6/src/main/kotlin/org/jetbrains/intellij/platform/gradle/tasks/InitializeIntelliJPlatformPluginTask.kt#L148 . This was added in the commit JetBrains/intellij-platform-gradle-plugin@a759cec . Looks like this change could break coroutine debugging for IDEA plugin authors? Could you investigate?

dkhalanskyjb

The idea behind the changes is clear and seems sound. The only questions I have are for comments, the build scripts (but I'm still reading those), and the possibility of this causing breakage (which may be fine if we communicate this properly).

dkhalanskyjb · 2024-10-11T12:39:25Z

kotlinx-coroutines-core/jvm/src/debug/internal/AgentInstallationType.kt

+ *
+ * Usage Note: Fleet (Reflection): FleetDebugProbes
+ * Usage Note: Android (Hard Coded, ignored for Leak Detection)
+ * Usage Note: IntelliJ (Suppress KotlinInternalInJava): CoroutineDumpState


What is this comment trying to convey?

I tried to add small notes on internal APIs which are forcefully used by clients (by reflection or ignoring internal visibility). Ss those clients are 'not to be broken', I think its worth marking the code somehow as "internal, but breached'

Ok, makes sense. Though a comment would not stop us: who reads them? Better to mark this as @PublishedApi so that this appears in the binary compatibility validator dump. Also, it would help a lot if the notes were reworded, like "This object must not be renamed or moved, because this would break binary compatibility for the following clients that forcefully access it:", and then the list. Even with zero context, this gives a clear and actionable impression.

I used

* 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

I hope this is clear with this rewording?

dkhalanskyjb · 2024-10-11T13:01:35Z

integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/kotlin/DynamicAttachDebugJpmsTest.kt

+     * Using:
+     *
+     *     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")


What does this mean?

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

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.

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

The comment that you added is now gone.

(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.

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

dkhalanskyjb · 2024-10-11T13:01:41Z

integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/kotlin/DynamicAttachDebugJpmsTest.kt

+     *
+     * 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


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

sellmair · 2024-10-14T04:44:01Z

I've looked through https://grep.app/search?q=kotlinx.coroutines.debug.AgentPremain, searching for places where people maybe rely on the current fully qualified name programmatically, and this is the only thing I've found: https://github.com/JetBrains/intellij-platform-gradle-plugin/blob/b6916aa28808b31d5cda83da5da4e381595bd0b6/src/main/kotlin/org/jetbrains/intellij/platform/gradle/tasks/InitializeIntelliJPlatformPluginTask.kt#L148 . This was added in the commit JetBrains/intellij-platform-gradle-plugin@a759cec . Looks like this change could break coroutine debugging for IDEA plugin authors? Could you investigate?

I am aware of this usage and I think notifying the team ahead is fine, or a PR can be created
@hsz would you mind taking a look?

sellmair · 2024-10-14T06:35:42Z

I agreed with @hsz that I will track the kotlinx coroutines release:
Once a version of coroutines, which includes this change, lands in IntelliJ, I will notify the team and they will introduce the change 👍

dkhalanskyjb · 2024-10-14T09:21:25Z

kotlinx-coroutines-core/jvm/src/debug/internal/AgentInstallationType.kt

+ *
+ * Usage Note: Fleet (Reflection): FleetDebugProbes
+ * Usage Note: Android (Hard Coded, ignored for Leak Detection)
+ * Usage Note: IntelliJ (Suppress KotlinInternalInJava): CoroutineDumpState


Ok, makes sense. Though a comment would not stop us: who reads them? Better to mark this as @PublishedApi so that this appears in the binary compatibility validator dump. Also, it would help a lot if the notes were reworded, like "This object must not be renamed or moved, because this would break binary compatibility for the following clients that forcefully access it:", and then the list. Even with zero context, this gives a clear and actionable impression.

dkhalanskyjb · 2024-10-14T09:21:27Z

kotlinx-coroutines-core/jvm/src/debug/internal/AgentInstallationType.kt

+ *
+ * Usage Note: Fleet (Reflection): FleetDebugProbes
+ * Usage Note: Android (Hard Coded, ignored for Leak Detection)
+ * Usage Note: IntelliJ (Suppress KotlinInternalInJava): CoroutineDumpState
 */
 internal object AgentInstallationType {
    internal var isInstalledStatically = false


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

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

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

Got it! Also checked it in ✅

(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.

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

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

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 }

Interestingly even

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

would break the binary signature

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

dkhalanskyjb · 2024-10-14T09:36:23Z

integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/kotlin/DynamicAttachDebugJpmsTest.kt

+     * Using:
+     *
+     *     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")


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.

dkhalanskyjb

Please also add descriptions of the new tests to integration-testing/README.md

sellmair · 2024-10-15T04:59:11Z

Please also add descriptions of the new tests to integration-testing/README.md

Done ✅

dkhalanskyjb · 2024-10-15T08:41:23Z

kotlinx-coroutines-core/jvm/src/debug/internal/AgentInstallationType.kt

+ *
+ * Usage Note: Fleet (Reflection): FleetDebugProbes
+ * Usage Note: Android (Hard Coded, ignored for Leak Detection)
+ * Usage Note: IntelliJ (Suppress KotlinInternalInJava): CoroutineDumpState
 */
 internal object AgentInstallationType {
    internal var isInstalledStatically = false


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

dkhalanskyjb · 2024-10-15T08:42:48Z

integration-testing/jpmsTest/src/debugDynamicAgentJpmsTest/kotlin/DynamicAttachDebugJpmsTest.kt

+            }
+        }
+
+    @Test()


Suggested change

@Test()

@Test

dkhalanskyjb · 2024-10-15T08:42:56Z

integration-testing/src/debugDynamicAgentJpmsTest/kotlin/DynamicAttachDebugTest.kt

+            }
+        }
+
+    @Test()


Suggested change

@Test()

@Test

dkhalanskyjb · 2024-10-15T08:47:49Z

integration-testing/src/debugDynamicAgentJpmsTest/kotlin/DynamicAttachDebugTest.kt

@@ -0,0 +1,54 @@
+@file:OptIn(ExperimentalCoroutinesApi::class)


It looks like the whole integration-testing/src/debugDynamicAgentJpmsTest directory is not even used at all and is just a copy of integration-testing/jpmsTest/src?

Yes, this is an artifact, thank you for finding it! Fixed ✅

dkhalanskyjb

Please use the develop branch as the target and rebase your changes on top of it.

dkhalanskyjb · 2024-10-21T08:05:32Z

kotlinx-coroutines-core/jvm/src/debug/internal/AgentInstallationType.kt

+ *
+ * Usage Note: Fleet (Reflection): FleetDebugProbes
+ * Usage Note: Android (Hard Coded, ignored for Leak Detection)
+ * Usage Note: IntelliJ (Suppress KotlinInternalInJava): CoroutineDumpState
 */
 internal object AgentInstallationType {
    internal var isInstalledStatically = false


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

- `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

As those dependencies are not *required* to be available at runtime - bytebuddy: Is shadowed and packaged into the -debug.jar - junit: Is optional

…API stable ... as it has known usages in products which we do not want to break

dkhalanskyjb

Thank you! This issue was really annoying, it's great that you fixed it!

dkhalanskyjb · 2024-12-19T15:37:38Z

@sellmair @hsz FYI: a release with this (1.10.0) is soon to be published!

sellmair requested a review from dkhalanskyjb October 10, 2024 08:26

sellmair force-pushed the sellmair/coroutines-debug-split-pkg branch from f4d6051 to 66b26d5 Compare October 10, 2024 12:09

dkhalanskyjb reviewed Oct 11, 2024

View reviewed changes

sellmair requested a review from dkhalanskyjb October 14, 2024 04:53

hsz mentioned this pull request Oct 14, 2024

Adjust Kotlin Coroutines Debug Agent premain class name JetBrains/intellij-platform-gradle-plugin#1794

Open

dkhalanskyjb reviewed Oct 14, 2024

View reviewed changes

dkhalanskyjb requested changes Oct 14, 2024

View reviewed changes

sellmair requested a review from dkhalanskyjb October 15, 2024 04:54

dkhalanskyjb requested changes Oct 15, 2024

View reviewed changes

sellmair requested a review from dkhalanskyjb October 16, 2024 05:06

dkhalanskyjb changed the base branch from master to develop October 21, 2024 08:12

dkhalanskyjb changed the base branch from develop to master October 21, 2024 08:12

dkhalanskyjb requested changes Oct 21, 2024

View reviewed changes

sellmair added 3 commits October 21, 2024 10:33

kotlinx-coroutines-debug: Soften requirements on bytebuddy & junit

859f945

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 …

faa26b8

…API stable ... as it has known usages in products which we do not want to break

sellmair force-pushed the sellmair/coroutines-debug-split-pkg branch from 39e7e79 to faa26b8 Compare October 21, 2024 08:37

sellmair changed the base branch from master to develop October 21, 2024 08:37

dkhalanskyjb approved these changes Oct 21, 2024

View reviewed changes

dkhalanskyjb merged commit 1500c83 into develop Oct 21, 2024
1 check passed

dkhalanskyjb deleted the sellmair/coroutines-debug-split-pkg branch October 21, 2024 10:12

+                          }
+                      }
+                  @Test()

		@@ -0,0 +1,54 @@
		@file:OptIn(ExperimentalCoroutinesApi::class)

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

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

Conversation

sellmair commented Oct 10, 2024

dkhalanskyjb commented Oct 11, 2024

dkhalanskyjb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sellmair commented Oct 14, 2024 • edited Loading

sellmair commented Oct 14, 2024

dkhalanskyjb Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb left a comment

Choose a reason for hiding this comment

sellmair commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb left a comment

Choose a reason for hiding this comment

dkhalanskyjb commented Dec 19, 2024

dkhalanskyjb Oct 14, 2024 •

edited

Loading

sellmair commented Oct 14, 2024 •

edited

Loading

dkhalanskyjb Oct 14, 2024 •

edited

Loading

dkhalanskyjb Oct 18, 2024 •

edited

Loading