Fix ClassCastException in LMAX Disruptor 3 initialization

[jackshirazi]

During startup with an agent, the resulting object can be in the wrong classloader leading to a ClassCastException. Catching that falls back to the old behaviour which works fine

The new behaviour was introduced in 2.23.0 with #2112

[ppkarwasz]

@jackshirazi,

Thank You for bringing this problem to our attention.

Instead of ignoring the CCE, I would rather solve its cause.

While LoaderUtil might be useful to load classes specified by the user, in this case its usage is not appropriate: we exactly know were the class is and we don't need to look in the TCCL.

Can you replace the LoaderUtil invocation with a simple Class.forName(...).getConstructor()... chain of methods?

A test case would be useful, although in this case we can probably just add a comment not to use LoaderUtil with a link to this issue.

[jackshirazi]

We don't use LoaderUtil. The linked issue is this. The full sequence is pretty convoluted

1. Elastic agent opens a url connection, java.net.URL.openConnection which loads sun.net.www.protocol.http.HttpURLConnection
2. HttpURLConnection inits it's logger, a jul logger
3. The command line set -Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager so apache logger is used
4. Because this was loaded from the elastic agent, the current classloader is co.elastic.apm.agent.premain.ShadedClassLoader
5. ShadedClassLoader loads the log4j classes up to org.apache.logging.log4j.core.async.RingBufferLogEventHandler
6. The log4j2 class AsyncLoggerDisruptor casts the RingBufferLogEventHandler instance as a EventHandler, but EventHandler was loaded by log4j2 in the app classloader
7. hence the CCE

Because this is a class loading race condition with the JVM classes using the log4j2 logger before the JVM is initialized (agents kick in at premain), this is complex to solve outside of log4j2, whereas this patch is pretty simple

[ppkarwasz]

Did you try replacing LoaderUtil.newInstanceOf with:

return (EventHandler<RingBufferLogEvent>)
        Class.forName("org.apache.logging.log4j.core.async.RingBufferLogEventHandler")
                .getConstructor()
                .newInstance();

This version should also be more GraalVM-friendly and shouldn't cause a CCE.

[jackshirazi]

Again, we're not using LoaderUtil. This is also not in Graal (Graal doesn't yet support agents, and when it does it won't have this kind of issue because it flattens classloading)

[github-actions]

Job	Requested goals	Build Tool Version	Build Outcome	Build Scan®
build-macos-latest	clean install	3.9.8	✅
build-ubuntu-latest	clean install	3.9.8	✅
build-windows-latest	clean install	3.9.8	✅

Generated by gradle/develocity-actions

[ppkarwasz]

Again, we're not using LoaderUtil. This is also not in Graal (Graal doesn't yet support agents, and when it does it won't have this kind of issue because it flattens classloading)

Sorry, I am not referring to the code of your application, but these lines in Log4j Core:

logging-log4j2/log4j-core/src/main/java/org/apache/logging/log4j/core/async/AsyncLoggerDisruptor.java

Lines 56 to 60 in 2d5f582

	try {
	return LoaderUtil.newInstanceOf("org.apache.logging.log4j.core.async.RingBufferLogEventHandler");
	} catch (final ReflectiveOperationException | LinkageError e) {
	LOGGER.warn("Failed to create event handler for LMAX Disruptor 3.x, trying version 4.x.", e);
	}

I think that line 57 constitutes the problem, since it might load the RingBufferLogEventHandler class from a different classloader than the one that loaded AsyncLoggerDisruptor.

[jackshirazi]

Thanks, I'll test that

[jackshirazi]

that works, sorry took me a while to get what you were suggesting

[vy]

@ppkarwasz, I cannot think of an easy way to test this. The rest LGTM. How shall we proceed?

[ppkarwasz]

@ppkarwasz, I cannot think of an easy way to test this. The rest LGTM. How shall we proceed?

We should at least add a code comment "Don't use LoaderUtil", otherwise we risk reverting the change. 😉