Fix ref-counting when CompositeByteBuf is used with retainedSlice() #8497
Conversation
|
Can one of the admins verify this patch? |
Motivation: ByteBuf.retainedSlice() and similar methods produce sliced buffers with an independent refcount to the buffer that they wrap. One of the optimizations in 10539f4 was to use the ref to the unwrapped buffer object for added slices, but this did not take into account the above special case when later releasing. Thanks to @rkapsi for discovering this via netty#8495. Modifications: Since a reference to the slice is still kept in the Component class, just changed Component.freeIfNecessary() to release the slice in preference to the unwrapped buf. Also added a unit test which reproduces the bug. Result: Fixes netty#8495
njhill
force-pushed
the
composite-refcnt-fix
branch
from
fcc8824 to
38957b8
Compare
|
@netty-bot test this please |
|
Took the branch for a spin, no more Exceptions or ERRORs as reported in #8495 . |
| // refcount to the unwrapped buf if it is a PooledSlicedByteBuf | ||
| ByteBuf s = slice; | ||
| (s != null ? s : buf).release(); | ||
| // null out in both cases since it could be racy |
There was a problem hiding this comment.
I think this would be easier to read like this:
ByteBuf buffer = s == null ? buf : s;
buffer.release();
There was a problem hiding this comment.
@njhill I am confused... why did you need to to have s and toRelease here ? Which not just do what I suggested ?
Which would be:
ByteBuf buffer = slice == null ? buf : slice;
buffer.release();
There was a problem hiding this comment.
@normanmaurer oh, sorry... I misinterpreted your original suggestion since it used s. Maybe I am being overly cautious, but just based on the fact that slice gets nulled after the component is released I thought it would be safer to do the check on a local var (I ack that it probably is only possible to hit an NPE problem if there is another bug or incorrect use though).
Happy to change it to use slice directly if you think that's fine...
There was a problem hiding this comment.
yeah I think I would go for the easier code or would at least not write it so compact to make it easier to understand
There was a problem hiding this comment.
done - changed to use if/else, hopefully this is the most readable of all :)
| @@ -1144,6 +1144,36 @@ public void testReleasesItsComponents() { | |||
| assertEquals(0, buffer.refCnt()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testReleasesItsComponents2() { | |||
| ByteBuf buffer = PooledByteBufAllocator.DEFAULT.buffer(); // 1 | |||
There was a problem hiding this comment.
Can you add a comment that using the PooledByteBufAllocator is important here ?
| ByteBuf s3 = s2.readRetainedSlice(2); // 4 | ||
| ByteBuf s4 = s3.readRetainedSlice(2); // 5 | ||
|
|
||
| ByteBuf composite = PooledByteBufAllocator.DEFAULT.compositeBuffer() |
There was a problem hiding this comment.
@normanmaurer Actually I don't think this one would make a difference since it's only used if/when the composite buffer gets expanded right?
There was a problem hiding this comment.
@njhill ah yeah true... then fix it please to use Unpooled.
| assertEquals(2, buffer.refCnt()); | ||
|
|
||
| // releasing composite should release the 4 components | ||
| ReferenceCountUtil.release(composite); |
There was a problem hiding this comment.
just use composite.release()
| assertEquals(1, buffer.refCnt()); | ||
|
|
||
| // last remaining ref to buffer | ||
| ReferenceCountUtil.release(buffer); |
|
@rkapsi thanks a lot! |
njhill
force-pushed
the
composite-refcnt-fix
branch
from
f70b806 to
ac2b6c9
Compare
|
Thanks @normanmaurer, have pushed a commit addressing your comments. Re the use of |
|
@njhill I guess it would make sense to cleanup this stuff as a followup. |
|
@netty-bot test this please |
|
@njhill merged... thanks! |
Motivation: The special case fixed in netty#8497 also requires that we keep a derived slice when trimming components in place, as done by the capacity(int) and discardReadBytes() methods. Modifications: Ensure that we keep a ref to trimmed components' original retained slice in capacity(int) and discardReadBytes() methods, so that it is released properly when the they are later freed. Add unit test which fails prior to the fix. Result: Edge case leak is eliminated.
Motivation: The special case fixed in #8497 also requires that we keep a derived slice when trimming components in place, as done by the capacity(int) and discardReadBytes() methods. Modifications: Ensure that we keep a ref to trimmed components' original retained slice in capacity(int) and discardReadBytes() methods, so that it is released properly when the they are later freed. Add unit test which fails prior to the fix. Result: Edge case leak is eliminated.
Motivation: The special case fixed in #8497 also requires that we keep a derived slice when trimming components in place, as done by the capacity(int) and discardReadBytes() methods. Modifications: Ensure that we keep a ref to trimmed components' original retained slice in capacity(int) and discardReadBytes() methods, so that it is released properly when the they are later freed. Add unit test which fails prior to the fix. Result: Edge case leak is eliminated.
Motivation: The special case fixed in netty#8497 also requires that we keep a derived slice when trimming components in place, as done by the capacity(int) and discardReadBytes() methods. Modifications: Ensure that we keep a ref to trimmed components' original retained slice in capacity(int) and discardReadBytes() methods, so that it is released properly when the they are later freed. Add unit test which fails prior to the fix. Result: Edge case leak is eliminated.
Motivation:
ByteBuf.retainedSlice()and similar methods produce sliced buffers with an independent refcount to the buffer that they wrap.One of the optimizations in 10539f4 was to use a ref to the unwrapped buffer object for added slices, but this did not take into account the above special case when later releasing.
Thanks to @rkapsi for discovering this via #8495.
Modifications:
Since a reference to the slice is still kept in the
Componentclass, just changedComponent.freeIfNecessary()to release the slice in preference to the unwrapped buf.Also added a unit test which reproduces the bug.
Result:
Fixes #8495