Attach coroutine debug agent safely

[mvicsokolova]

Recently there was a bug in the debugger IDEA-377586: the debug session failed at the start because initialization of the coroutine debug agent failed with this error:

Exception in thread "main" java.lang.NoClassDefFoundError: kotlin/Result
	at kotlinx.coroutines.debug.internal.AgentPremain.<clinit>(AgentPremain.kt:19)
	at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized0(Native Method)
	at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized(Unsafe.java:1160)
	at java.base/jdk.internal.reflect.MethodHandleAccessorFactory.ensureClassInitialized(MethodHandleAccessorFactory.java:300)
	at java.base/jdk.internal.reflect.MethodHandleAccessorFactory.newMethodAccessor(MethodHandleAccessorFactory.java:71)
	at java.base/jdk.internal.reflect.ReflectionFactory.newMethodAccessor(ReflectionFactory.java:159)
	at java.base/java.lang.reflect.Method.acquireMethodAccessor(Method.java:726)
	at java.base/java.lang.reflect.Method.invoke(Method.java:577)
	at java.instrument/sun.instrument.InstrumentationImpl.loadClassAndStartAgent(InstrumentationImpl.java:560)
	at java.instrument/sun.instrument.InstrumentationImpl.loadClassAndCallPremain(InstrumentationImpl.java:572)

This happened because debugger applied kotlinx-coroutines-core as a -javaagent to the run configuration, which did not have kotlin-stdlib in it's bootstrap classpath and the Kotlin classes would've only be loaded later.

Ad a WA the debugger just skips run configurations like this and does not try to attach the coroutine debug agent at all.
This issue relates to the main problem of the coroutines agent attach (#4469), and SPI solution would help in this case too.

For now I suggest to provide some "safe" wrapper around AgentPremain, which would do nothing in case AgentPremain initialization fails.
It would be bad to fail completely silently, as the user would want to know, what could be done to attach the agent anyway, e.g. add kotlin-stdlib.jar to the bootstrap classpath or smth. So maybe we can choose silent failure / clear error message by setting some system property.

Anyway, I think it makes sense to provide some readable clear error message if something went wrong and give a hint how to attach the agent, because the debugger does not help in these cases.

If there may be a better solution, let's discuss please)

[dkhalanskyjb]

The idea of providing better error messages when the debug agent failed to attach is good. The proposed solution looks too complex to me, but maybe I'm missing some context.

[dkhalanskyjb]

If someone controls the system environment at the point when premain is being run, it means they can edit the command line which starts the process, and then, they can simply avoid specifying the Java agent. What's the purpose of adding a new property, then?

[mvicsokolova]

True, we don't need a property, we can just add the hint to remove the -javaagent argument in the error message

[mvicsokolova]

Removed this extra system property

[dkhalanskyjb]

I don't understand the purpose of this wrapper. What's stopping us from adding the check to AgentPremain itself? Won't it be enough to call something like this function as the first thing in AgentPremain.premain?

private fun checkIfStdlibIsAvailable() {
  try {
    Result.success(42)
  } catch (_: Throwable) {
    error("kotlinx.coroutines debug agent failed to load " +
        "because the Kotlin standard library is not present in the classpath.")
  }
}

[mvicsokolova]

This makes sense, yes) This is the first point when we find out that stdlib is not loaded

[gorrus]

If someone later adds a field of some stdlib type to the AgentPremain class, it will cause problems at class loading time. The wrapper will protect against that.

[mvicsokolova]

No, actually this does not help, because the error happens when the AgentPremain class is trying to initialize, at the moment AgentPremain.<clinit> is called. And this check in premain is already too late.

[dkhalanskyjb]

To protect against that, we could implement the check in AgentPremain like so:

// This should be the first property, to ensure the check happens first! Add new properties only below!
val dummy = checkIfStdlibIsAvailable()

[mvicsokolova]

And replaced the wrapper class with this dummy property check

[gorrus]

please also add a test to ensure that the agent does no harm if stdlib is not available

[mvicsokolova]

@dkhalanskyjb hi!)
I need some help with Gradle here 👀

How could we check, that :safeDebugAgentTest:run fails with an error from AgentPremain?
Is there smth like assertFailsWith in Gradle?

I've thought of an alternative to run :safeDebugAgentTest:run from a Kotlin test: to create a ProcessBuilder + try / catch, but maybe there is an easier option?

[dkhalanskyjb]

I'm no Gradle expert, either. I've seen this approach to checking whether and how a Gradle task fails:

• https://github.com/Kotlin/kotlinx-benchmark/blob/bb2208c76f819056e1fa0e665dee503249d4650e/integration/src/test/kotlin/kotlinx/benchmark/integration/BenchmarkFunctionNameValidationTest.kt the test itself,
• https://github.com/Kotlin/kotlinx-benchmark/blob/bb2208c76f819056e1fa0e665dee503249d4650e/integration/src/test/resources/templates/funny-names the project whose build should fail,
• https://github.com/Kotlin/kotlinx-benchmark/blob/bb2208c76f819056e1fa0e665dee503249d4650e/integration/build.gradle.kts#L15 the required dependency.

I think this should point you in the right direction, but if you encounter any difficulties, please let me know, and I'll look into this more closely.

[murfel]

also possibly @shanshin could help?

[shanshin]

Would you like to check the cause of the failure in functional tests or directly in the build?

[mvicsokolova]

I want to ensure that the run task of the project fails (:safeDebugAgentTest:run), I guess those examples from the benchmarks should help, I'll try

Thank you)

[mvicsokolova]

So, I decided to avoid gradle test runners, that would be too complicated. Just forwarded errorOutput of the gradle task and added an error message check.

[mvicsokolova]

And here I assumed that the error on TC: JVM crashed (This problem is reported because OutOfMemoryError has been found in the build log text) could be fixed with more heap space, but I'm not sure

[dkhalanskyjb]

It may be a mistake on the part of TeamCity. The only places in the build log where I see the OutOfMemoryError are this message that you quote and -XX:+HeapDumpOnOutOfMemoryError, which technically contains the string OutOfMemoryError but does not indicate any problems.