mixin.bugfix.packet_leak=true causes Ilegal Reference error on servers
Saereth opened this issue · 23 comments
MC 1.16.5, forge 36.2.39
I enabled the mixin.bugfix.packet_leak=true option in the config as this is a pretty major issue for 1.16.5, and I was pleased to see modern fixed included a fix for it. unfortunately when we run it after a second or two of being connected to the server it kicks with the following error:
Here is the latest.log https://gist.github.com/Saereth/dc789e5ad30ca2342cab71b1e545664b
Now this is a fairly large pack with a lot of mods so certainly it could be another mod but short of a binary search I was hoping you might have some insight in at least where to look or if this is actually something that can be resolved on Modern Fix's end. Thanks! If you need any additional info I'll be glad to provide.
Thanks for this report. I have just made some changes to the patch that should in theory fix the issue. However I do not have a server setup with which to test it, or an easy way to see if the leak is actually gone, so I'm glad someone else does. :)
Can you try the dev build linked in the README and let me know if that fixes it?
Is the leak also fixed? I would like to turn the option on by default if it's known to be working.
The custom payload packet for sure is fixed now yeah, after 1 hour without the fix I'd be OOM with 6.5gb ram used, with the fix I'm at 2.8gb used after 2 hours. Had multiple testers confirm. There still seems to be some leak in the netty buffer but its much slower, took about 8 hours of being connected to an MP server to go OOM with 6.5gb allocated.
Quite possibly a separate issue though
Ah I did find this in the log as well
May 13, 2023 6:35:21 PM io.netty.util.ResourceLeakDetector reportTracedLeak
SEVERE: LEAK: ByteBuf.release() was not called before it's garbage-collected. See http://netty.io/wiki/reference-counted-objects.html for more information.
Recent access records:
Created at:
io.netty.buffer.PooledByteBufAllocator.newDirectBuffer(PooledByteBufAllocator.java:331)
io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:185)
io.netty.buffer.UnsafeByteBufUtil.copy(UnsafeByteBufUtil.java:436)
io.netty.buffer.PooledUnsafeDirectByteBuf.copy(PooledUnsafeDirectByteBuf.java:309)
io.netty.buffer.AbstractByteBuf.copy(AbstractByteBuf.java:1170)
net.minecraft.network.PacketBuffer.copy(PacketBuffer.java:1009)
net.minecraft.network.play.server.SCustomPayloadPlayPacket.func_180735_b(SCustomPayloadPlayPacket.java:69)
net.minecraft.client.network.play.ClientPlayNetHandler.func_147240_a(ClientPlayNetHandler.java:1693)
net.minecraft.network.play.server.SCustomPayloadPlayPacket.redirect$zcm000$handleAndFree(SCustomPayloadPlayPacket.java:535)
net.minecraft.network.play.server.SCustomPayloadPlayPacket.func_148833_a(SCustomPayloadPlayPacket.java:59)
net.minecraft.network.play.server.SCustomPayloadPlayPacket.func_148833_a(SCustomPayloadPlayPacket.java:11)
net.minecraft.network.PacketThreadUtil.func_225383_a(SourceFile:21)
net.minecraft.util.concurrent.ThreadTaskExecutor.func_213166_h(SourceFile:144)
net.minecraft.util.concurrent.RecursiveEventLoop.func_213166_h(SourceFile:23)
net.minecraft.util.concurrent.ThreadTaskExecutor.func_213168_p(SourceFile:118)
net.minecraft.util.concurrent.ThreadTaskExecutor.func_213160_bf(SourceFile:103)
net.minecraft.client.Minecraft.func_195542_b(Minecraft.java:948)
net.minecraft.client.Minecraft.func_99999_d(Minecraft.java:607)
net.minecraft.client.main.Main.main(Main.java:184)
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
java.base/java.lang.reflect.Method.invoke(Method.java:566)
net.minecraftforge.fml.loading.FMLClientLaunchProvider.lambda$launchService$0(FMLClientLaunchProvider.java:37)
cpw.mods.modlauncher.LaunchServiceHandlerDecorator.launch(LaunchServiceHandlerDecorator.java:37)
cpw.mods.modlauncher.LaunchServiceHandler.launch(LaunchServiceHandler.java:54)
cpw.mods.modlauncher.LaunchServiceHandler.launch(LaunchServiceHandler.java:72)
cpw.mods.modlauncher.Launcher.run(Launcher.java:82)
cpw.mods.modlauncher.Launcher.main(Launcher.java:66)
Full log if its useful:
https://gist.github.com/Saereth/bc83afdcd4b91c253036258c04fc331d
So it appears partially fixed?
Mind trying the dev build again? 😅 (Same link but you need to redownload it for the new version.)
seems this error is back with the current build
I suspect some mod relies on the broken leaking behavior, as I read through the code carefully this time and couldn't see any case in MC/base Forge that could cause this problem.
Maybe I will go back to the first fix I gave you which doesn't completely solve the problem but doesn't crash either, and we will have to leave it at that. :(
if its a mod relying on the broken behavior I'd be willing to keep using the second one and do a binary search until I pin it down, obviously having a complete fix is better. The first one is still a vast improvement. I'll have time to dig into this more tomorrow. Ty for all your work on this so far
Weird, I suspect it might be some mod's TOP integration that's the culprit then as I couldn't reproduce it with just ModernFix+TOP.
Most recent modifications to the packet leak fix nullified the reading of the server brand, making (example):
"Paper (Velocity)" turned into "null" on all servers.
I'm not too sure if this would affect anything else but I didn't receive any other errors/warnings nor crashes from this change. Could just be some sort of missing string of code or something minor. Not too sure, however
Did that issue occur even with the fix that @Saereth mentioned worked partially?
https://github.com/embeddedt/ModernFix/actions/runs/4967192410
The same packet that causes the memory leak is also used to convey the server brand, so if my patch causes the brand to be missing it means other corruption can occur.
Im not seeing any issue with the server branding on either version
unless you're referring to something else?
Upon continual extensive testing its not just TOP but seems several mods rely on that so your first fist is probably gonna be our best bet
@Saereth It's the branding that shows in the top left of F3 after you've joined ("forge" server), which apparently becomes "null" in this case.
I also would feel more comfortable going with the first fix if @R00tB33rMan can confirm the branding issue is gone.
Currently the latest builds are using the second fix; I would need to revert to the first one and propagate that change to 1.19.4 first.
Unfortunately null still persists in the F3 brand on build 11 (latest 1.19.4 build).
@R00tB33rMan I remembered that the leak fix is not necessary on versions past 1.16, but I think I may have introduced a mixin to newer versions by accident when merging. So there shouldn't be any issues with the latest build.
@Saereth I have reverted to the first fix which you said worked mostly (aside from the eventual OOM after 8 hours). I'll likely be releasing a new version of ModernFix which includes this tomorrow or the day after.