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

Consistency between pooled/unpooled derived buffers

Browse Source 
Motivation:
4bba7526e2f58018817972f38279cc232f519100 introduced changes which made pooled and unpooled derived buffers inconsistent in a few ways:
- Pooled derived buffers always generated a duplicate buffer when duplicate() was called and always generated a sliced buffer when slice() was called. Unpooled derived buffers some times generated a sliced buffer when duplicate() was called.
- The indexes that were set for duplicate buffers generated from slices were not always consistent.
There were also some various bugs in the derived pooled buffer implementation.

Modifications:
- Make pooled/unpooled consistently generate duplicate buffers when duplicate() is called and sliced buffers when slice() is called.
- Fix bugs in the derived pooled buffer

Result:
More consistent behavior from the derived pooled/unpooled buffers.
This commit is contained in:
Scott Mitchell 2016-11-17 20:56:19 -08:00
parent f289ebf7fa
commit 930633350d
9 changed files with 62 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
buffer/src/main/java/io/netty/buffer/AbstractDerivedByteBuf.java
View File

@ -91,7 +91,7 @@ public abstract class AbstractDerivedByteBuf extends AbstractByteBuf {
@Override
public final boolean release(int decrement) {
return unwrap().release(decrement);
return release0(decrement);
}
boolean release0(int decrement) {

8
buffer/src/main/java/io/netty/buffer/AbstractPooledDerivedByteBuf.java
View File

@ -269,15 +269,13 @@ abstract class AbstractPooledDerivedByteBuf extends AbstractReferenceCountedByte
@Override
public ByteBuf duplicate() {
// Capacity is not allowed to change for a sliced ByteBuf, so length == capacity()
final ByteBuf duplicate = slice(0, capacity());
duplicate.setIndex(readerIndex(), writerIndex());
return duplicate;
return new PooledNonRetainedDuplicateByteBuf(referenceCountDelegate, unwrap())
.setIndex(idx(readerIndex()), idx(writerIndex()));
}
@Override
public ByteBuf retainedDuplicate() {
return PooledDuplicatedByteBuf.newInstance(unwrap(), this, readerIndex(), writerIndex());
return PooledDuplicatedByteBuf.newInstance(unwrap(), this, idx(readerIndex()), idx(writerIndex()));
}
@Override

4
buffer/src/main/java/io/netty/buffer/AbstractUnpooledSlicedByteBuf.java
View File

@ -216,9 +216,7 @@ abstract class AbstractUnpooledSlicedByteBuf extends AbstractDerivedByteBuf {
@Override
public ByteBuf duplicate() {
final ByteBuf duplicate = unwrap().slice(adjustment, length());
duplicate.setIndex(readerIndex(), writerIndex());
return duplicate;
return unwrap().duplicate().setIndex(idx(readerIndex()), idx(writerIndex()));
}
@Override

4
buffer/src/main/java/io/netty/buffer/ByteBuf.java
View File

@ -2135,12 +2135,14 @@ public abstract class ByteBuf implements ReferenceCounted, Comparable<ByteBuf> {
* Returns a buffer which shares the whole region of this buffer.
* Modifying the content of the returned buffer or this buffer affects
* each other's content while they maintain separate indexes and marks.
* This method is identical to {@code buf.slice(0, buf.capacity())}.
* This method does not modify {@code readerIndex} or {@code writerIndex} of
* this buffer.
* <p>
* The reader and writer marks will not be duplicated. Also be aware that this method will
* NOT call {@link #retain()} and so the reference count will NOT be increased.
* @return A buffer whose readable content is equivalent to the buffer returned by {@link #slice()}.
* However this buffer will share the capacity of the underlying buffer, and therefore allows access to all of the
* underlying content if necessary.
*/
public abstract ByteBuf duplicate();

4
buffer/src/main/java/io/netty/buffer/PooledDuplicatedByteBuf.java
View File

@ -94,9 +94,7 @@ final class PooledDuplicatedByteBuf extends AbstractPooledDerivedByteBuf {
@Override
public ByteBuf duplicate() {
ByteBuf duplicate = duplicate0();
duplicate.setIndex(readerIndex(), writerIndex());
return duplicate;
return duplicate0().setIndex(readerIndex(), writerIndex());
}
@Override

7
buffer/src/main/java/io/netty/buffer/PooledSlicedByteBuf.java
View File

@ -113,15 +113,12 @@ final class PooledSlicedByteBuf extends AbstractPooledDerivedByteBuf {
@Override
public ByteBuf duplicate() {
ByteBuf duplicate = duplicate0();
duplicate.setIndex(idx(readerIndex()), adjustment + capacity());
return duplicate;
return duplicate0().setIndex(idx(readerIndex()), idx(writerIndex()));
}
@Override
public ByteBuf retainedDuplicate() {
// Capacity is not allowed to change for a sliced ByteBuf, so length == capacity()
return PooledDuplicatedByteBuf.newInstance(unwrap(), this, idx(readerIndex()), adjustment + capacity());
return PooledDuplicatedByteBuf.newInstance(unwrap(), this, idx(readerIndex()), idx(writerIndex()));
}
@Override

35
buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
View File

@ -52,7 +52,6 @@ import static io.netty.buffer.Unpooled.buffer;
import static io.netty.buffer.Unpooled.copiedBuffer;
import static io.netty.buffer.Unpooled.directBuffer;
import static io.netty.buffer.Unpooled.wrappedBuffer;
import static io.netty.util.ReferenceCountUtil.releaseLater;
import static io.netty.util.internal.EmptyArrays.EMPTY_BYTES;
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertArrayEquals;
@ -1729,19 +1728,15 @@ public abstract class AbstractByteBufTest {
// Make sure all properties are copied.
ByteBuf duplicate = buffer.duplicate();
assertEquals(buffer.readerIndex(), duplicate.readerIndex());
assertEquals(buffer.writerIndex(), duplicate.writerIndex());
assertEquals(buffer.capacity(), duplicate.capacity());
assertSame(buffer.order(), duplicate.order());
for (int i = 0; i < duplicate.capacity(); i ++) {
assertEquals(buffer.getByte(i), duplicate.getByte(i));
}
assertEquals(buffer.readableBytes(), duplicate.readableBytes());
assertEquals(0, buffer.compareTo(duplicate));
// Make sure the buffer content is shared.
buffer.setByte(readerIndex, (byte) (buffer.getByte(readerIndex) + 1));
assertEquals(buffer.getByte(readerIndex), duplicate.getByte(readerIndex));
duplicate.setByte(1, (byte) (duplicate.getByte(1) + 1));
assertEquals(buffer.getByte(1), duplicate.getByte(1));
assertEquals(buffer.getByte(readerIndex), duplicate.getByte(duplicate.readerIndex()));
duplicate.setByte(duplicate.readerIndex(), (byte) (duplicate.getByte(duplicate.readerIndex()) + 1));
assertEquals(buffer.getByte(readerIndex), duplicate.getByte(duplicate.readerIndex()));
}
@Test
@ -3411,6 +3406,15 @@ public abstract class AbstractByteBufTest {
ByteBuf slice2 = slice1.retainedSlice(slice1.readerIndex() + 1, 4);
assertEquals(0, slice2.compareTo(expected2));
assertEquals(0, slice2.compareTo(slice2.duplicate()));
assertEquals(0, slice2.compareTo(slice2.slice()));
ByteBuf tmpBuf = slice2.retainedDuplicate();
assertEquals(0, slice2.compareTo(tmpBuf));
tmpBuf.release();
tmpBuf = slice2.retainedSlice();
assertEquals(0, slice2.compareTo(tmpBuf));
tmpBuf.release();
ByteBuf slice3 = doSlice1 ? slice2.slice(slice2.readerIndex() + 1, 2) : slice2.duplicate();
if (doSlice1) {
@ -3540,6 +3544,15 @@ public abstract class AbstractByteBufTest {
ByteBuf dup2 = retainedDuplicate2 ? dup1.retainedDuplicate()
: dup1.duplicate().retain();
assertEquals(0, dup2.compareTo(expected));
assertEquals(0, dup2.compareTo(dup2.duplicate()));
assertEquals(0, dup2.compareTo(dup2.slice()));
ByteBuf tmpBuf = dup2.retainedDuplicate();
assertEquals(0, dup2.compareTo(tmpBuf));
tmpBuf.release();
tmpBuf = dup2.retainedSlice();
assertEquals(0, dup2.compareTo(tmpBuf));
tmpBuf.release();
// The handler created a slice of the slice and is now done with it.
dup2.release();
@ -3574,7 +3587,7 @@ public abstract class AbstractByteBufTest {
ByteBuf b = dup.retainedDuplicate();
assertEquals(0, dup.compareTo(b));
b.release();
assertEquals(0, dup.compareTo(dup.slice(0, dup.capacity())));
assertEquals(0, dup.compareTo(dup.slice(dup.readerIndex(), dup.readableBytes())));
} finally {
if (retainedDuplicate) {
dup.release();

33
buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java
View File

@ -27,10 +27,23 @@ import java.util.Iterator;
import java.util.List;
import java.util.NoSuchElementException;
import static io.netty.buffer.Unpooled.*;
import static io.netty.util.internal.EmptyArrays.*;
import static org.hamcrest.CoreMatchers.*;
import static org.junit.Assert.*;
import static io.netty.buffer.Unpooled.EMPTY_BUFFER;
import static io.netty.buffer.Unpooled.buffer;
import static io.netty.buffer.Unpooled.compositeBuffer;
import static io.netty.buffer.Unpooled.directBuffer;
import static io.netty.buffer.Unpooled.wrappedBuffer;
import static io.netty.util.internal.EmptyArrays.EMPTY_BYTES;
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNotSame;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
/**
* An abstract test class for composite channel buffers
@ -567,13 +580,15 @@ public abstract class AbstractCompositeByteBufTest extends AbstractByteBufTest {
buf.release();
}
@SuppressWarnings("deprecation")
@Test
public void testComponentMustBeSlice() {
public void testComponentMustBeDuplicate() {
CompositeByteBuf buf = compositeBuffer();
buf.addComponent(buffer(4).setIndex(1, 3));
assertThat(buf.component(0), is(instanceOf(AbstractUnpooledSlicedByteBuf.class)));
assertThat(buf.component(0).capacity(), is(2));
assertThat(buf.component(0).maxCapacity(), is(2));
buf.addComponent(buffer(4, 6).setIndex(1, 3));
assertThat(buf.component(0), is(instanceOf(AbstractDerivedByteBuf.class)));
assertThat(buf.component(0).capacity(), is(4));
assertThat(buf.component(0).maxCapacity(), is(6));
assertThat(buf.component(0).readableBytes(), is(2));
buf.release();
}

7
buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java
View File

@ -16,6 +16,7 @@
package io.netty.buffer;
import io.netty.util.internal.ThreadLocalRandom;
import org.junit.Ignore;
import org.junit.Test;
import java.io.IOException;
@ -138,16 +139,14 @@ public class SlicedByteBufTest extends AbstractByteBufTest {
// Ignore for SlicedByteBuf
}
@Test(expected = UnsupportedOperationException.class)
@Ignore("Sliced ByteBuf objects don't allow the capacity to change. So this test would fail and shouldn't be run")
@Override
public void testDuplicateCapacityChange() {
super.testDuplicateCapacityChange();
}
@Test(expected = UnsupportedOperationException.class)
@Ignore("Sliced ByteBuf objects don't allow the capacity to change. So this test would fail and shouldn't be run")
@Override
public void testRetainedDuplicateCapacityChange() {
super.testRetainedDuplicateCapacityChange();
}
@Test