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

Buffer should not expose nativeAddress() directly (#11665)

Browse Source 
Motivation:
Accessing the native address, if a buffer has any, violates the no-aliasing rule for Buffers.
Also, it inherently assumes that a buffer has a single, native memory allocation.
The native address, or addresses, are already available via the Readable- and WritableComponents.
Accessing these via forEachReadable and forEachWritable will also ensure that composite buffers will be handled correctly.

Modification:
Remove the nativeAddress() method from the Buffer API.
Update the ByteBufAdaptor and a few tests to cope with this change.

Result:
Less error prone code, and make unsafe APIs a bit more hidden.
This commit is contained in:
Chris Vest 2021-09-08 20:02:59 +02:00 committed by GitHub
parent 3152ec76db
commit 1eb9a9764e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 99 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
buffer/src/main/java/io/netty/buffer/api/Buffer.java
View File

@ -172,12 +172,6 @@ public interface Buffer extends Resource<Buffer>, BufferAccessor {
*/
Buffer fill(byte value);
/**
* Gives the native memory address backing this buffer, or return 0 if this buffer has no native memory address.
* @return The native memory address, if any, otherwise 0.
*/
long nativeAddress();
/**
* Makes this buffer read-only. This is irreversible.
* This operation is also idempotent, so calling this method multiple times on the same buffer makes no difference.

16
buffer/src/main/java/io/netty/buffer/api/BufferAllocator.java
View File

@ -25,8 +25,9 @@ import java.util.function.Supplier;
public interface BufferAllocator extends AutoCloseable {
/**
* Produces a {@link BufferAllocator} that allocates unpooled, on-heap buffers.
* On-heap buffers have a {@code byte[]} internally, and their {@linkplain Buffer#nativeAddress() native address}
* is zero.
* On-heap buffers have a {@code byte[]} internally, and their
* {@linkplain ReadableComponent#readableNativeAddress() readable} and
* {@linkplain WritableComponent#writableNativeAddress() writable} native addresses are zero.
* <p>
* The concrete {@link Buffer} implementation is chosen by {@link MemoryManager#instance()}.
*
@ -39,7 +40,8 @@ public interface BufferAllocator extends AutoCloseable {
/**
* Produces a {@link BufferAllocator} that allocates unpooled, off-heap buffers.
* Off-heap buffers a native memory pointer internally, which can be obtained from their
* {@linkplain Buffer#nativeAddress() native address method.
* {@linkplain ReadableComponent#readableNativeAddress() readable} and
* {@linkplain WritableComponent#writableNativeAddress() writable} native address methods.
* <p>
* The concrete {@link Buffer} implementation is chosen by {@link MemoryManager#instance()}.
*
@ -51,8 +53,9 @@ public interface BufferAllocator extends AutoCloseable {
/**
* Produces a pooling {@link BufferAllocator} that allocates and recycles on-heap buffers.
* On-heap buffers have a {@code byte[]} internally, and their {@linkplain Buffer#nativeAddress() native address}
* is zero.
* On-heap buffers have a {@code byte[]} internally, and their
* {@linkplain ReadableComponent#readableNativeAddress() readable} and
* {@linkplain WritableComponent#writableNativeAddress() writable} native addresses are zero.
* <p>
* The concrete {@link Buffer} implementation is chosen by {@link MemoryManager#instance()}.
*
@ -65,7 +68,8 @@ public interface BufferAllocator extends AutoCloseable {
/**
* Produces a pooling {@link BufferAllocator} that allocates and recycles off-heap buffers.
* Off-heap buffers a native memory pointer internally, which can be obtained from their
* {@linkplain Buffer#nativeAddress() native address method.
* {@linkplain ReadableComponent#readableNativeAddress() readable} and
* {@linkplain WritableComponent#writableNativeAddress() writable} native address methods.
* <p>
* The concrete {@link Buffer} implementation is chosen by {@link MemoryManager#instance()}.
*

5
buffer/src/main/java/io/netty/buffer/api/BufferStub.java
View File

@ -76,11 +76,6 @@ public class BufferStub implements Buffer {
return delegate.fill(value);
}
@Override
public long nativeAddress() {
return delegate.nativeAddress();
}
@Override
public Buffer makeReadOnly() {
return delegate.makeReadOnly();

5
buffer/src/main/java/io/netty/buffer/api/CompositeBuffer.java
View File

@ -384,11 +384,6 @@ public final class CompositeBuffer extends ResourceSupport<Buffer, CompositeBuff
return this;
}
@Override
public long nativeAddress() {
return 0;
}
@Override
public CompositeBuffer makeReadOnly() {
for (Buffer buf : bufs) {

36
buffer/src/main/java/io/netty/buffer/api/adaptor/ByteBufAdaptor.java
View File

@ -48,7 +48,7 @@ import static io.netty.buffer.api.internal.Statics.isOwned;
public final class ByteBufAdaptor extends ByteBuf {
private final ByteBufAllocatorAdaptor alloc;
private final Buffer buffer;
private final boolean hasMemoryAddress;
private long nativeAddress;
private final int maxCapacity;
public ByteBufAdaptor(ByteBufAllocatorAdaptor alloc, Buffer buffer, int maxCapacity) {
@ -58,10 +58,36 @@ public final class ByteBufAdaptor extends ByteBuf {
}
this.alloc = Objects.requireNonNull(alloc, "The ByteBuf allocator adaptor cannot be null.");
this.buffer = Objects.requireNonNull(buffer, "The buffer being adapted cannot be null.");
hasMemoryAddress = buffer.nativeAddress() != 0;
updateNativeAddress(buffer);
this.maxCapacity = maxCapacity;
}
private void updateNativeAddress(Buffer buffer) {
if (buffer.countReadableComponents() == 1) {
buffer.forEachReadable(0, (index, component) -> {
if (index == 0) {
nativeAddress = component.readableNativeAddress();
return true;
} else {
nativeAddress = 0;
return false;
}
});
} else if (buffer.countWritableComponents() == 1) {
buffer.forEachWritable(0, (index, component) -> {
if (index == 0) {
nativeAddress = component.writableNativeAddress();
return true;
} else {
nativeAddress = 0;
return false;
}
});
} else {
nativeAddress = 0;
}
}
/**
* Extracts the underlying {@link Buffer} instance that is backing this {@link ByteBuf}, if any.
* This is similar to {@link #unwrap()} except the return type is a {@link Buffer}.
@ -154,7 +180,7 @@ public final class ByteBufAdaptor extends ByteBuf {
@Override
public boolean isDirect() {
return hasMemoryAddress;
return nativeAddress != 0;
}
@Override
@ -1549,7 +1575,7 @@ public final class ByteBufAdaptor extends ByteBuf {
@Override
public boolean hasMemoryAddress() {
return hasMemoryAddress;
return isDirect();
}
@Override
@ -1557,7 +1583,7 @@ public final class ByteBufAdaptor extends ByteBuf {
if (!hasMemoryAddress()) {
throw new UnsupportedOperationException("No memory address associated with this buffer.");
}
return buffer.nativeAddress();
return nativeAddress;
}
@Override

3
buffer/src/main/java/io/netty/buffer/api/bytebuffer/NioBuffer.java
View File

@ -165,8 +165,7 @@ class NioBuffer extends AdaptableBuffer<NioBuffer> implements ReadableComponent,
return this;
}
@Override
public long nativeAddress() {
private long nativeAddress() {
return rmem.isDirect() && PlatformDependent.hasUnsafe()? PlatformDependent.directBufferAddress(rmem) : 0;
}

11
buffer/src/main/java/io/netty/buffer/api/unsafe/UnsafeBuffer.java
View File

@ -135,8 +135,7 @@ class UnsafeBuffer extends AdaptableBuffer<UnsafeBuffer> implements ReadableComp
return this;
}
@Override
public long nativeAddress() {
private long nativeAddress() {
return base == null? address : 0;
}
@ -232,14 +231,16 @@ class UnsafeBuffer extends AdaptableBuffer<UnsafeBuffer> implements ReadableComp
if (dest.readOnly()) {
throw bufferIsReadOnly(this);
}
long nativeAddress = dest.nativeAddress();
try {
if (dest instanceof UnsafeBuffer) {
UnsafeBuffer destUnsafe = (UnsafeBuffer) dest;
long nativeAddress = destUnsafe.nativeAddress();
if (nativeAddress != 0) {
PlatformDependent.copyMemory(base, address + srcPos, null, nativeAddress + destPos, length);
} else if (dest instanceof UnsafeBuffer) {
UnsafeBuffer destUnsafe = (UnsafeBuffer) dest;
} else {
PlatformDependent.copyMemory(
base, address + srcPos, destUnsafe.base, destUnsafe.address + destPos, length);
}
} else {
Statics.copyToViaReverseLoop(this, srcPos, dest, destPos, length);
}

50
buffer/src/test/java/io/netty/buffer/api/tests/BufferBulkAccessTest.java
View File

@ -233,7 +233,30 @@ public class BufferBulkAccessTest extends BufferTestSupport {
public void heapBufferMustHaveZeroAddress(Fixture fixture) {
try (BufferAllocator allocator = fixture.createAllocator();
Buffer buf = allocator.allocate(8)) {
assertThat(buf.nativeAddress()).isZero();
assertThat(buf.countReadableComponents()).isZero();
assertThat(buf.countWritableComponents()).isOne();
buf.forEachWritable(0, (index, component) -> {
assertThat(component.writableNativeAddress()).isZero();
return true;
});
buf.writeInt(42);
assertThat(buf.countReadableComponents()).isOne();
assertThat(buf.countWritableComponents()).isOne();
buf.forEachReadable(0, (index, component) -> {
assertThat(component.readableNativeAddress()).isZero();
return true;
});
buf.forEachWritable(0, (index, component) -> {
assertThat(component.writableNativeAddress()).isZero();
return true;
});
buf.writeInt(42);
assertThat(buf.countReadableComponents()).isOne();
assertThat(buf.countWritableComponents()).isZero();
buf.forEachReadable(0, (index, component) -> {
assertThat(component.readableNativeAddress()).isZero();
return true;
});
}
}
@ -242,7 +265,30 @@ public class BufferBulkAccessTest extends BufferTestSupport {
public void directBufferMustHaveNonZeroAddress(Fixture fixture) {
try (BufferAllocator allocator = fixture.createAllocator();
Buffer buf = allocator.allocate(8)) {
assertThat(buf.nativeAddress()).isNotZero();
assertThat(buf.countReadableComponents()).isZero();
assertThat(buf.countWritableComponents()).isOne();
buf.forEachWritable(0, (index, component) -> {
assertThat(component.writableNativeAddress()).isNotZero();
return true;
});
buf.writeInt(42);
assertThat(buf.countReadableComponents()).isOne();
assertThat(buf.countWritableComponents()).isOne();
buf.forEachReadable(0, (index, component) -> {
assertThat(component.readableNativeAddress()).isNotZero();
return true;
});
buf.forEachWritable(0, (index, component) -> {
assertThat(component.writableNativeAddress()).isNotZero();
return true;
});
buf.writeInt(42);
assertThat(buf.countReadableComponents()).isOne();
assertThat(buf.countWritableComponents()).isZero();
buf.forEachReadable(0, (index, component) -> {
assertThat(component.readableNativeAddress()).isNotZero();
return true;
});
}
}