Netty - PooledByteBufAllocator and Unpooled.wrappedBuffer method - potential bug?

[felixunivers]

After allocating some direct buffers and trying them to be combined into a wrapped buffer,
the referenced initial buffers become unusable / freed. Example:

    ByteBufAllocator pbba = PooledByteBufAllocator.DEFAULT;
    BUFFERS.add(0, pbba.directBuffer(32, 32));      // buffer of fixed size  32 bytes - header 1
    BUFFERS.add(1, pbba.directBuffer(128, 128));    // buffer of fixed size 128 bytes - header 2
    BUFFERS.add(2, pbba.directBuffer(256));         // buffer of minimum size 256 (can grow) - for data of various size

                                                    // Combine 2 buffers - indexes: 0 and 2
    BUFFERS.add(3, Unpooled.wrappedBuffer(BUFFERS.get(0), BUFFERS.get(2)));

If we now do:

ByteBuf bb0 = BUFFERS.get(0);
System.out.println(bb0.refCnt());    // returns 0  !!!
bb0.writeBytes(headerPrefix);        // throws exception;   headerPrefix = 4 bytes array

bb0 is shown by debugger as 'freed', .refCnt() returns 0, and the next line - bb0.writeBytes(x) throws exception:
io.netty.util.IllegalReferenceCountException: refCnt: 0

BUT, if we write some data into the buffer before we call the wrappedBuffer(...) method,
all appears to be working fine:

HEADER_BUFFERS.get(0).writeBytes(headerPrefix);   // headerPrefix = 4 bytes array
Unpooled.wrappedBuffer(BUFFERS.get(0), BUFFERS.get(2));
ByteBuf bb0 = BUFFERS.get(0);
System.out.println(bb0.refCnt());              // returns 1  !!!

BUT, since we did not write any data to the 2nd buffer, the problem remains there.
ByteBuf bb2 = BUFFERS.get(2);
System.out.println(bb2.refCnt()); // returns 0 !!!

Perhaps this is a feature to auto-release unused memory, but in this case it presents itself rather as a bug. Hopefully it is something Netty designers/insiders could clear up for us.

[normanmaurer]

@felixunivers I am not sure if I correctly understand the problem but if I understood it correctly you are surprised that calling Unpooled.wrappedBuffer(...) with an "empty buffer" will release the "empty buffer" right away. I think this works as "designed" as once you call this method the ownership is transferred and in this case it will just release the buffer directly as its empty.

I can see why this may be confusing and could lead to surprising and hard to debug code. Let me do a pr to change this and discuss with the other developers in this pr.

[normanmaurer]

@felixunivers so actually after thinking a bit more about it and reading our java docs again I think it works as expected... Changing it would be surprising to people that expect it to work like documented.

That said we just deprecated these methods in favour of using Unpooled.wrappedUnmodifiableBuffer(...). The new method will work exactly how you expected.

WDYT?

[normanmaurer]

See also #8040 and #8096

[felixunivers]

Norman's explanation and my further experience with Netty's buffers shows that releasing empty buffers appears as a logical approach for easier/automatic handling of memory for ByteBuf-s.

My goal here was to create one dynamic wrapped buffer based on 2 or more underlying/member buffers. For example buf0 is the wrapped/container buffer and buf1, buf2, ... are the member buffers. When any of the member buffers get updated we see the combined result in buf0.

buf0 = Unpooled.wrappedBuffer(buf1, buf2, ...);

From my testing I am seeing that wrappedBuffer can effectively be used only for the bytes that have been written to the member buffers before the wrappedBuffer - buf0 was created. The writerIndex of each member buffer determines the resulting stream of bytes in wrappedBuffer - buf0 at the time of its creation. If bytes in the member buffers change, these changes are reflected in the wrappedBuffer - buf0, BUT only up to the writerIndex of each member buffer at the time of wrappedBuffer - buf0 creation.
In other words, if we write additional bytes to any of the member buffers, after the wrappedBuffer - buf0 was created, these new bytes are NOT reflected in the wrappedBuffer - buf0.

This surely works as designed. Perhaps the documentation could be updated to make this point clear to users. (For the authors - Feel free to use my description and modify as you see fit for the documentation).

Perhaps, when you Netty gentlemen get bored - a "DynamicWrappedBuffer" that reflects all changes in the member buffers dynamically / instantly could be a cool buffer to add. (Likely a bit PITA to design though).

[Mr00Anderson]

Perhaps, when you Netty gentlemen get bored - a "DynamicWrappedBuffer" that reflects all changes in the member buffers dynamically / instantly could be a cool buffer to add. (Likely a bit PITA to design though).

They take contributions FYI. They probably hardly get bored. 😄

[normanmaurer]

Closing as it seems like all is working as designed