Weak error handling in ByteToMessageDecoder?

[martinfurmanski]

I have this stack trace from 4.1.32 for a leak report, but the code is not significantly changed in this area in 4.1.43 as far as I can see.

io.netty.buffer.PooledByteBufAllocator.newDirectBuffer(PooledByteBufAllocator.java:331)
io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:185)
io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:176)
io.netty.buffer.AbstractByteBufAllocator.buffer(AbstractByteBufAllocator.java:113)
io.netty.handler.codec.ByteToMessageDecoder.expandCumulation(ByteToMessageDecoder.java:529)
io.netty.handler.codec.ByteToMessageDecoder$1.cumulate(ByteToMessageDecoder.java:89)
io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276)
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:1434)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:965)
io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:163)
io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:656)
io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:591)
io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:508)
io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:470)
io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:909)
java.lang.Thread.run(Thread.java:748)
org.neo4j.helpers.NamedThreadFactory$2.run(NamedThreadFactory.java:122

My contention is that the error handling is weak around expandCumulation because for example:

    static ByteBuf expandCumulation(ByteBufAllocator alloc, ByteBuf cumulation, int readable) {
        ByteBuf oldCumulation = cumulation;
        cumulation = alloc.buffer(oldCumulation.readableBytes() + readable);
        cumulation.writeBytes(oldCumulation);
        oldCumulation.release();
        return cumulation;
    }

if this fails after alloc.buffer then that buffer gets completely lost. Even up the stack in ByteToMessageDecoder$1.cumulate (MERGE_CUMULATOR) if writeBytes fails then the returned buffer gets lost.

Thoughts?

[njhill]

@martinfurmanski I think you are right. How about using a newCumulation var instead of oldCumulation like this:

    static ByteBuf expandCumulation(ByteBufAllocator alloc, ByteBuf cumulation, int readable) {
        ByteBuf newCumulation = alloc.buffer(cumulation.readableBytes() + readable);
        try {
            newCumulation.writeBytes(cumulation);
            cumulation.release();
            return cumulation = newCumulation;
        } finally {
            if (newCumulation != cumulation) { 
                newCumulation.release();
            }
        }
    }

[normanmaurer]

Sounds like a plan... @martinfurmanski do you want to provide a PR ?

[martinfurmanski]

@njhill @normanmaurer Thanks for confirming the issue. I'll attempt a PR shortly!

[normanmaurer]

@martinfurmanski thanks again for the patch

[martinfurmanski]

@martinfurmanski thanks again for the patch

Thank you both for the review and acceptance!