Fix ByteBuffer Memory Leak in ByteBufferAsyncWritableChannel #3171
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3171 +/- ##
============================================
+ Coverage 64.24% 69.91% +5.67%
- Complexity 10398 12799 +2401
============================================
Files 840 930 +90
Lines 71755 78941 +7186
Branches 8611 9428 +817
============================================
+ Hits 46099 55193 +9094
+ Misses 23004 20834 -2170
- Partials 2652 2914 +262 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ByteBufferAsyncWritableChannel.write(ByteBuffer) creates a memory leak by wrapping the input ByteBuffer in a Netty ByteBuf wrapper without releasing it. Callback has no reference to it. According to Netty's reference counting contract, whoever creates or retains a ByteBuf is responsible for releasing it. Since ByteBufferAsyncWritableChannel creates the wrapper internally, it must release it. The original code never calls wrapper.release(), causing every call to write(ByteBuffer) to leak native memory. ByteBufferAsyncWritableChannelTest.testWriteByteBufferReleasesWrapper proves the bug by: 1. Calling write(ByteBuffer) which creates an internal wrapper 2. Using reflection to extract the wrapper ByteBuf from the channel's internal queue 3. Verifying the wrapper has refCnt=1 before resolution 4. Calling resolveOldestChunk() to complete the normal flow 5. Asserting the wrapper has refCnt=0 (released) after resolution The fix adds a boolean flag to ChunkData to mark which ByteBufs are internal wrappers that need releasing then using that knowledge to correctly release the ByteBuf when needed.
j-tyler
force-pushed
the
j-tyler/fix-bytebuffer-async-writable-channel
branch
from
4786b0c to
fa38498
Compare
| chunkData.resolveChunk(e); | ||
| // Release wrapper if it was created by write(ByteBuffer) | ||
| if (chunkData.isInternalWrapper && chunkData.buf != null) { | ||
| chunkData.buf.release(); |
There was a problem hiding this comment.
would it make sense to make this part of chunkData.resolveChunk() instaed of having this logic in multiple places?
There was a problem hiding this comment.
Yes, this cleans it up nicely. Done.
| throw new IllegalArgumentException("Source buffer cannot be null"); | ||
| } | ||
| return write(Unpooled.wrappedBuffer(src), callback); | ||
| if (!isOpen()) { |
There was a problem hiding this comment.
i think we also need src.release() here.
@SophieGuo410 made similar change https://github.com/linkedin/ambry/pull/3162/files please check if that is sufficient.
There was a problem hiding this comment.
the reason we remove the isOpen logic in write is because we think if isOpen is false, it should already call the resolveAllRemainingChunks in close() to release the buf. Do you think if that's good enough?
There was a problem hiding this comment.
Right, two issues in the same class! I pulled both changes together and deduplicated the logic in write().
| callback.onCompletion(bytesWritten, exception); | ||
| } | ||
| // Release wrapper if it was created by write(ByteBuffer) | ||
| if (isInternalWrapper) { |
There was a problem hiding this comment.
based on my understanding, the buf should be released through callback?
Referring something like
ContentWriteCallback.onCompletion
There was a problem hiding this comment.
The callback doesn't have reference to the ByteBuf if that ByteBuf was created internally on write with a ByteBuffer.
There was a problem hiding this comment.
I see, I thought it will release itself, but turns out it's not due to tryFree is false?
4 participants
Summary
ByteBufferAsyncWritableChannel.write(ByteBuffer) creates a memory leak by wrapping the input ByteBuffer in a Netty ByteBuf wrapper without releasing it. Callback has no reference to it. According to Netty's reference counting contract, whoever creates or retains a ByteBuf is responsible for releasing it. Since ByteBufferAsyncWritableChannel creates the wrapper internally, it must release it. The original code never calls wrapper.release(), causing every call to write(ByteBuffer) to leak native memory.
The fix adds a boolean flag to ChunkData to mark which ByteBufs are internal wrappers that need releasing then using that knowledge to correctly release the ByteBuf when needed.
Testing Done
ByteBufferAsyncWritableChannelTest.testWriteByteBufferReleasesWrapper proves the bug by: