From 669db5bd71d5f011d0f9419012c1a0a45c92e26c Mon Sep 17 00:00:00 2001 From: Matteo Merli Date: Fri, 9 Oct 2026 08:41:31 -0700 Subject: [PATCH 1/2] core: Support getByteBuffer() on byte array ReadableBuffers Since #12924, CompositeReadableBuffer coalesces runs of small buffers into a byte array wrapped with ReadableBuffers.wrap(byte[]). That wrapper did not support getByteBuffer(), so once a message contained coalesced buffers, byteBufferSupported() returned false for the whole message and marshallers reading it through HasByteBuffer had to copy it. The byte array wrapper now returns its unread bytes as a ByteBuffer slice of the backing array, as Netty does for heap ByteBufs. --- .../io/grpc/internal/ReadableBuffers.java | 10 ++++ .../internal/CompositeReadableBufferTest.java | 28 +++++++++++ .../internal/ReadableBuffersArrayTest.java | 48 +++++++++++++++++++ 3 files changed, 86 insertions(+) diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index 439745e29b2..2880f9424fc 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -218,6 +218,16 @@ public void reset() { } offset = mark; } + + @Override + public boolean byteBufferSupported() { + return true; + } + + @Override + public ByteBuffer getByteBuffer() { + return ByteBuffer.wrap(bytes, offset, end - offset).slice(); + } } /** diff --git a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java index 6d9ac3b68eb..d4ac4b273cc 100644 --- a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java +++ b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java @@ -28,6 +28,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.nio.ByteBuffer; import java.nio.InvalidMarkException; import org.junit.After; import org.junit.Before; @@ -336,6 +337,33 @@ public void coalesceBeforeLargeBuffer() { assertEquals(1524, composite.readableBytes()); } + @Test + public void coalesceBeforeLargeBufferKeepsByteBufferSupport() { + composite = new CompositeReadableBuffer(); + // Small frames that support getByteBuffer(), as Netty's do + composite.addBuffer(ReadableBuffers.wrap(ByteBuffer.wrap("hello ".getBytes(UTF_8)))); + composite.addBuffer(ReadableBuffers.wrap(ByteBuffer.wrap("world".getBytes(UTF_8)))); + composite.addBuffer(ReadableBuffers.wrap(ByteBuffer.wrap(new byte[1024]))); + assertEquals(2, composite.getBufferCount()); + + assertTrue(composite.byteBufferSupported()); + assertEquals(ByteBuffer.wrap("hello world".getBytes(UTF_8)), composite.getByteBuffer()); + } + + @Test + public void coalesceOnMaxSmallBuffersKeepsByteBufferSupport() { + composite = new CompositeReadableBuffer(); + byte[] expected = new byte[1000]; + for (int i = 0; i < 1000; i++) { + expected[i] = (byte) i; + composite.addBuffer(ReadableBuffers.wrap(ByteBuffer.wrap(new byte[] {(byte) i}))); + } + assertEquals(1, composite.getBufferCount()); + + assertTrue(composite.byteBufferSupported()); + assertEquals(ByteBuffer.wrap(expected), composite.getByteBuffer()); + } + @Test public void largeBufferResetsTailSmallBufferCount() { composite = new CompositeReadableBuffer(); diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersArrayTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersArrayTest.java index 5b0fb02c611..ff138f8c8d6 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersArrayTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersArrayTest.java @@ -19,9 +19,11 @@ import static io.grpc.internal.ReadableBuffers.wrap; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import java.nio.ByteBuffer; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -46,6 +48,52 @@ public void bufferShouldExposeArray() { assertEquals(2, buffer.arrayOffset()); } + @Test + public void getByteBufferShouldExposeArrayWithoutCopying() { + byte[] array = msg.getBytes(UTF_8); + ReadableBuffer buffer = wrap(array, 1, msg.length() - 2); + assertTrue(buffer.byteBufferSupported()); + + ByteBuffer byteBuffer = buffer.getByteBuffer(); + assertEquals(ByteBuffer.wrap(array, 1, msg.length() - 2), byteBuffer); + assertSame(array, byteBuffer.array()); + assertEquals(1, byteBuffer.arrayOffset() + byteBuffer.position()); + // Bytes outside of the wrapped range are not reachable through the ByteBuffer. + assertEquals(msg.length() - 2, byteBuffer.capacity()); + } + + @Test + public void getByteBufferShouldStartAtReadPosition() { + ReadableBuffer buffer = buffer(); + buffer.readUnsignedByte(); + buffer.readBytes(new byte[2], 0, 2); + buffer.skipBytes(2); + assertEquals(ByteBuffer.wrap(msg.substring(5).getBytes(UTF_8)), buffer.getByteBuffer()); + + buffer.skipBytes(buffer.readableBytes()); + assertFalse(buffer.getByteBuffer().hasRemaining()); + } + + @Test + public void getByteBufferShouldBeLimitedToReadBytesSlice() { + ReadableBuffer buffer = buffer(); + buffer.skipBytes(5); + ReadableBuffer slice = buffer.readBytes(10); + assertEquals(ByteBuffer.wrap(msg.substring(5, 15).getBytes(UTF_8)), slice.getByteBuffer()); + } + + @Test + public void getByteBufferShouldFollowMarkAndReset() { + ReadableBuffer buffer = buffer(); + buffer.skipBytes(5); + buffer.mark(); + buffer.skipBytes(10); + assertEquals(ByteBuffer.wrap(msg.substring(15).getBytes(UTF_8)), buffer.getByteBuffer()); + + buffer.reset(); + assertEquals(ByteBuffer.wrap(msg.substring(5).getBytes(UTF_8)), buffer.getByteBuffer()); + } + @Override protected ReadableBuffer buffer() { return ReadableBuffers.wrap(msg.getBytes(UTF_8), 0, msg.length()); From 5d27912a63f45e0e6895aa0177fa9ccd6eabd98b Mon Sep 17 00:00:00 2001 From: Matteo Merli Date: Mon, 12 Oct 2026 14:45:50 -0700 Subject: [PATCH 2/2] core: Wrap coalesced buffers as a ByteBuffer instead Per review, leave ReadableBuffers.wrap(byte[]) as it was and fix only the coalescing. CompositeReadableBuffer now wraps the merged bytes with ReadableBuffers.wrap(ByteBuffer.wrap(...)), which supports getByteBuffer(), so merging small buffers no longer turns off byteBufferSupported(). --- .../internal/CompositeReadableBuffer.java | 5 +- .../io/grpc/internal/ReadableBuffers.java | 10 ---- .../internal/ReadableBuffersArrayTest.java | 48 ------------------- 3 files changed, 3 insertions(+), 60 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index 6578db5ba56..90f2e2b4417 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -123,8 +123,9 @@ private void coalesceTailSmallBuffers(int count) { b.close(); } - // Wrap and enqueue the single compacted buffer back at the tail - ReadableBuffer singleBuffer = ReadableBuffers.wrap(coalescedBytes); + // Wrap and enqueue the single compacted buffer back at the tail. Wrap it as a ByteBuffer, + // since wrap(byte[]) doesn't support getByteBuffer() and would disable it for the composite. + ReadableBuffer singleBuffer = ReadableBuffers.wrap(ByteBuffer.wrap(coalescedBytes)); readableBuffers.add(singleBuffer); // Note: The global `readableBytes` remains perfectly synced since we diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index 2880f9424fc..439745e29b2 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -218,16 +218,6 @@ public void reset() { } offset = mark; } - - @Override - public boolean byteBufferSupported() { - return true; - } - - @Override - public ByteBuffer getByteBuffer() { - return ByteBuffer.wrap(bytes, offset, end - offset).slice(); - } } /** diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersArrayTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersArrayTest.java index ff138f8c8d6..5b0fb02c611 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersArrayTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersArrayTest.java @@ -19,11 +19,9 @@ import static io.grpc.internal.ReadableBuffers.wrap; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; -import java.nio.ByteBuffer; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -48,52 +46,6 @@ public void bufferShouldExposeArray() { assertEquals(2, buffer.arrayOffset()); } - @Test - public void getByteBufferShouldExposeArrayWithoutCopying() { - byte[] array = msg.getBytes(UTF_8); - ReadableBuffer buffer = wrap(array, 1, msg.length() - 2); - assertTrue(buffer.byteBufferSupported()); - - ByteBuffer byteBuffer = buffer.getByteBuffer(); - assertEquals(ByteBuffer.wrap(array, 1, msg.length() - 2), byteBuffer); - assertSame(array, byteBuffer.array()); - assertEquals(1, byteBuffer.arrayOffset() + byteBuffer.position()); - // Bytes outside of the wrapped range are not reachable through the ByteBuffer. - assertEquals(msg.length() - 2, byteBuffer.capacity()); - } - - @Test - public void getByteBufferShouldStartAtReadPosition() { - ReadableBuffer buffer = buffer(); - buffer.readUnsignedByte(); - buffer.readBytes(new byte[2], 0, 2); - buffer.skipBytes(2); - assertEquals(ByteBuffer.wrap(msg.substring(5).getBytes(UTF_8)), buffer.getByteBuffer()); - - buffer.skipBytes(buffer.readableBytes()); - assertFalse(buffer.getByteBuffer().hasRemaining()); - } - - @Test - public void getByteBufferShouldBeLimitedToReadBytesSlice() { - ReadableBuffer buffer = buffer(); - buffer.skipBytes(5); - ReadableBuffer slice = buffer.readBytes(10); - assertEquals(ByteBuffer.wrap(msg.substring(5, 15).getBytes(UTF_8)), slice.getByteBuffer()); - } - - @Test - public void getByteBufferShouldFollowMarkAndReset() { - ReadableBuffer buffer = buffer(); - buffer.skipBytes(5); - buffer.mark(); - buffer.skipBytes(10); - assertEquals(ByteBuffer.wrap(msg.substring(15).getBytes(UTF_8)), buffer.getByteBuffer()); - - buffer.reset(); - assertEquals(ByteBuffer.wrap(msg.substring(5).getBytes(UTF_8)), buffer.getByteBuffer()); - } - @Override protected ReadableBuffer buffer() { return ReadableBuffers.wrap(msg.getBytes(UTF_8), 0, msg.length());