andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

CompositeByteBuf.addComponent always assume reference count ownership

Browse Source 
Motivation:
The current interface for CompositeByteBuf.addComponent is not clear under what conditions ownership is transferred when addComponent is called. There should be a well defined behavior so that users can ensure that no leaks occur.

Modifications:
- CompositeByteBuf.addComponent should always assume reference count ownership

Result:
Users that call CompositeByteBuf.addComponent do not have to independently check if the buffer's ownership has been transferred and if not independently release the buffer.
Fixes https://github.com/netty/netty/issues/4760
This commit is contained in:
Scott Mitchell 2016-01-26 13:44:20 -08:00
parent 7a7160f176
commit 6312c2f00b
1 changed files with 118 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

129
buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
View File

@ -33,6 +33,8 @@ import java.util.List;
import java.util.ListIterator;
import java.util.NoSuchElementException;
import static io.netty.util.internal.ObjectUtil.checkNotNull;
/**
* A virtual buffer which shows multiple buffers as a single merged buffer. It is recommended to use
* {@link ByteBufAllocator#compositeBuffer()} or {@link Unpooled#wrappedBuffer(ByteBuf...)} instead of calling the
@ -117,13 +119,16 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
/**
* Add the given {@link ByteBuf}.
*
* <p>
* Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
* If you need to have it increased you need to handle it by your own.
*
* @param buffer the {@link ByteBuf} to add
* <p>
* {@link ByteBuf#release()} ownership of {@code buffer} is transfered to this {@link CompositeByteBuf}.
* @param buffer the {@link ByteBuf} to add. {@link ByteBuf#release()} ownership is transfered to this
* {@link CompositeByteBuf}.
*/
public CompositeByteBuf addComponent(ByteBuf buffer) {
checkNotNull(buffer, "buffer");
addComponent0(components.size(), buffer);
consolidateIfNeeded();
return this;
@ -131,11 +136,14 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
/**
* Add the given {@link ByteBuf}s.
*
* <p>
* Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
* If you need to have it increased you need to handle it by your own.
*
* @param buffers the {@link ByteBuf}s to add
* <p>
* {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects in {@code buffers} is transfered to this
* {@link CompositeByteBuf}.
* @param buffers the {@link ByteBuf}s to add. {@link ByteBuf#release()} ownership of all {@link ByteBuf#release()}
* ownership of all {@link ByteBuf} objects is transfered to this {@link CompositeByteBuf}.
*/
public CompositeByteBuf addComponents(ByteBuf... buffers) {
addComponents0(components.size(), buffers);
@ -145,11 +153,14 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
/**
* Add the given {@link ByteBuf}s.
*
* <p>
* Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
* If you need to have it increased you need to handle it by your own.
*
* @param buffers the {@link ByteBuf}s to add
* <p>
* {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects in {@code buffers} is transfered to this
* {@link CompositeByteBuf}.
* @param buffers the {@link ByteBuf}s to add. {@link ByteBuf#release()} ownership of all {@link ByteBuf#release()}
* ownership of all {@link ByteBuf} objects is transfered to this {@link CompositeByteBuf}.
*/
public CompositeByteBuf addComponents(Iterable<ByteBuf> buffers) {
addComponents0(components.size(), buffers);
@ -159,32 +170,38 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
/**
* Add the given {@link ByteBuf} on the specific index.
*
* <p>
* Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
* If you need to have it increased you need to handle it by your own.
*
* @param cIndex the index on which the {@link ByteBuf} will be added
* @param buffer the {@link ByteBuf} to add
* <p>
* {@link ByteBuf#release()} ownership of {@code buffer} is transfered to this {@link CompositeByteBuf}.
* @param cIndex the index on which the {@link ByteBuf} will be added.
* @param buffer the {@link ByteBuf} to add. {@link ByteBuf#release()} ownership is transfered to this
* {@link CompositeByteBuf}.
*/
public CompositeByteBuf addComponent(int cIndex, ByteBuf buffer) {
checkNotNull(buffer, "buffer");
addComponent0(cIndex, buffer);
consolidateIfNeeded();
return this;
}
/**
* Precondition is that {@code buffer != null}.
*/
private int addComponent0(int cIndex, ByteBuf buffer) {
assert buffer != null;
boolean wasAdded = false;
try {
checkComponentIndex(cIndex);
if (buffer == null) {
throw new NullPointerException("buffer");
}
int readableBytes = buffer.readableBytes();
// No need to consolidate - just add a component to the list.
@SuppressWarnings("deprecation")
Component c = new Component(buffer.order(ByteOrder.BIG_ENDIAN).slice());
if (cIndex == components.size()) {
components.add(c);
wasAdded = components.add(c);
if (cIndex == 0) {
c.endOffset = readableBytes;
} else {
@ -194,21 +211,32 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
}
} else {
components.add(cIndex, c);
wasAdded = true;
if (readableBytes != 0) {
updateComponentOffsets(cIndex);
}
}
return cIndex;
} finally {
if (!wasAdded) {
buffer.release();
}
}
}
/**
* Add the given {@link ByteBuf}s on the specific index
*
* <p>
* Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
* If you need to have it increased you need to handle it by your own.
*
* @param cIndex the index on which the {@link ByteBuf} will be added.
* @param buffers the {@link ByteBuf}s to add
* <p>
* {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects in {@code buffers} is transfered to this
* {@link CompositeByteBuf}.
* @param cIndex the index on which the {@link ByteBuf} will be added. {@link ByteBuf#release()} ownership of all
* {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects is transfered to this
* {@link CompositeByteBuf}.
* @param buffers the {@link ByteBuf}s to add. {@link ByteBuf#release()} ownership of all {@link ByteBuf#release()}
* ownership of all {@link ByteBuf} objects is transfered to this {@link CompositeByteBuf}.
*/
public CompositeByteBuf addComponents(int cIndex, ByteBuf... buffers) {
addComponents0(cIndex, buffers);
@ -217,14 +245,16 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
}
private int addComponents0(int cIndex, ByteBuf... buffers) {
checkNotNull(buffers, "buffers");
int i = 0;
try {
checkComponentIndex(cIndex);
if (buffers == null) {
throw new NullPointerException("buffers");
}
// No need for consolidation
for (ByteBuf b: buffers) {
while (i < buffers.length) {
// Increment i now to prepare for the next iteration and prevent a duplicate release (addComponent0
// will release if an exception occurs, and we also release in the finally block here).
ByteBuf b = buffers[i++];
if (b == null) {
break;
}
@ -235,6 +265,18 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
}
}
return cIndex;
} finally {
for (; i < buffers.length; ++i) {
ByteBuf b = buffers[i];
if (b != null) {
try {
b.release();
} catch (Throwable ignored) {
// ignore
}
}
}
}
}
/**
@ -242,9 +284,13 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
*
* Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
* If you need to have it increased you need to handle it by your own.
*
* <p>
* {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects in {@code buffers} is transfered to this
* {@link CompositeByteBuf}.
* @param cIndex the index on which the {@link ByteBuf} will be added.
* @param buffers the {@link ByteBuf}s to add
* @param buffers the {@link ByteBuf}s to add. {@link ByteBuf#release()} ownership of all
* {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects is transfered to this
* {@link CompositeByteBuf}.
*/
public CompositeByteBuf addComponents(int cIndex, Iterable<ByteBuf> buffers) {
addComponents0(cIndex, buffers);
@ -253,21 +299,32 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
}
private int addComponents0(int cIndex, Iterable<ByteBuf> buffers) {
if (buffers == null) {
throw new NullPointerException("buffers");
}
if (buffers instanceof ByteBuf) {
// If buffers also implements ByteBuf (e.g. CompositeByteBuf), it has to go to addComponent(ByteBuf).
return addComponent0(cIndex, (ByteBuf) buffers);
}
checkNotNull(buffers, "buffers");
if (!(buffers instanceof Collection)) {
List<ByteBuf> list = new ArrayList<ByteBuf>();
try {
for (ByteBuf b: buffers) {
list.add(b);
}
buffers = list;
} finally {
if (buffers != list) {
for (ByteBuf b: buffers) {
if (b != null) {
try {
b.release();
} catch (Throwable ignored) {
// ignore
}
}
}
}
}
}
Collection<ByteBuf> col = (Collection<ByteBuf>) buffers;
@ -1457,10 +1514,7 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
}
private ByteBuf allocBuffer(int capacity) {
if (direct) {
return alloc().directBuffer(capacity);
}
return alloc().heapBuffer(capacity);
return direct ? alloc().directBuffer(capacity) : alloc().heapBuffer(capacity);
}
@Override
@ -1482,7 +1536,6 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements
}
void freeIfNecessary() {
// Unwrap so that we can free slices, too.
buf.release(); // We should not get a NPE here. If so, it must be a bug.
}
}