From ef714c90d959f124e868568aa37fc365448596ce Mon Sep 17 00:00:00 2001 From: Chris Vest Date: Fri, 7 May 2021 10:54:04 +0200 Subject: [PATCH 1/2] Hide Rc.countBorrows The state that people really care about is whether or not an Rc has ownership. Exposing the reference count will probably just confuse people. The reference count is still exposed on RcSupport because it may be (and is, in the case of ByteBufAdaptor) needed to support implementation details. --- .../java/io/netty/buffer/api/BufferHolder.java | 5 ----- src/main/java/io/netty/buffer/api/Rc.java | 9 --------- .../java/io/netty/buffer/api/RcSupport.java | 8 +++++++- .../buffer/api/adaptor/ByteBufAdaptor.java | 18 +++++++++++++++--- 4 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/main/java/io/netty/buffer/api/BufferHolder.java b/src/main/java/io/netty/buffer/api/BufferHolder.java index 6a1f6db..e17600d 100644 --- a/src/main/java/io/netty/buffer/api/BufferHolder.java +++ b/src/main/java/io/netty/buffer/api/BufferHolder.java @@ -78,11 +78,6 @@ public abstract class BufferHolder> implements Rc { return buf.isOwned(); } - @Override - public int countBorrows() { - return buf.countBorrows(); - } - @SuppressWarnings("unchecked") @Override public Send send() { diff --git a/src/main/java/io/netty/buffer/api/Rc.java b/src/main/java/io/netty/buffer/api/Rc.java index 6ec164e..48d776a 100644 --- a/src/main/java/io/netty/buffer/api/Rc.java +++ b/src/main/java/io/netty/buffer/api/Rc.java @@ -69,15 +69,6 @@ public interface Rc> extends AutoCloseable { */ boolean isOwned(); - /** - * Count the number of borrows of this object. - * Note that even if the number of borrows is {@code 0}, this object might not be {@linkplain #isOwned() owned} - * because there could be other restrictions involved in ownership. - * - * @return The number of borrows, if any, of this object. - */ - int countBorrows(); - /** * Check if this object is accessible. * diff --git a/src/main/java/io/netty/buffer/api/RcSupport.java b/src/main/java/io/netty/buffer/api/RcSupport.java index 99a4310..6448adc 100644 --- a/src/main/java/io/netty/buffer/api/RcSupport.java +++ b/src/main/java/io/netty/buffer/api/RcSupport.java @@ -106,7 +106,13 @@ public abstract class RcSupport, T extends RcSupport> impl return acquires == 0; } - @Override + /** + * Count the number of borrows of this object. + * Note that even if the number of borrows is {@code 0}, this object might not be {@linkplain #isOwned() owned} + * because there could be other restrictions involved in ownership. + * + * @return The number of borrows, if any, of this object. + */ public int countBorrows() { return Math.max(acquires, 0); } diff --git a/src/main/java/io/netty/buffer/api/adaptor/ByteBufAdaptor.java b/src/main/java/io/netty/buffer/api/adaptor/ByteBufAdaptor.java index fa0ec00..619e0ff 100644 --- a/src/main/java/io/netty/buffer/api/adaptor/ByteBufAdaptor.java +++ b/src/main/java/io/netty/buffer/api/adaptor/ByteBufAdaptor.java @@ -24,6 +24,7 @@ import io.netty.buffer.SlicedByteBuf; import io.netty.buffer.Unpooled; import io.netty.buffer.api.Buffer; import io.netty.buffer.api.BufferAllocator; +import io.netty.buffer.api.RcSupport; import io.netty.util.ByteProcessor; import io.netty.util.IllegalReferenceCountException; @@ -212,13 +213,13 @@ public final class ByteBufAdaptor extends ByteBuf { public ByteBuf ensureWritable(int minWritableBytes) { checkAccess(); if (writableBytes() < minWritableBytes) { - int borrows = buffer.countBorrows(); try { - if (borrows == 0) { + if (buffer.isOwned()) { // Good place. buffer.ensureWritable(minWritableBytes); } else { // Highly questionable place, but ByteBuf technically allows this, so we have to emulate. + int borrows = countBorrows(); release(borrows); try { buffer.ensureWritable(minWritableBytes); @@ -1650,7 +1651,18 @@ public final class ByteBufAdaptor extends ByteBuf { @Override public int refCnt() { - return buffer.isAccessible()? 1 + buffer.countBorrows() : 0; + return 1 + countBorrows(); + } + + private int countBorrows() { + if (!buffer.isAccessible()) { + return -1; + } + if (buffer instanceof RcSupport) { + var rc = (RcSupport) buffer; + return rc.countBorrows(); + } + return buffer.isOwned()? 0 : 1; } @Override From 1eece080af0481421007dbb78aeb8626bfa63455 Mon Sep 17 00:00:00 2001 From: Chris Vest Date: Fri, 7 May 2021 11:31:46 +0200 Subject: [PATCH 2/2] Fix up tests that relied on the borrow count --- .../netty/buffer/api/BufferReadOnlyTest.java | 1 - .../api/BufferReferenceCountingTest.java | 59 +++++++++---------- .../netty/buffer/api/BufferTestSupport.java | 4 ++ 3 files changed, 31 insertions(+), 33 deletions(-) diff --git a/src/test/java/io/netty/buffer/api/BufferReadOnlyTest.java b/src/test/java/io/netty/buffer/api/BufferReadOnlyTest.java index 6ec8019..b28ffff 100644 --- a/src/test/java/io/netty/buffer/api/BufferReadOnlyTest.java +++ b/src/test/java/io/netty/buffer/api/BufferReadOnlyTest.java @@ -163,7 +163,6 @@ public class BufferReadOnlyTest extends BufferTestSupport { assertTrue(buf.isOwned()); assertTrue(buf.isAccessible()); assertThat(buf.countComponents()).isOne(); - assertThat(buf.countBorrows()).isZero(); assertEquals((byte) 1, buf.readByte()); assertEquals((byte) 2, buf.readByte()); assertEquals((byte) 3, buf.readByte()); diff --git a/src/test/java/io/netty/buffer/api/BufferReferenceCountingTest.java b/src/test/java/io/netty/buffer/api/BufferReferenceCountingTest.java index 010abd3..d625391 100644 --- a/src/test/java/io/netty/buffer/api/BufferReferenceCountingTest.java +++ b/src/test/java/io/netty/buffer/api/BufferReferenceCountingTest.java @@ -162,12 +162,11 @@ public class BufferReferenceCountingTest extends BufferTestSupport { void sliceWithoutOffsetAndSizeWillIncreaseReferenceCount(Fixture fixture) { try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(8)) { - int borrows = buf.countBorrows(); try (Buffer ignored = buf.slice()) { assertFalse(buf.isOwned()); assertThrows(IllegalStateException.class, buf::send); } - assertEquals(borrows, buf.countBorrows()); + assertTrue(buf.isOwned()); } } @@ -176,12 +175,11 @@ public class BufferReferenceCountingTest extends BufferTestSupport { void sliceWithOffsetAndSizeWillIncreaseReferenceCount(Fixture fixture) { try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(8)) { - int borrows = buf.countBorrows(); try (Buffer ignored = buf.slice(0, 8)) { assertFalse(buf.isOwned()); assertThrows(IllegalStateException.class, buf::send); } - assertEquals(borrows, buf.countBorrows()); + assertTrue(buf.isOwned()); } } @@ -253,10 +251,9 @@ public class BufferReferenceCountingTest extends BufferTestSupport { void sliceWithNegativeOffsetMustThrow(Fixture fixture) { try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(8)) { - int borrows = buf.countBorrows(); assertThrows(IndexOutOfBoundsException.class, () -> buf.slice(-1, 1)); // Verify that the slice is closed properly afterwards. - assertEquals(borrows, buf.countBorrows()); + assertTrue(buf.isOwned()); } } @@ -265,10 +262,9 @@ public class BufferReferenceCountingTest extends BufferTestSupport { void sliceWithNegativeSizeMustThrow(Fixture fixture) { try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(8)) { - int borrows = buf.countBorrows(); assertThrows(IllegalArgumentException.class, () -> buf.slice(0, -1)); // Verify that the slice is closed properly afterwards. - assertEquals(borrows, buf.countBorrows()); + assertTrue(buf.isOwned()); } } @@ -277,12 +273,11 @@ public class BufferReferenceCountingTest extends BufferTestSupport { void sliceWithSizeGreaterThanCapacityMustThrow(Fixture fixture) { try (BufferAllocator allocator = fixture.createAllocator(); Buffer 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. - assertEquals(borrows, buf.countBorrows()); + assertTrue(buf.isOwned()); } } @@ -291,10 +286,9 @@ public class BufferReferenceCountingTest extends BufferTestSupport { void sliceWithZeroSizeMustBeAllowed(Fixture fixture) { try (BufferAllocator allocator = fixture.createAllocator(); Buffer 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()); + assertTrue(buf.isOwned()); } } @@ -303,24 +297,24 @@ public class BufferReferenceCountingTest extends BufferTestSupport { public void acquireComposingAndSlicingMustIncrementBorrows(Fixture fixture) { try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(8)) { - int borrows = buf.countBorrows(); + int borrows = countBorrows(buf); try (Buffer ignored = buf.acquire()) { - assertEquals(borrows + 1, buf.countBorrows()); + assertEquals(borrows + 1, countBorrows(buf)); try (Buffer slice = buf.slice()) { assertEquals(0, slice.capacity()); // We haven't written anything, so the slice is empty. - int sliceBorrows = slice.countBorrows(); - assertEquals(borrows + 2, buf.countBorrows()); + int sliceBorrows = countBorrows(slice); + assertEquals(borrows + 2, countBorrows(buf)); try (Buffer ignored1 = CompositeBuffer.compose(allocator, buf, slice)) { - assertEquals(borrows + 3, buf.countBorrows()); + assertEquals(borrows + 3, countBorrows(buf)); // Note: Slice is empty; not acquired by the composite buffer. - assertEquals(sliceBorrows, slice.countBorrows()); + assertEquals(sliceBorrows, countBorrows(slice)); } - assertEquals(sliceBorrows, slice.countBorrows()); - assertEquals(borrows + 2, buf.countBorrows()); + assertEquals(sliceBorrows, countBorrows(slice)); + assertEquals(borrows + 2, countBorrows(buf)); } - assertEquals(borrows + 1, buf.countBorrows()); + assertEquals(borrows + 1, countBorrows(buf)); } - assertEquals(borrows, buf.countBorrows()); + assertEquals(borrows, countBorrows(buf)); } } @@ -330,23 +324,24 @@ public class BufferReferenceCountingTest extends BufferTestSupport { try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(8)) { buf.writeByte((byte) 1); - int borrows = buf.countBorrows(); + int borrows = countBorrows(buf); try (Buffer ignored = buf.acquire()) { - assertEquals(borrows + 1, buf.countBorrows()); + assertEquals(borrows + 1, countBorrows(buf)); try (Buffer slice = buf.slice()) { assertEquals(1, slice.capacity()); - int sliceBorrows = slice.countBorrows(); - assertEquals(borrows + 2, buf.countBorrows()); + int sliceBorrows = countBorrows(slice); + assertEquals(borrows + 2, countBorrows(buf)); try (Buffer ignored1 = CompositeBuffer.compose(allocator, buf, slice)) { - assertEquals(borrows + 3, buf.countBorrows()); - assertEquals(sliceBorrows + 1, slice.countBorrows()); + assertEquals(borrows + 3, countBorrows(buf)); + assertEquals(sliceBorrows + 1, countBorrows(slice)); } - assertEquals(sliceBorrows, slice.countBorrows()); - assertEquals(borrows + 2, buf.countBorrows()); + assertEquals(sliceBorrows, countBorrows(slice)); + assertEquals(borrows + 2, countBorrows(buf)); } - assertEquals(borrows + 1, buf.countBorrows()); + assertEquals(borrows + 1, countBorrows(buf)); } - assertEquals(borrows, buf.countBorrows()); + assertEquals(borrows, countBorrows(buf)); + assertTrue(buf.isOwned()); } } diff --git a/src/test/java/io/netty/buffer/api/BufferTestSupport.java b/src/test/java/io/netty/buffer/api/BufferTestSupport.java index b2c4c72..20a8671 100644 --- a/src/test/java/io/netty/buffer/api/BufferTestSupport.java +++ b/src/test/java/io/netty/buffer/api/BufferTestSupport.java @@ -970,6 +970,10 @@ public abstract class BufferTestSupport { return bs; } + public static int countBorrows(Buffer buf) { + return ((RcSupport) buf).countBorrows(); + } + public static void assertEquals(Buffer expected, Buffer actual) { assertThat(toByteArray(actual)).containsExactly(toByteArray(expected)); }