Barteks2x
commented
2 years ago On java 15+, wouldn't it be possible to use hidden classes instead, in which case no special classloader would be needed?
Closed sfPlayer1 closed 1 year ago
Barteks2x
commented
2 years ago On java 15+, wouldn't it be possible to use hidden classes instead, in which case no special classloader would be needed?
zml2008
commented
2 years ago It would theoretically be possible, but the way hidden classes interact with the module system means that it's at best a toss-up as to whether it's best to fix this in the existing system or migrate to hidden classes.
sciwhiz12
commented
2 years ago A user (..𝓐p𝓸th𝓲con (Anti-Human)#4044) came into Forgecord with a crash that seems to be related to this issue: https://pastebin.com/iUFf6W5S. Seems in this instance, the triggering code is in JEI's use of parallelStream in VanillaPlugin#replaceSpecialCraftingRecipes, and the user seems to be able to reproduce this issue consistently.
On another note, this issue doesn't seem to crop up neither in my testing nor, by lack of user reports pertaining to the issue, most users' environments. Could you please provide a minimal reproduction setup for this issue, along with the specs of the machine it was tested on for reference?
sfPlayer1
commented
2 years ago Pretty sure that's the same issue indeed. I don't have a specialized reproducer (or time to create one), just ran across it reliably whenever I set a ForgeCraft server instance up.
It should reproduce well with patching the thread.setContextCL workarounds out of Aequivaleo and then running it along with cyclopscore + a mod using it, which should then fail in a non-fatal way after loading the world. Theoretically all that is needed is getting ASMClassLoader.loadClass to run on a thread without the usual context cl set (e.g. FJ pool). The cache in ASMEventHandler, maybe preceding class loading or the lack of classes to load by the subscriber may make that less straight forward.
I btw don't think it is particularly rare, its fallout may however be limited in most cases as worker thread exceptions tend to not crash the game and most users won't be able to track it down to the cause.
Sm0keySa1m0n
commented
2 years ago Just thought I’d add this here as it’s most likely related: https://gist.github.com/Sm0keySa1m0n/28ca796a43b2fb84ad6f6b5ac754f8c1
Sm0keySa1m0n
commented
2 years ago Just thought I’d add this here as it’s most likely related: https://gist.github.com/Sm0keySa1m0n/28ca796a43b2fb84ad6f6b5ac754f8c1
This stack trace is actually due to using the current thread's context class loader in EventSubclassTransformer rather than ASMClassLoader. Fixing this would require EventSubclassTransformer to get access to the TransformingClassLoader somehow which as it currently stands is not accessible to it in any way. Either it would have to be exposed by modlauncher or you could reflect into cpw.mods.modlauncher.Launcher and grab the classLoader field.
Sm0keySa1m0n
commented
2 years ago Another related stack trace, this time because the context class loader is null:
java.lang.NullPointerException: Cannot invoke "java.lang.ClassLoader.loadClass(String)" because the return value of "java.lang.Thread.getContextClassLoader()" is null
at net.minecraftforge.eventbus.EventSubclassTransformer.buildEvents(EventSubclassTransformer.java:62) ~[eventbus-5.0.7.jar:?] {}
at net.minecraftforge.eventbus.EventSubclassTransformer.transform(EventSubclassTransformer.java:44) ~[eventbus-5.0.7.jar:?] {}
at net.minecraftforge.eventbus.EventBusEngine.processClass(EventBusEngine.java:21) ~[eventbus-5.0.7.jar:?] {}
at net.minecraftforge.eventbus.service.ModLauncherService.processClassWithFlags(ModLauncherService.java:20) ~[eventbus-5.0.7.jar:5.0.7+5.0.7+master.6d3407cc] {}
at cpw.mods.modlauncher.LaunchPluginHandler.offerClassNodeToPlugins(LaunchPluginHandler.java:88) ~[modlauncher-9.1.3.jar:?] {}
at cpw.mods.modlauncher.ClassTransformer.transform(ClassTransformer.java:120) ~[modlauncher-9.1.3.jar:?] {}
at cpw.mods.modlauncher.TransformingClassLoader.maybeTransformClassBytes(TransformingClassLoader.java:50) ~[modlauncher-9.1.3.jar:?] {}
at cpw.mods.cl.ModuleClassLoader.readerToClass(ModuleClassLoader.java:110) ~[securejarhandler-1.0.1.jar:?] {}
at cpw.mods.cl.ModuleClassLoader.lambda$findClass$16(ModuleClassLoader.java:213) ~[securejarhandler-1.0.1.jar:?] {}
at cpw.mods.cl.ModuleClassLoader.loadFromModule(ModuleClassLoader.java:223) ~[securejarhandler-1.0.1.jar:?] {}
at cpw.mods.cl.ModuleClassLoader.findClass(ModuleClassLoader.java:213) ~[securejarhandler-1.0.1.jar:?] {}
at cpw.mods.cl.ModuleClassLoader.loadClass(ModuleClassLoader.java:130) ~[securejarhandler-1.0.1.jar:?] {}
at java.lang.ClassLoader.loadClass(ClassLoader.java:520) ~[?:?] {}
at com.mojang.blaze3d.platform.GlDebug.printDebugLog(GlDebug.java:100) ~[?:?] {re:classloading,pl:runtimedistcleaner:A}
at org.lwjgl.opengl.GLDebugMessageCallbackI.callback(GLDebugMessageCallbackI.java:39) [lwjgl-opengl-3.2.2.jar%2379!/:build 10] {}```
cpw
commented
2 years ago Fixed in 6.0 by removing it...
901864e
XFactHD
commented
1 year ago The mentioned commit seems to only partially fix this issue as it only remedies the cause of the stacktrace in the initial comment. The stacktraces in later comments appear to stem from the EventSubclassTransformer using the current thread's context classloader (which @Sm0keySa1m0n mentioned as well): https://github.com/MinecraftForge/EventBus/blob/master/src/main/java/net/minecraftforge/eventbus/EventSubclassTransformer.java#L92. This is another such stacktrace when a class is loaded on a worker thread in a ForkJoinPool.
The environment in question is using Forge 1.19.2-43.2.3 (which uses EventBus 6.0.3) and the library containing the class that fails to load (com.github.benmanes.caffeine.cache.RemovalCause) is JiJed into a mod.
Preemptively loading this class in a place where Forge controls the classloader (i.e. early client init in my case: https://github.com/XFactHD/FramedBlocks/commit/48188e56c2f80fe9024949c0deb822b999548bbd#diff-2395ad95a2d66aa749892076a966fed48558a2d2dc9c0f32a8f4802cbae7b96eR62-R63) prevents this from happening.
Assuming this is not caused by the fact that the library is JiJed, this issue could technically also happen with the Guava cache (which Caffeine's API replicates) included by Minecraft itself if the vanilla code were to use time-based cache eviction and managed to trigger it before any eviction done within cache access calls (i.e. size-based cache eviction) happens (which would classload the Guava-equivalent of that class on a thread with a Forge-controlled classloader and achieve the same as my mitigation).
TheCurle
commented
1 year ago Some cursory testing implies that XFact's findings are correct; this is still an active issue and the mitgations were only for a single execution branch stemming from the root problem.
LexManos
commented
1 year ago https://github.com/MinecraftForge/EventBus/commit/eb8e549bbf75dd3c478924615e34390e383efc45 Should of addressed this.
https://github.com/MinecraftForge/EventBus/blob/6d3407ccf8a8f74bc66c759cbf40f3909c918c8f/src/main/java/net/minecraftforge/eventbus/ASMEventHandler.java#L181 forces classes to be loaded with the thread's context class loader instead of whatever loaded the subscribing class in the first place.
The thread context class loader is not an universally appropriate substitute, it breaks with threads living outside any application context such as the ForkJoinPool common pool. This breaks popular uses such as parallelStream() or the default executor CompletableFuture overloads, leading to exceptions such as this:
Such a class load issue additionally incurs failure to load the affected class in the future with throwing NoClassDefFoundError. In the above example any later attempts to use the
CapabilityConstructorRegistry$ItemStackEventListenertype fail on other threads, including the regular server thread which presumably has the context class loader set properly.To fix the problem ASMEventHandler will most likely have to grab+store the appropriate class loader upon its creation (may be as simple
method.getDeclaringClass().getClassLoader()?) and use that to define the generated class. In practice this would mean having an ASMClassLoader instance for each target class loader encountered, adapting a single instance every time is not advisable.More specifically:
classLoaderinstance field to ASMEventHandlerclassLoaderwithmethod.getDeclaringClass().getClassLoader()in the constructorLOADERto beConcurrentHashMap<ClassLoader, ASMClassLoader>classLoaderfield to ASMClassLoader and populate it from the constructorASMClassLoader.loadClassuseclassLoaderinstead ofThread.currentThread().getContextClassLoader()LOADER.defineinvocation toLOADER.computeIfAbsent(classLoader, ASMClassLoader::new).define