Custom data packets have a PacketBuffer containing the data payload. While client->server packets release this buffer after the packet has been handled, server->client packets generally do not. (Compare processPacket for CPacketCustomPayload and SPacketCustomPayload)
Strangely, the buffer can actually be released in NetHandlerPlayClient#handleCustomPayload, but only for "MC|TrList" packets.
Putting the call to release in processPacket and removing the current "MC|TrList" special case seems like the best solution here.
Here's a log extract from a detected leak: (seen on a decompiled client connecting to a local server in offline mode, passing -Dio.netty.leakDetection.level=PARANOID to client VM args)
Nov 12, 2017 2:34:18 AM 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: 3
#3:
io.netty.buffer.AdvancedLeakAwareByteBuf.toString(AdvancedLeakAwareByteBuf.java:744)
net.minecraft.network.PacketBuffer.toString(PacketBuffer.java:1289)
net.minecraft.network.PacketBuffer.readString(PacketBuffer.java:380)
net.minecraft.client.network.NetHandlerPlayClient.handleCustomPayload(NetHandlerPlayClient.java:1864)
net.minecraft.network.play.server.SPacketCustomPayload.processPacket(SPacketCustomPayload.java:54)
net.minecraft.network.play.server.SPacketCustomPayload.processPacket(SPacketCustomPayload.java:11)
net.minecraft.network.PacketThreadUtil$1.run(PacketThreadUtil.java:15)
java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
java.util.concurrent.FutureTask.run(FutureTask.java:266)
net.minecraft.util.Util.runTask(Util.java:50)
net.minecraft.client.Minecraft.runGameLoop(Minecraft.java:1068)
net.minecraft.client.Minecraft.run(Minecraft.java:398)
net.minecraft.client.main.Main.main(Main.java:118)
sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
java.lang.reflect.Method.invoke(Method.java:498)
net.minecraftforge.gradle.GradleStartCommon.launch(GradleStartCommon.java:97)
GradleStart.main(GradleStart.java:26)
#2:
io.netty.buffer.AdvancedLeakAwareByteBuf.readByte(AdvancedLeakAwareByteBuf.java:396)
net.minecraft.network.PacketBuffer.readByte(PacketBuffer.java:864)
net.minecraft.network.PacketBuffer.readVarInt(PacketBuffer.java:201)
net.minecraft.network.PacketBuffer.readString(PacketBuffer.java:368)
net.minecraft.client.network.NetHandlerPlayClient.handleCustomPayload(NetHandlerPlayClient.java:1864)
net.minecraft.network.play.server.SPacketCustomPayload.processPacket(SPacketCustomPayload.java:54)
net.minecraft.network.play.server.SPacketCustomPayload.processPacket(SPacketCustomPayload.java:11)
net.minecraft.network.PacketThreadUtil$1.run(PacketThreadUtil.java:15)
java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
java.util.concurrent.FutureTask.run(FutureTask.java:266)
net.minecraft.util.Util.runTask(Util.java:50)
net.minecraft.client.Minecraft.runGameLoop(Minecraft.java:1068)
net.minecraft.client.Minecraft.run(Minecraft.java:398)
net.minecraft.client.main.Main.main(Main.java:118)
sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
java.lang.reflect.Method.invoke(Method.java:498)
net.minecraftforge.gradle.GradleStartCommon.launch(GradleStartCommon.java:97)
GradleStart.main(GradleStart.java:26)
#1:
io.netty.buffer.AdvancedLeakAwareByteBuf.writeBytes(AdvancedLeakAwareByteBuf.java:600)
io.netty.buffer.AbstractByteBuf.readBytes(AbstractByteBuf.java:829)
io.netty.buffer.WrappedByteBuf.readBytes(WrappedByteBuf.java:616)
io.netty.buffer.AdvancedLeakAwareByteBuf.readBytes(AdvancedLeakAwareByteBuf.java:469)
net.minecraft.network.PacketBuffer.readBytes(PacketBuffer.java:959)
net.minecraft.network.play.server.SPacketCustomPayload.readPacketData(SPacketCustomPayload.java:38)
net.minecraft.network.NettyPacketDecoder.decode(NettyPacketDecoder.java:38)
io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:411)
io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:248)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:293)
io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:267)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:293)
io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:280)
io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:396)
io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:248)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
io.netty.handler.timeout.IdleStateHandler.channelRead(IdleStateHandler.java:287)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1334)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:926)
io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:134)
io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:624)
io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:559)
io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:476)
io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:438)
io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:858)
java.lang.Thread.run(Thread.java:748)
Created at:
io.netty.util.ResourceLeakDetector.track(ResourceLeakDetector.java:237)
io.netty.buffer.PooledByteBufAllocator.newDirectBuffer(PooledByteBufAllocator.java:327)
io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:181)
io.netty.buffer.AbstractByteBufAllocator.buffer(AbstractByteBufAllocator.java:117)
io.netty.buffer.AbstractByteBuf.readBytes(AbstractByteBuf.java:828)
io.netty.buffer.WrappedByteBuf.readBytes(WrappedByteBuf.java:616)
io.netty.buffer.AdvancedLeakAwareByteBuf.readBytes(AdvancedLeakAwareByteBuf.java:469)
net.minecraft.network.PacketBuffer.readBytes(PacketBuffer.java:959)
net.minecraft.network.play.server.SPacketCustomPayload.readPacketData(SPacketCustomPayload.java:38)
net.minecraft.network.NettyPacketDecoder.decode(NettyPacketDecoder.java:38)
io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:411)
io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:248)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:293)
io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:267)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:293)
io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:280)
io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:396)
io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:248)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
io.netty.handler.timeout.IdleStateHandler.channelRead(IdleStateHandler.java:287)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1334)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:926)
io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:134)
io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:624)
io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:559)
io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:476)
io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:438)
io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:858)
java.lang.Thread.run(Thread.java:748)Linked issues
Comments 6
The client plugin message handler seems to have been restructured in 18w01a and now this is no longer an issue (fixed with an explicit release call in a finally block, not in processPacket though).
As of 1.14.2 Pre-Release 2 (not sure if earlier), this seems to be once again/still an issue.
The client->server custom payload packet frees the ByteBuf created in the packet read method after the custom payload handler has executed via "if (data != null) { data.release(); }". Vanilla does not, ever, create copies of "data". So far, so good.
The server->client custom payload packet frees the ByteBuf created by accessing the packet's "data" "getter". However, this "getter" actually creates a copy of the ByteBuf, which is then released - the original ByteBuf is never freed, unlike in the client->server custom payload.
As copies are made in the S->C "data" "getter", a quick fix would be to use the same release() call for the original "data" object as is used in the C->S packet processing method.
Hm, yeah, that does seem to be the case. I'm not sure why I didn't catch that; I'm pretty sure I tested whether leak warnings were generated by netty when I resolved it as fixed (but that was over a year ago now so I'm not sure about that).
The call to copy was added in 18w01a, so assuming that actually causes issues (I haven't confirmed myself yet), this was never truly fixed.
I've been doing extensive testing with an user "creeper123123321" as part of hooking the networking system; I am very confident that, while the copy itself is fine (and probably a good idea!), the underlying ByteBuf being copied is never freed in S->C (whereas it is in C->S), and this causes a leak.
I've looked at the underlying code for 1.16.5 and 1.17.1 using the Forge workspace, and I can confirm that the leak still exists, as the FriendlyByteBuf (and the underlying ByteBuf) for the S->C custom payload packet is still not released properly. For reference, I've opened PR #8042 at Forge which patches this.
Looks like the order of events here was (all information based off of decompilation without mappings; may be inaccurate):
In 1.6.4, pre-netty, the packet used a byte[] and the message was read using a DataInputStream that was constructed in the handler for
MC|TrList(butMC|Brandusednew String, so it didn't need to).In 13w41a (first netty build), that was changed to a call on
Unpooled.wrappedBufferon the byte array. TheByteBufwas not closed. Same thing for a lot of the server packets.In 1.7.5, a call to
releasewas added to both the server and client channels that used aByteBuf. I can't find a ticket corresponding to this.In 14w31a (1.8 snapshot) it was changed from a byte[] to a
ByteBuf. Server code continued callingUnpooled.wrappedBufferon thatByteBuf, sometimes closing and sometimes not closing (it also changed a case where it was usingDataInputStreamto use theByteBufforMC|TrSel- that's one place where it wasn't closed);processPacketdid not release the buffer. Client code directly used theByteBuf, and calledreleaseonly inMC|TrList, which is where this bug first appeared.In 16w20a (first 1.10 snapshot), the call to
releaseinprocessPacketwas added for the server version, and the residual calls toreleasein the server handler were removed. The client handler was not touched. I can't find a ticket corresponding to this.Great find!