andreacavalli/netty-incubator-buffer-api
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Run all Buf tests on slices as well.

Browse Source 
Motivation:
Slices should behave identical to normal and composite buffers in all but a very select few aspects related to ownership.

Modification:
Extend the test generation to also produce slice-versions of nearly all test cases. Both slices that cover the entire buffer, and slices that only cover a part of their parent buffer.
Also fix a handful of bugs that this uncovered.

Result:
Buffer slices are now tested much more thoroughly, and a few bugs were fixed.
This commit is contained in:
Chris Vest 2020-11-12 14:55:42 +01:00
parent bb5aff940f
commit ec9395d36e
7 changed files with 125 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
buffer/src/main/java/io/netty/buffer/b2/Buf.java
View File

@ -243,6 +243,9 @@ public interface Buf extends Rc<Buf>, BufAccessors {
* <p>
* This method increments the reference count of this buffer.
* The reference count is decremented again when the slice is deallocated.
* <p>
* The slice is created with a {@linkplain #writerOffset() write offset} equal to the length of the slice,
* so that the entire contents of the slice is ready to be read.
*
* @return A new buffer instance, with independent {@link #readerOffset()} and {@link #writerOffset()},
* that is a view of the readable region of this buffer.
@ -259,6 +262,9 @@ public interface Buf extends Rc<Buf>, BufAccessors {
* <p>
* This method increments the reference count of this buffer.
* The reference count is decremented again when the slice is deallocated.
* <p>
* The slice is created with a {@linkplain #writerOffset() write offset} equal to the length of the slice,
* so that the entire contents of the slice is ready to be read.
*
* @return A new buffer instance, with independent {@link #readerOffset()} and {@link #writerOffset()},
* that is a view of the given region of this buffer.

4
buffer/src/main/java/io/netty/buffer/b2/CompositeBuf.java
View File

@ -179,7 +179,7 @@ final class CompositeBuf extends RcSupport<Buf, CompositeBuf> implements Buf {
public Buf slice(int offset, int length) {
checkWriteBounds(offset, length);
if (offset < 0 || length < 0) {
throw new IndexOutOfBoundsException(
throw new IllegalArgumentException(
"Offset and length cannot be negative, but offset was " +
offset + ", and length was " + length + '.');
}
@ -248,7 +248,7 @@ final class CompositeBuf extends RcSupport<Buf, CompositeBuf> implements Buf {
}
while (length > 0) {
var buf = (Buf) chooseBuffer(srcPos, 0);
int toCopy = buf.capacity() - subOffset;
int toCopy = Math.min(buf.capacity() - subOffset, length);
dest.copyInto(buf, subOffset, destPos, toCopy);
srcPos += toCopy;
destPos += toCopy;

3
buffer/src/main/java/io/netty/buffer/b2/MemSegBuf.java
View File

@ -127,6 +127,9 @@ class MemSegBuf extends RcSupport<Buf, MemSegBuf> implements Buf {
@Override
public Buf slice(int offset, int length) {
if (length < 0) {
throw new IllegalArgumentException("Length cannot be negative: " + length + '.');
}
var slice = seg.asSlice(offset, length);
acquire();
Drop<MemSegBuf> drop = b -> close();

10
buffer/src/main/java/io/netty/buffer/b2/Rc.java
View File

@ -68,4 +68,14 @@ public interface Rc<I extends Rc<I>> extends AutoCloseable {
* or {@code false} if calling {@link #send()} would throw an exception.
*/
boolean isSendable();
/**
* Count the number of borrows of this object.
* If the number of borrows is {@code 0}, then the object is in an "owned" state.
* If the number of borrows is greater than {@code 0}, then the object is in a "borrowed" state.
* If this returns a negative number, then this object has been deallocated.
*
* @return The number of borrows, if any, of this object, or a negative number if this object has been deallocated.
*/
int countBorrows();
}

5
buffer/src/main/java/io/netty/buffer/b2/RcSupport.java
View File

@ -90,6 +90,11 @@ public abstract class RcSupport<I extends Rc<I>, T extends RcSupport<I, T>> impl
return acquires == 0;
}
@Override
public int countBorrows() {
return acquires;
}
/**
* Prepare this instance for ownsership transfer. This method is called from {@link #send()} in the sending thread.
* This method should put this Rc in a deactivated state where it is no longer accessible from the currently owning

104
buffer/src/test/java/io/netty/buffer/b2/BufTest.java
View File

@ -44,6 +44,7 @@ import static io.netty.buffer.b2.Fixture.Properties.COMPOSITE;
import static io.netty.buffer.b2.Fixture.Properties.DIRECT;
import static io.netty.buffer.b2.Fixture.Properties.HEAP;
import static io.netty.buffer.b2.Fixture.Properties.POOLED;
import static io.netty.buffer.b2.Fixture.Properties.SLICE;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
@ -62,6 +63,10 @@ public class BufTest {
return fixtures = fixtureCombinations().toArray(Fixture[]::new);
}
static Stream<Fixture> nonSliceAllocators() {
return fixtureCombinations().filter(f -> !f.isSlice());
}
static Stream<Fixture> heapAllocators() {
return fixtureCombinations().filter(Fixture::isHeap);
}
@ -139,7 +144,44 @@ public class BufTest {
}
};
}, COMPOSITE));
return builder.build();
return builder.build().flatMap(f -> {
// Inject slice versions of everything
Builder<Fixture> andSlices = Stream.builder();
andSlices.add(f);
andSlices.add(new Fixture(f + ".slice(0, capacity())", () -> {
var allocatorBase = f.get();
return new Allocator() {
@Override
public Buf allocate(int size) {
try (Buf base = allocatorBase.allocate(size)) {
return base.slice(0, base.capacity()).writerOffset(0);
}
}
@Override
public void close() {
allocatorBase.close();
}
};
}, SLICE));
andSlices.add(new Fixture(f + ".slice(1, capacity() - 2)", () -> {
var allocatorBase = f.get();
return new Allocator() {
@Override
public Buf allocate(int size) {
try (Buf base = allocatorBase.allocate(size + 2)) {
return base.slice(1, size).writerOffset(0);
}
}
@Override
public void close() {
allocatorBase.close();
}
};
}, SLICE));
return andSlices.build();
});
}
@BeforeAll
@ -198,7 +240,7 @@ public class BufTest {
}
@ParameterizedTest
@MethodSource("allocators")
@MethodSource("nonSliceAllocators")
void allocateAndSendToThread(Fixture fixture) throws Exception {
try (Allocator allocator = fixture.createAllocator()) {
ArrayBlockingQueue<Send<Buf>> queue = new ArrayBlockingQueue<>(10);
@ -218,7 +260,7 @@ public class BufTest {
}
@ParameterizedTest
@MethodSource("allocators")
@MethodSource("nonSliceAllocators")
void allocateAndSendToThreadViaSyncQueue(Fixture fixture) throws Exception {
SynchronousQueue<Send<Buf>> queue = new SynchronousQueue<>();
Future<Byte> future = executor.submit(() -> {
@ -237,7 +279,7 @@ public class BufTest {
}
@ParameterizedTest
@MethodSource("allocators")
@MethodSource("nonSliceAllocators")
void sendMustThrowWhenBufIsAcquired(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator();
Buf buf = allocator.allocate(8)) {
@ -252,7 +294,7 @@ public class BufTest {
}
@ParameterizedTest
@MethodSource("allocators")
@MethodSource("nonSliceAllocators")
void mustThrowWhenAllocatingZeroSizedBuffer(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator()) {
assertThrows(IllegalArgumentException.class, () -> allocator.allocate(0));
@ -450,10 +492,12 @@ public class BufTest {
void sliceWithoutOffsetAndSizeWillIncreaseReferenceCount(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator();
Buf buf = allocator.allocate(8)) {
int borrows = buf.countBorrows();
try (Buf ignored = buf.slice()) {
assertFalse(buf.isSendable());
assertThrows(IllegalStateException.class, buf::send);
}
assertEquals(borrows, buf.countBorrows());
}
}
@ -462,10 +506,12 @@ public class BufTest {
void sliceWithOffsetAndSizeWillIncreaseReferenceCount(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator();
Buf buf = allocator.allocate(8)) {
int borrows = buf.countBorrows();
try (Buf ignored = buf.slice(0, 8)) {
assertFalse(buf.isSendable());
assertThrows(IllegalStateException.class, buf::send);
}
assertEquals(borrows, buf.countBorrows());
}
}
@ -504,7 +550,7 @@ public class BufTest {
}
@ParameterizedTest
@MethodSource("allocators")
@MethodSource("nonSliceAllocators")
void sendOnSliceWithoutOffsetAndSizeMustThrow(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator();
Buf buf = allocator.allocate(8)) {
@ -519,7 +565,7 @@ public class BufTest {
}
@ParameterizedTest
@MethodSource("allocators")
@MethodSource("nonSliceAllocators")
void sendOnSliceWithOffsetAndSizeMustThrow(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator();
Buf buf = allocator.allocate(8)) {
@ -537,9 +583,10 @@ public class BufTest {
void sliceWithNegativeOffsetMustThrow(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator();
Buf buf = allocator.allocate(8)) {
int borrows = buf.countBorrows();
assertThrows(IndexOutOfBoundsException.class, () -> buf.slice(-1, 1));
// Verify that the slice is closed properly afterwards.
assertTrue(buf.isSendable());
assertEquals(borrows, buf.countBorrows());
}
}
@ -548,9 +595,10 @@ public class BufTest {
void sliceWithNegativeSizeMustThrow(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator();
Buf buf = allocator.allocate(8)) {
assertThrows(IndexOutOfBoundsException.class, () -> buf.slice(0, -1));
int borrows = buf.countBorrows();
assertThrows(IllegalArgumentException.class, () -> buf.slice(0, -1));
// Verify that the slice is closed properly afterwards.
assertTrue(buf.isSendable());
assertEquals(borrows, buf.countBorrows());
}
}
@ -559,11 +607,45 @@ public class BufTest {
void sliceWithSizeGreaterThanCapacityMustThrow(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator();
Buf buf = allocator.allocate(8)) {
int borrows = buf.countBorrows();
assertThrows(IndexOutOfBoundsException.class, () -> buf.slice(0, 9));
buf.slice(0, 8).close(); // This is still fine.
assertThrows(IndexOutOfBoundsException.class, () -> buf.slice(1, 8));
// Verify that the slice is closed properly afterwards.
assertTrue(buf.isSendable());
assertEquals(borrows, buf.countBorrows());
}
}
@ParameterizedTest
@MethodSource("allocators")
void sliceWithZeroSizeMustBeAllowed(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator();
Buf buf = allocator.allocate(8)) {
int borrows = buf.countBorrows();
buf.slice(0, 0).close(); // This is fine.
// Verify that the slice is closed properly afterwards.
assertEquals(borrows, buf.countBorrows());
}
}
@ParameterizedTest
@MethodSource("allocators")
public void acquireComposingAndSlicingMustIncrementBorrows(Fixture fixture) {
try (Allocator allocator = fixture.createAllocator();
Buf buf = allocator.allocate(8)) {
int borrows = buf.countBorrows();
try (Buf ignored = buf.acquire()) {
assertEquals(borrows + 1, buf.countBorrows());
try (Buf slice = buf.slice()) {
assertEquals(borrows + 2, buf.countBorrows());
try (Buf ignored1 = Buf.compose(buf, slice)) {
assertEquals(borrows + 3, buf.countBorrows());
}
assertEquals(borrows + 2, buf.countBorrows());
}
assertEquals(borrows + 1, buf.countBorrows());
}
assertEquals(borrows, buf.countBorrows());
}
}

7
buffer/src/test/java/io/netty/buffer/b2/Fixture.java
View File

@ -64,11 +64,16 @@ public final class Fixture implements Supplier<Allocator> {
return properties.contains(Properties.CLEANER);
}
public boolean isSlice() {
return properties.contains(Properties.SLICE);
}
public enum Properties {
HEAP,
DIRECT,
COMPOSITE,
CLEANER,
POOLED
POOLED,
SLICE
}
}