Fix ByteBuf memory leak in NettyResponseChannel.write(ByteBuffer) #3183
Conversation
NettyResponseChannel.write(ByteBuffer) was leaking memory by not releasing the internal bytebuf wrapper it creates. According to Netty's reference counting contract, whoever creates or retains a ByteBuf is responsible for releasing it. The issue: - write(ByteBuffer) wraps the ByteBuffer in a ByteBuf internally - This wrapper was never released by NettyResponseChannel - Caller has no reference to this wrapper cannot release it themselves The fix: - Release the wrapper bytebuf in the callback internally created - This ensures cleanup happens regardless of success or exception
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3183 +/- ##
============================================
+ Coverage 64.24% 69.87% +5.63%
- Complexity 10398 12802 +2404
============================================
Files 840 930 +90
Lines 71755 78996 +7241
Branches 8611 9431 +820
============================================
+ Hits 46099 55200 +9101
+ Misses 23004 20876 -2128
- Partials 2652 2920 +268 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @Override | ||
| public void onCompletion(Long result, Exception exception) { | ||
| // This internal wrapped bytebuffer is owned by this class and must be released in the callback. | ||
| ByteBuf wrapper = Unpooled.wrappedBuffer(src); |
There was a problem hiding this comment.
I remember a unpooled wrapped buffered doesn't need to be released. Even if we don't want release it, it's not going to leak any memory.
I suppose we can call release on it, but I don't think it's going to make any difference.
There was a problem hiding this comment.
It's not the wrapper that is a problem it's the wrapper's reference to the underlying Bytebuffer. If the ByteBuffer is pooled direct memory it will not become available for re-use until the ByteBuf wrapper here is GCed. If we correctly release it, it will become available immediately. If the time between GC cycles is long, the pool can easily exhaust.
3 participants
Summary
NettyResponseChannel.write(ByteBuffer) was leaking memory by not releasing
the internal bytebuf wrapper it creates. According to Netty's reference
counting contract, whoever creates or retains a ByteBuf is responsible for
releasing it.
The issue:
The fix:
Testing Done
Added a new test class that specifically tests the memory management in NettyResponseChannel
proving both the write(ByteBuffer) and write(ByteBuf) paths manage memory as expected.