From 9936df1420789bfbfd5b864c8e9d7c76c5378762 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Sun, 16 Aug 2020 19:21:25 -0700 Subject: [PATCH 01/46] Add mark&reset methods and canUseByteBuffer&getByteBuffer methods to the interface. --- .../java/io/grpc/internal/ReadableBuffer.java | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffer.java b/core/src/main/java/io/grpc/internal/ReadableBuffer.java index 7d2ca7ebba5..c5f7e6ff268 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffer.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.io.OutputStream; import java.nio.ByteBuffer; +import javax.annotation.Nullable; /** * Interface for an abstract byte buffer. Buffers are intended to be a read-only, except for the @@ -123,6 +124,39 @@ public interface ReadableBuffer extends Closeable { */ int arrayOffset(); + /** + * Marks the current position in this buffer. A subsequent call to the {@link #reset} method + * repositions this stream at the last marked position so that subsequent reads re-read the same + * bytes. + */ + void mark(); + + /** + * Repositions this buffer to the position at the time {@link #mark} was last called on this + * buffer. + */ + void reset(); + + /** + * Indicates whether or not {@link #getByteBuffer} operation is supported for this buffer. + */ + boolean canUseByteBuffer(); + + /** + * Gets a {@link ByteBuffer} that contains up to {@code length} bytes of this buffer's content, + * or {@code null} if this buffer has been exhausted. The position of this buffer is unchanged + * after calling this method. The returned buffer's content should not be modified, but the + * position, limit, and mark may be changed. Operations for changing the position, limit, and + * mark of the returned {@link ByteBuffer} does not affect the position, limit, and mark of + * this buffer. {@link ByteBuffer}s returned by this method have independent position, limit + * and mark. This is an optional method, so callers should first check {@link #canUseByteBuffer}. + * + * @param length the maximum number of bytes to contain in returned {@link ByteBuffer}. + * @throws UnsupportedOperationException the buffer does not support this method. + */ + @Nullable + ByteBuffer getByteBuffer(int length); + /** * Closes this buffer and releases any resources. */ From aa93e1d46c23838e58e3d39087e6828cc79ad40b Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Sun, 16 Aug 2020 19:21:48 -0700 Subject: [PATCH 02/46] Default implementations. --- .../grpc/internal/AbstractReadableBuffer.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java index e43b7a7cc0e..a2d5b24ca34 100644 --- a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java @@ -16,6 +16,8 @@ package io.grpc.internal; +import java.nio.ByteBuffer; + /** * Abstract base class for {@link ReadableBuffer} implementations. */ @@ -45,6 +47,24 @@ public int arrayOffset() { throw new UnsupportedOperationException(); } + @Override + public void mark() {} + + @Override + public void reset() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean canUseByteBuffer() { + return false; + } + + @Override + public ByteBuffer getByteBuffer(int length) { + throw new UnsupportedOperationException(); + } + @Override public void close() {} From 454e224de35e7fe1c30ba4fffa9be903343102bb Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Sun, 16 Aug 2020 19:22:17 -0700 Subject: [PATCH 03/46] Wire new methods for forwarder --- .../internal/ForwardingReadableBuffer.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java index 954d0ac5486..6015c6baa04 100644 --- a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.io.OutputStream; import java.nio.ByteBuffer; +import javax.annotation.Nullable; /** * Base class for a wrapper around another {@link ReadableBuffer}. @@ -96,6 +97,27 @@ public int arrayOffset() { return buf.arrayOffset(); } + @Override + public void mark() { + buf.mark(); + } + + @Override + public void reset() { + buf.reset(); + } + + @Override + public boolean canUseByteBuffer() { + return buf.canUseByteBuffer(); + } + + @Nullable + @Override + public ByteBuffer getByteBuffer(int length) { + return buf.getByteBuffer(length); + } + @Override public void close() { buf.close(); From 3f47d5ec5af0483651aa78d845eddd48a576c8fc Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Sun, 16 Aug 2020 19:23:00 -0700 Subject: [PATCH 04/46] Support mark&reset and retrieving content via ByteBuffer for netty. --- .../io/grpc/netty/NettyReadableBuffer.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java index 37caccb0eb3..f47b294a7ae 100644 --- a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java +++ b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java @@ -94,6 +94,26 @@ public int arrayOffset() { return buffer.arrayOffset() + buffer.readerIndex(); } + @Override + public void mark() { + buffer.markReaderIndex(); + } + + @Override + public void reset() { + buffer.resetReaderIndex(); + } + + @Override + public boolean canUseByteBuffer() { + return buffer.isDirect() && buffer.nioBufferCount() > 0; + } + + @Override + public ByteBuffer getByteBuffer(int length) { + return buffer.nioBuffers(buffer.readerIndex(), length)[0]; + } + /** * If the first call to close, calls {@link ByteBuf#release} to release the internal Netty buffer. */ From b3c9974692d1680fcb12e80a566bcaa8f006c3cd Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Sun, 16 Aug 2020 19:23:21 -0700 Subject: [PATCH 05/46] Implementation for composite. --- .../internal/CompositeReadableBuffer.java | 119 ++++++++++++++---- 1 file changed, 95 insertions(+), 24 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index 9a9bf5c9266..99cfbd41e8c 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -20,8 +20,10 @@ import java.io.OutputStream; import java.nio.Buffer; import java.nio.ByteBuffer; +import java.nio.InvalidMarkException; import java.util.ArrayDeque; import java.util.Queue; +import javax.annotation.Nullable; /** * A {@link ReadableBuffer} that is composed of 0 or more {@link ReadableBuffer}s. This provides a @@ -34,7 +36,9 @@ public class CompositeReadableBuffer extends AbstractReadableBuffer { private int readableBytes; - private final Queue buffers = new ArrayDeque<>(); + private final Queue readableBuffers = new ArrayDeque<>(); + private final Queue rewindableBuffers = new ArrayDeque<>(); + private boolean marked; /** * Adds a new {@link ReadableBuffer} at the end of the buffer list. After a buffer is added, it is @@ -43,16 +47,24 @@ public class CompositeReadableBuffer extends AbstractReadableBuffer { * this {@code CompositeBuffer}. */ public void addBuffer(ReadableBuffer buffer) { + boolean markHead = marked && readableBuffers.isEmpty(); + enqueueBuffer(buffer); + if (markHead) { + readableBuffers.peek().mark(); + } + } + + private void enqueueBuffer(ReadableBuffer buffer) { if (!(buffer instanceof CompositeReadableBuffer)) { - buffers.add(buffer); + readableBuffers.add(buffer); readableBytes += buffer.readableBytes(); return; } CompositeReadableBuffer compositeBuffer = (CompositeReadableBuffer) buffer; - while (!compositeBuffer.buffers.isEmpty()) { - ReadableBuffer subBuffer = compositeBuffer.buffers.remove(); - buffers.add(subBuffer); + while (!compositeBuffer.readableBuffers.isEmpty()) { + ReadableBuffer subBuffer = compositeBuffer.readableBuffers.remove(); + readableBuffers.add(subBuffer); } readableBytes += compositeBuffer.readableBytes; compositeBuffer.readableBytes = 0; @@ -138,27 +150,78 @@ public int readInternal(ReadableBuffer buffer, int length) throws IOException { @Override public CompositeReadableBuffer readBytes(int length) { - checkReadable(length); - readableBytes -= length; - - CompositeReadableBuffer newBuffer = new CompositeReadableBuffer(); - while (length > 0) { - ReadableBuffer buffer = buffers.peek(); - if (buffer.readableBytes() > length) { + final CompositeReadableBuffer newBuffer = new CompositeReadableBuffer(); + execute(new ReadOperation() { + @Override + int readInternal(ReadableBuffer buffer, int length) { newBuffer.addBuffer(buffer.readBytes(length)); - length = 0; - } else { - newBuffer.addBuffer(buffers.poll()); - length -= buffer.readableBytes(); + return 0; } - } + }, length); return newBuffer; } + @Override + public void mark() { + while (!rewindableBuffers.isEmpty()) { + rewindableBuffers.remove().close(); + } + marked = true; + ReadableBuffer buffer = readableBuffers.peek(); + if (buffer != null) { + buffer.mark(); + } + } + + @Override + public void reset() { + if (!marked) { + throw new InvalidMarkException(); + } + ReadableBuffer buffer; + if ((buffer = readableBuffers.peek()) != null) { + int currentRemain = buffer.readableBytes(); + buffer.reset(); + readableBytes += (buffer.readableBytes() - currentRemain); + } + int size = readableBuffers.size(); + while ((buffer = rewindableBuffers.poll()) != null) { + buffer.reset(); + readableBuffers.add(buffer); + readableBytes += buffer.readableBytes(); + } + for (int i = 0; i < size; i++) { + readableBuffers.add(readableBuffers.remove()); + } + } + + @Override + public boolean canUseByteBuffer() { + for (ReadableBuffer buffer : readableBuffers) { + if (!buffer.canUseByteBuffer()) { + return false; + } + } + return true; + } + + @Nullable + @Override + public ByteBuffer getByteBuffer(int length) { + if (readableBuffers.isEmpty()) { + return null; + } + ReadableBuffer buffer = readableBuffers.peek(); + return buffer.getByteBuffer(Math.min(length, buffer.readableBytes())); + } + @Override public void close() { - while (!buffers.isEmpty()) { - buffers.remove().close(); + while (!readableBuffers.isEmpty()) { + readableBuffers.remove().close(); + } + while (!rewindableBuffers.isEmpty()) { + rewindableBuffers.remove().close(); } } @@ -169,12 +232,12 @@ public void close() { private void execute(ReadOperation op, int length) { checkReadable(length); - if (!buffers.isEmpty()) { + if (!readableBuffers.isEmpty()) { advanceBufferIfNecessary(); } - for (; length > 0 && !buffers.isEmpty(); advanceBufferIfNecessary()) { - ReadableBuffer buffer = buffers.peek(); + for (; length > 0 && !readableBuffers.isEmpty(); advanceBufferIfNecessary()) { + ReadableBuffer buffer = readableBuffers.peek(); int lengthToCopy = Math.min(length, buffer.readableBytes()); // Perform the read operation for this buffer. @@ -197,9 +260,17 @@ private void execute(ReadOperation op, int length) { * If the current buffer is exhausted, removes and closes it. */ private void advanceBufferIfNecessary() { - ReadableBuffer buffer = buffers.peek(); + ReadableBuffer buffer = readableBuffers.peek(); if (buffer.readableBytes() == 0) { - buffers.remove().close(); + if (marked) { + rewindableBuffers.add(readableBuffers.remove()); + ReadableBuffer next = readableBuffers.peek(); + if (next != null) { + next.mark(); + } + } else { + readableBuffers.remove().close(); + } } } From 2ede5256cd4b149795a596e2e3af9c0282293e93 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Sun, 16 Aug 2020 19:24:05 -0700 Subject: [PATCH 06/46] Define interface for accessing readable content via ByteBuffers. --- api/src/main/java/io/grpc/HasByteBuffer.java | 41 ++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 api/src/main/java/io/grpc/HasByteBuffer.java diff --git a/api/src/main/java/io/grpc/HasByteBuffer.java b/api/src/main/java/io/grpc/HasByteBuffer.java new file mode 100644 index 00000000000..8ba3449214e --- /dev/null +++ b/api/src/main/java/io/grpc/HasByteBuffer.java @@ -0,0 +1,41 @@ +/* + * Copyright 2020 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc; + +import java.nio.ByteBuffer; +import javax.annotation.Nullable; + +/** + * Extension to an {@link java.io.InputStream} whose content can be accessed as {@link + * ByteBuffer}s. + * + *

This can be used for optimizing the case for the consumer of a {@link ByteBuffer}-backed + * input stream supports efficient reading from {@link ByteBuffer}s directly. This turns the reader + * interface from an {@link java.io.InputStream} to {@link ByteBuffer}s, without copying the + * content to a byte array and read from it. + */ +// TODO(chengyuanzhang): add ExperimentalApi annotation. +public interface HasByteBuffer { + + /** + * Gets a {@link ByteBuffer} containing up to {@code length} bytes of the content, or {@code + * null} if has reached end of the content. + * @param length the maximum number of bytes to contain in returned {@link ByteBuffer}. + */ + @Nullable + ByteBuffer getByteBuffer(int length); +} From 3dca3121adf283252ed35b2a5eef24ba3bbe1984 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Sun, 16 Aug 2020 19:26:09 -0700 Subject: [PATCH 07/46] Implement mark&reset for simple readable buffers. --- .../io/grpc/internal/ReadableBuffers.java | 70 ++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index 34805420fa5..98dfa059df9 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -19,6 +19,7 @@ import static com.google.common.base.Charsets.UTF_8; import com.google.common.base.Preconditions; +import io.grpc.HasByteBuffer; import io.grpc.KnownLength; import java.io.IOException; import java.io.InputStream; @@ -26,6 +27,7 @@ import java.nio.Buffer; import java.nio.ByteBuffer; import java.nio.charset.Charset; +import javax.annotation.Nullable; /** * Utility methods for creating {@link ReadableBuffer} instances. @@ -103,7 +105,11 @@ public static String readAsStringUtf8(ReadableBuffer buffer) { * @param owner if {@code true}, the returned stream will close the buffer when closed. */ public static InputStream openStream(ReadableBuffer buffer, boolean owner) { - return new BufferInputStream(owner ? buffer : ignoreClose(buffer)); + if (!owner) { + buffer = ignoreClose(buffer); + } + return buffer.canUseByteBuffer() + ? new ByteBufferInputStream(buffer) : new BufferInputStream(buffer); } /** @@ -128,6 +134,7 @@ private static class ByteArrayWrapper extends AbstractReadableBuffer { int offset; final int end; final byte[] bytes; + int mark = -1; ByteArrayWrapper(byte[] bytes) { this(bytes, 0, bytes.length); @@ -204,6 +211,16 @@ public byte[] array() { public int arrayOffset() { return offset; } + + @Override + public void mark() { + mark = offset; + } + + @Override + public void reset() { + offset = mark; + } } /** @@ -292,12 +309,22 @@ public byte[] array() { public int arrayOffset() { return bytes.arrayOffset() + bytes.position(); } + + @Override + public void mark() { + bytes.mark(); + } + + @Override + public void reset() { + bytes.reset(); + } } /** * An {@link InputStream} that is backed by a {@link ReadableBuffer}. */ - private static final class BufferInputStream extends InputStream implements KnownLength { + private static class BufferInputStream extends InputStream implements KnownLength { final ReadableBuffer buffer; public BufferInputStream(ReadableBuffer buffer) { @@ -330,11 +357,50 @@ public int read(byte[] dest, int destOffset, int length) throws IOException { return length; } + @Override + public long skip(long n) throws IOException { + int length = (int) Math.min(buffer.readableBytes(), n); + buffer.skipBytes(length); + return length; + } + + @Override + public void mark(int readlimit) { + buffer.mark(); + } + + @Override + public void reset() throws IOException { + buffer.reset(); + } + + @Override + public boolean markSupported() { + return true; + } + @Override public void close() throws IOException { buffer.close(); } } + /** + * A {@link ReadableBuffer}-backed {@link InputStream} that supports the operation of getting + * bytes via {@link ByteBuffer}s. + */ + private static class ByteBufferInputStream extends BufferInputStream implements HasByteBuffer { + + ByteBufferInputStream(ReadableBuffer buffer) { + super(buffer); + } + + @Nullable + @Override + public ByteBuffer getByteBuffer(int length) { + return buffer.getByteBuffer(length); + } + } + private ReadableBuffers() {} } From 62493530fcf7d7cca0884968853437b29f6acd75 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Sun, 16 Aug 2020 19:26:47 -0700 Subject: [PATCH 08/46] Use HasByteBuffer interface for accesing input stream's backing ByteBuffer. --- .../io/grpc/protobuf/lite/ProtoLiteUtils.java | 67 ++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index ddba5b8d5b1..7c9356ac7ed 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -25,6 +25,7 @@ import com.google.protobuf.MessageLite; import com.google.protobuf.Parser; import io.grpc.ExperimentalApi; +import io.grpc.HasByteBuffer; import io.grpc.KnownLength; import io.grpc.Metadata; import io.grpc.MethodDescriptor.Marshaller; @@ -35,6 +36,9 @@ import java.io.OutputStream; import java.lang.ref.Reference; import java.lang.ref.WeakReference; +import java.nio.ByteBuffer; +import java.util.Iterator; +import java.util.NoSuchElementException; /** * Utility methods for using protobuf with grpc. @@ -173,7 +177,11 @@ public T parse(InputStream stream) { try { if (stream instanceof KnownLength) { int size = stream.available(); - if (size > 0 && size <= DEFAULT_MAX_MESSAGE_SIZE) { + // TODO(chengyuanzhang): we may still want to go with the byte array approach for small + // messages. + if (stream instanceof HasByteBuffer && stream.markSupported()) { + cis = CodedInputStream.newInstance(new KnownLengthByteBufferIterable(stream, size)); + } else if (size > 0 && size <= DEFAULT_MAX_MESSAGE_SIZE) { Reference ref; // buf should not be used after this method has returned. byte[] buf; @@ -255,4 +263,61 @@ public T parseBytes(byte[] serialized) { } } } + + private static final class KnownLengthByteBufferIterable implements Iterable { + private final InputStream stream; + private final int length; + + private KnownLengthByteBufferIterable(InputStream stream, int length) { + this.stream = stream; + this.length = length; + stream.mark(length); + } + + @Override + public Iterator iterator() { + try { + stream.reset(); + stream.mark(length); + return new Iterator() { + private ByteBuffer buffer; + + @Override + public boolean hasNext() { + if (buffer != null) { + return true; + } + try { + buffer = ((HasByteBuffer) stream).getByteBuffer(stream.available()); + } catch (IOException e) { + throw new RuntimeException(e); + } + return buffer != null; + } + + @Override + public ByteBuffer next() { + if (!hasNext()) { + throw new NoSuchElementException(); + } + ByteBuffer res = buffer; + try { + stream.skip(buffer.remaining()); + } catch (IOException e) { + throw new RuntimeException(e); + } + buffer = null; + return res; + } + + @Override + public void remove() { + throw new UnsupportedOperationException(); + } + }; + } catch (IOException e) { + throw new RuntimeException(e); + } + } + } } From 29dce6730c349e4db625c0be271a12f9eac21d2e Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 12:18:01 -0700 Subject: [PATCH 09/46] Eliminate the length argument for retrieving the ByteBuffer. --- api/src/main/java/io/grpc/HasByteBuffer.java | 11 +++++++---- .../grpc/internal/AbstractReadableBuffer.java | 2 +- .../grpc/internal/CompositeReadableBuffer.java | 5 ++--- .../internal/ForwardingReadableBuffer.java | 4 ++-- .../java/io/grpc/internal/ReadableBuffer.java | 18 +++++++++--------- .../java/io/grpc/internal/ReadableBuffers.java | 14 ++++++++++++-- .../io/grpc/netty/NettyReadableBuffer.java | 4 ++-- .../io/grpc/protobuf/lite/ProtoLiteUtils.java | 6 +----- 8 files changed, 36 insertions(+), 28 deletions(-) diff --git a/api/src/main/java/io/grpc/HasByteBuffer.java b/api/src/main/java/io/grpc/HasByteBuffer.java index 8ba3449214e..179d6935b41 100644 --- a/api/src/main/java/io/grpc/HasByteBuffer.java +++ b/api/src/main/java/io/grpc/HasByteBuffer.java @@ -32,10 +32,13 @@ public interface HasByteBuffer { /** - * Gets a {@link ByteBuffer} containing up to {@code length} bytes of the content, or {@code - * null} if has reached end of the content. - * @param length the maximum number of bytes to contain in returned {@link ByteBuffer}. + * Gets a {@link ByteBuffer} containing some bytes of the content next to be read, or {@code + * null} if has reached end of the content. The number of bytes contained in the returned buffer + * is implementation specific. Calling this method does not change the position of the input + * stream. The returned buffer's content should not be modified, but the position, limit, and + * mark may be changed. Operations for changing the position, limit, and mark of the returned + * buffer does not affect the position, limit, and mark of this input stream. */ @Nullable - ByteBuffer getByteBuffer(int length); + ByteBuffer getByteBuffer(); } diff --git a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java index a2d5b24ca34..659ccf4fbb4 100644 --- a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java @@ -61,7 +61,7 @@ public boolean canUseByteBuffer() { } @Override - public ByteBuffer getByteBuffer(int length) { + public ByteBuffer getByteBuffer() { throw new UnsupportedOperationException(); } diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index 99cfbd41e8c..0c893daeaa6 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -207,12 +207,11 @@ public boolean canUseByteBuffer() { @Nullable @Override - public ByteBuffer getByteBuffer(int length) { + public ByteBuffer getByteBuffer() { if (readableBuffers.isEmpty()) { return null; } - ReadableBuffer buffer = readableBuffers.peek(); - return buffer.getByteBuffer(Math.min(length, buffer.readableBytes())); + return readableBuffers.peek().getByteBuffer(); } @Override diff --git a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java index 6015c6baa04..816d1b516d8 100644 --- a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java @@ -114,8 +114,8 @@ public boolean canUseByteBuffer() { @Nullable @Override - public ByteBuffer getByteBuffer(int length) { - return buf.getByteBuffer(length); + public ByteBuffer getByteBuffer() { + return buf.getByteBuffer(); } @Override diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffer.java b/core/src/main/java/io/grpc/internal/ReadableBuffer.java index c5f7e6ff268..43b25dd799e 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffer.java @@ -143,19 +143,19 @@ public interface ReadableBuffer extends Closeable { boolean canUseByteBuffer(); /** - * Gets a {@link ByteBuffer} that contains up to {@code length} bytes of this buffer's content, - * or {@code null} if this buffer has been exhausted. The position of this buffer is unchanged - * after calling this method. The returned buffer's content should not be modified, but the - * position, limit, and mark may be changed. Operations for changing the position, limit, and - * mark of the returned {@link ByteBuffer} does not affect the position, limit, and mark of - * this buffer. {@link ByteBuffer}s returned by this method have independent position, limit - * and mark. This is an optional method, so callers should first check {@link #canUseByteBuffer}. + * Gets a {@link ByteBuffer} that contains some bytes of the content next to be read, or {@code + * null} if this buffer has been exhausted. The number of bytes contained in the returned buffer + * is implementation specific. The position of this buffer is unchanged after calling this + * method. The returned buffer's content should not be modified, but the position, limit, and + * mark may be changed. Operations for changing the position, limit, and mark of the returned + * buffer does not affect the position, limit, and mark of this buffer. Buffers returned by this + * method have independent position, limit and mark. This is an optional method, so callers + * should first check {@link #canUseByteBuffer}. * - * @param length the maximum number of bytes to contain in returned {@link ByteBuffer}. * @throws UnsupportedOperationException the buffer does not support this method. */ @Nullable - ByteBuffer getByteBuffer(int length); + ByteBuffer getByteBuffer(); /** * Closes this buffer and releases any resources. diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index 98dfa059df9..cae73b0e78f 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -319,6 +319,16 @@ public void mark() { public void reset() { bytes.reset(); } + + @Override + public boolean canUseByteBuffer() { + return true; + } + + @Override + public ByteBuffer getByteBuffer() { + return (ByteBuffer) bytes; + } } /** @@ -397,8 +407,8 @@ private static class ByteBufferInputStream extends BufferInputStream implements @Nullable @Override - public ByteBuffer getByteBuffer(int length) { - return buffer.getByteBuffer(length); + public ByteBuffer getByteBuffer() { + return buffer.getByteBuffer(); } } diff --git a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java index f47b294a7ae..e2babecbb34 100644 --- a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java +++ b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java @@ -110,8 +110,8 @@ public boolean canUseByteBuffer() { } @Override - public ByteBuffer getByteBuffer(int length) { - return buffer.nioBuffers(buffer.readerIndex(), length)[0]; + public ByteBuffer getByteBuffer() { + return buffer.nioBuffers()[0]; } /** diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index 7c9356ac7ed..10ae2901bf5 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -287,11 +287,7 @@ public boolean hasNext() { if (buffer != null) { return true; } - try { - buffer = ((HasByteBuffer) stream).getByteBuffer(stream.available()); - } catch (IOException e) { - throw new RuntimeException(e); - } + buffer = ((HasByteBuffer) stream).getByteBuffer(); return buffer != null; } From 22ea6c37148a273d51f7ece381c0ea8c377c4ec0 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 12:21:58 -0700 Subject: [PATCH 10/46] Do no require netty buffer to be direct from API's perspective. --- netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java index e2babecbb34..967eb1e7fe5 100644 --- a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java +++ b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java @@ -106,7 +106,7 @@ public void reset() { @Override public boolean canUseByteBuffer() { - return buffer.isDirect() && buffer.nioBufferCount() > 0; + return buffer.nioBufferCount() > 0; } @Override From 53e347c6c5c33670e0418afa8148652835b08420 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 12:30:42 -0700 Subject: [PATCH 11/46] Use Deque operations to avoid unncessary moves. --- .../io/grpc/internal/CompositeReadableBuffer.java | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index 0c893daeaa6..5e58ab103f2 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -22,7 +22,7 @@ import java.nio.ByteBuffer; import java.nio.InvalidMarkException; import java.util.ArrayDeque; -import java.util.Queue; +import java.util.Deque; import javax.annotation.Nullable; /** @@ -36,8 +36,8 @@ public class CompositeReadableBuffer extends AbstractReadableBuffer { private int readableBytes; - private final Queue readableBuffers = new ArrayDeque<>(); - private final Queue rewindableBuffers = new ArrayDeque<>(); + private final Deque readableBuffers = new ArrayDeque<>(); + private final Deque rewindableBuffers = new ArrayDeque<>(); private boolean marked; /** @@ -184,15 +184,11 @@ public void reset() { buffer.reset(); readableBytes += (buffer.readableBytes() - currentRemain); } - int size = readableBuffers.size(); - while ((buffer = rewindableBuffers.poll()) != null) { + while ((buffer = rewindableBuffers.pollLast()) != null) { buffer.reset(); - readableBuffers.add(buffer); + readableBuffers.addFirst(buffer); readableBytes += buffer.readableBytes(); } - for (int i = 0; i < size; i++) { - readableBuffers.add(readableBuffers.remove()); - } } @Override From 27801fea9d6786d3d1722162d003f7589b37b9b3 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 12:36:38 -0700 Subject: [PATCH 12/46] Make a list of ByteBuffers up-front instead of a running iterator. --- .../io/grpc/protobuf/lite/ProtoLiteUtils.java | 67 +++---------------- 1 file changed, 11 insertions(+), 56 deletions(-) diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index 10ae2901bf5..d17b165af2d 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -37,8 +37,8 @@ import java.lang.ref.Reference; import java.lang.ref.WeakReference; import java.nio.ByteBuffer; -import java.util.Iterator; -import java.util.NoSuchElementException; +import java.util.ArrayList; +import java.util.List; /** * Utility methods for using protobuf with grpc. @@ -180,7 +180,15 @@ public T parse(InputStream stream) { // TODO(chengyuanzhang): we may still want to go with the byte array approach for small // messages. if (stream instanceof HasByteBuffer && stream.markSupported()) { - cis = CodedInputStream.newInstance(new KnownLengthByteBufferIterable(stream, size)); + List buffers = new ArrayList<>(); + stream.mark(size); + while (stream.available() != 0) { + ByteBuffer buffer = ((HasByteBuffer) stream).getByteBuffer(); + stream.skip(buffer.remaining()); + buffers.add(buffer); + } + stream.reset(); + cis = CodedInputStream.newInstance(buffers); } else if (size > 0 && size <= DEFAULT_MAX_MESSAGE_SIZE) { Reference ref; // buf should not be used after this method has returned. @@ -263,57 +271,4 @@ public T parseBytes(byte[] serialized) { } } } - - private static final class KnownLengthByteBufferIterable implements Iterable { - private final InputStream stream; - private final int length; - - private KnownLengthByteBufferIterable(InputStream stream, int length) { - this.stream = stream; - this.length = length; - stream.mark(length); - } - - @Override - public Iterator iterator() { - try { - stream.reset(); - stream.mark(length); - return new Iterator() { - private ByteBuffer buffer; - - @Override - public boolean hasNext() { - if (buffer != null) { - return true; - } - buffer = ((HasByteBuffer) stream).getByteBuffer(); - return buffer != null; - } - - @Override - public ByteBuffer next() { - if (!hasNext()) { - throw new NoSuchElementException(); - } - ByteBuffer res = buffer; - try { - stream.skip(buffer.remaining()); - } catch (IOException e) { - throw new RuntimeException(e); - } - buffer = null; - return res; - } - - @Override - public void remove() { - throw new UnsupportedOperationException(); - } - }; - } catch (IOException e) { - throw new RuntimeException(e); - } - } - } } From eb71a68dfb6697369d99df68b756b0f3e0909280 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 12:55:46 -0700 Subject: [PATCH 13/46] Add getByteBufferSupported method for HasByteBuffer so that it can be used for okhttp as well. --- api/src/main/java/io/grpc/HasByteBuffer.java | 10 ++++++- .../io/grpc/internal/ReadableBuffers.java | 29 +++++++------------ .../io/grpc/protobuf/lite/ProtoLiteUtils.java | 3 +- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/api/src/main/java/io/grpc/HasByteBuffer.java b/api/src/main/java/io/grpc/HasByteBuffer.java index 179d6935b41..915b6b3c8a5 100644 --- a/api/src/main/java/io/grpc/HasByteBuffer.java +++ b/api/src/main/java/io/grpc/HasByteBuffer.java @@ -31,13 +31,21 @@ // TODO(chengyuanzhang): add ExperimentalApi annotation. public interface HasByteBuffer { + /** + * Indicates whether or not {@link #getByteBuffer} operation is supported. + */ + boolean getByteBufferSupported(); + /** * Gets a {@link ByteBuffer} containing some bytes of the content next to be read, or {@code * null} if has reached end of the content. The number of bytes contained in the returned buffer * is implementation specific. Calling this method does not change the position of the input * stream. The returned buffer's content should not be modified, but the position, limit, and * mark may be changed. Operations for changing the position, limit, and mark of the returned - * buffer does not affect the position, limit, and mark of this input stream. + * buffer does not affect the position, limit, and mark of this input stream. This is an optional + * method, so callers should first check {@link #getByteBufferSupported}. + * + * @throws UnsupportedOperationException if this operation is not supported. */ @Nullable ByteBuffer getByteBuffer(); diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index cae73b0e78f..7c4e6121025 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -105,11 +105,7 @@ public static String readAsStringUtf8(ReadableBuffer buffer) { * @param owner if {@code true}, the returned stream will close the buffer when closed. */ public static InputStream openStream(ReadableBuffer buffer, boolean owner) { - if (!owner) { - buffer = ignoreClose(buffer); - } - return buffer.canUseByteBuffer() - ? new ByteBufferInputStream(buffer) : new BufferInputStream(buffer); + return new BufferInputStream(owner ? buffer : ignoreClose(buffer)); } /** @@ -334,7 +330,8 @@ public ByteBuffer getByteBuffer() { /** * An {@link InputStream} that is backed by a {@link ReadableBuffer}. */ - private static class BufferInputStream extends InputStream implements KnownLength { + private static final class BufferInputStream extends InputStream + implements KnownLength, HasByteBuffer { final ReadableBuffer buffer; public BufferInputStream(ReadableBuffer buffer) { @@ -390,19 +387,8 @@ public boolean markSupported() { } @Override - public void close() throws IOException { - buffer.close(); - } - } - - /** - * A {@link ReadableBuffer}-backed {@link InputStream} that supports the operation of getting - * bytes via {@link ByteBuffer}s. - */ - private static class ByteBufferInputStream extends BufferInputStream implements HasByteBuffer { - - ByteBufferInputStream(ReadableBuffer buffer) { - super(buffer); + public boolean getByteBufferSupported() { + return buffer.canUseByteBuffer(); } @Nullable @@ -410,6 +396,11 @@ private static class ByteBufferInputStream extends BufferInputStream implements public ByteBuffer getByteBuffer() { return buffer.getByteBuffer(); } + + @Override + public void close() throws IOException { + buffer.close(); + } } private ReadableBuffers() {} diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index d17b165af2d..933f60bc2e3 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -179,7 +179,8 @@ public T parse(InputStream stream) { int size = stream.available(); // TODO(chengyuanzhang): we may still want to go with the byte array approach for small // messages. - if (stream instanceof HasByteBuffer && stream.markSupported()) { + if (stream instanceof HasByteBuffer + && stream.markSupported() && ((HasByteBuffer) stream).getByteBufferSupported()) { List buffers = new ArrayList<>(); stream.mark(size); while (stream.available() != 0) { From 5d3c65769fd4210d5600e5ac76089164a1d282e7 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 14:27:47 -0700 Subject: [PATCH 14/46] It's not necessary to implement getByteBuffer for ByteReadbaleBufferWrapper. --- .../main/java/io/grpc/internal/ReadableBuffers.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index 7c4e6121025..fc5535a147a 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -315,16 +315,6 @@ public void mark() { public void reset() { bytes.reset(); } - - @Override - public boolean canUseByteBuffer() { - return true; - } - - @Override - public ByteBuffer getByteBuffer() { - return (ByteBuffer) bytes; - } } /** From 9fd8d3c92e1cddf5fb5d978e164bc4b82168dd1c Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 15:31:12 -0700 Subject: [PATCH 15/46] Add test coverage for mark&reset and getByteBuffer for generic ByteBuffer. --- .../grpc/internal/ReadableBufferTestBase.java | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java b/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java index c53b89109cd..c99c58aab87 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java +++ b/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java @@ -23,6 +23,7 @@ import java.io.ByteArrayOutputStream; import java.nio.ByteBuffer; import java.util.Arrays; +import org.junit.Assume; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -116,6 +117,58 @@ public void partialReadToReadableBufferShouldSucceed() { assertArrayEquals(new byte[] {'h', 'e'}, Arrays.copyOfRange(array, 0, 2)); } + @Test + public void markAndResetWithMixedReadShouldSucceed() { + ReadableBuffer buffer = buffer(); + int offset = 5; + buffer.readBytes(new byte[offset], 0, offset); + buffer.mark(); + int b = buffer.readUnsignedByte(); + assertEquals(msg.length() - offset - 1, buffer.readableBytes()); + buffer.reset(); + assertEquals(msg.length() - offset, buffer.readableBytes()); + assertEquals(b, buffer.readUnsignedByte()); + } + + @Test + public void markAndResetWithReadToReadableBufferShouldSucceed() { + ReadableBuffer buffer = buffer(); + int offset = 5; + buffer.readBytes(offset); + int testLen = 100; + buffer.mark(); + ReadableBuffer first = buffer.readBytes(testLen); + assertEquals(msg.length() - offset - testLen, buffer.readableBytes()); + buffer.reset(); + assertEquals(msg.length() - offset, buffer.readableBytes()); + ReadableBuffer second = buffer.readBytes(testLen); + byte[] array1 = new byte[testLen]; + byte[] array2 = new byte[testLen]; + first.readBytes(array1, 0, testLen); + second.readBytes(array2, 0, testLen); + assertArrayEquals(array1, array2); + } + + @Test + public void getByteBufferDoesNotAffectBufferPosition() { + ReadableBuffer buffer = buffer(); + Assume.assumeTrue(buffer.canUseByteBuffer()); + ByteBuffer byteBuffer = buffer.getByteBuffer(); + assertEquals(msg.length(), buffer.readableBytes()); + byteBuffer.get(new byte[byteBuffer.remaining()]); + assertEquals(msg.length(), buffer.readableBytes()); + } + + @Test + public void getByteBufferIsNotAffectedByBufferRead() { + ReadableBuffer buffer = buffer(); + Assume.assumeTrue(buffer.canUseByteBuffer()); + ByteBuffer byteBuffer = buffer.getByteBuffer(); + int initialRemaining = byteBuffer.remaining(); + buffer.readBytes(new byte[100], 0, 100); + assertEquals(initialRemaining, byteBuffer.remaining()); + } + protected abstract ReadableBuffer buffer(); private static String repeatUntilLength(String toRepeat, int length) { From e3afe50d1410efb5e0cbba2ed09b09c7ddd02663 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 15:32:24 -0700 Subject: [PATCH 16/46] Add test coverage for netty's special get NIO bytebuffer operation. --- .../grpc/netty/NettyReadableBufferTest.java | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java b/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java index 8090e601911..65af8beb6e2 100644 --- a/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java @@ -17,11 +17,16 @@ package io.grpc.netty; import static com.google.common.base.Charsets.UTF_8; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import com.google.common.base.Splitter; import io.grpc.internal.ReadableBuffer; import io.grpc.internal.ReadableBufferTestBase; +import io.netty.buffer.CompositeByteBuf; import io.netty.buffer.Unpooled; +import java.nio.ByteBuffer; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -52,6 +57,29 @@ public void closeMultipleTimesShouldReleaseBufferOnce() { assertEquals(0, buffer.buffer().refCnt()); } + @Test + public void getByteBufferFromSingleNioBufferBackedBuffer() { + assertTrue(buffer.canUseByteBuffer()); + ByteBuffer byteBuffer = buffer.getByteBuffer(); + byte[] arr = new byte[byteBuffer.remaining()]; + byteBuffer.get(arr); + assertArrayEquals(msg.getBytes(UTF_8), arr); + } + + @Test + public void getByteBufferFromCompositeBufferReturnsOnlyFirstComponent() { + CompositeByteBuf composite = Unpooled.compositeBuffer(10); + int chunks = 4; + int chunkLen = msg.length() / chunks; + for (String chunk : Splitter.fixedLength(chunkLen).split(msg)) { + composite.addComponent(true, Unpooled.copiedBuffer(chunk.getBytes(UTF_8))); + } + buffer = new NettyReadableBuffer(composite); + byte[] array = new byte[chunkLen]; + buffer.getByteBuffer().get(array); + assertArrayEquals(msg.substring(0, chunkLen).getBytes(UTF_8), array); + } + @Override protected ReadableBuffer buffer() { return buffer; From e2fdd07737430fbb95a48f1470f18be636f51e4e Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 15:32:44 -0700 Subject: [PATCH 17/46] Skip test for operations not supported by okhttp. --- .../io/grpc/okhttp/OkHttpReadableBufferTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpReadableBufferTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpReadableBufferTest.java index 2ece98ffb97..f1145a1131b 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpReadableBufferTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpReadableBufferTest.java @@ -56,6 +56,18 @@ public void partialReadToByteBufferShouldSucceed() { // Not supported. } + @Override + @Test + public void markAndResetWithMixedReadShouldSucceed() { + // Not supported. + } + + @Override + @Test + public void markAndResetWithReadToReadableBufferShouldSucceed() { + // Not supported. + } + @Override protected ReadableBuffer buffer() { return buffer; From 033270b1718b649ddab4114fe02b12881dec6263 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 15:33:15 -0700 Subject: [PATCH 18/46] Add test coverage for BufferInputStream with getByteBuffer operation. --- .../io/grpc/internal/ReadableBuffersTest.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java index ea9daeed6a3..fc653bd225d 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java @@ -19,11 +19,15 @@ import static com.google.common.base.Charsets.UTF_8; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import io.grpc.HasByteBuffer; +import java.io.IOException; import java.io.InputStream; import org.junit.Test; import org.junit.runner.RunWith; @@ -128,4 +132,30 @@ public void bufferInputStream_close_closesBuffer() throws Exception { inputStream.close(); verify(buffer, times(1)).close(); } + + @Test + public void bufferInputStream_markAndReset() throws IOException { + ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); + InputStream inputStream = ReadableBuffers.openStream(buffer, true); + assertTrue(inputStream.markSupported()); + inputStream.mark(2); + byte[] first = new byte[5]; + inputStream.read(first); + assertEquals(0, inputStream.available()); + inputStream.reset(); + assertEquals(5, inputStream.available()); + byte[] second = new byte[5]; + inputStream.read(second); + assertArrayEquals(first, second); + } + + @Test + public void bufferInputStream_getByteBufferDelegatesToBuffer() { + ReadableBuffer buffer = mock(ReadableBuffer.class); + when(buffer.canUseByteBuffer()).thenReturn(true); + InputStream inputStream = ReadableBuffers.openStream(buffer, true); + assertTrue(((HasByteBuffer) inputStream).getByteBufferSupported()); + ((HasByteBuffer) inputStream).getByteBuffer(); + verify(buffer).getByteBuffer(); + } } From 0622d5132cede33f2a54259a644d7a4bc47f4e71 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 15:33:46 -0700 Subject: [PATCH 19/46] Add test using a known-length input stream with getByteBuffer operation for protobuf parse. --- .../protobuf/lite/ProtoLiteUtilsTest.java | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java index d05e884105e..5a9ee228945 100644 --- a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java +++ b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java @@ -29,6 +29,7 @@ import com.google.protobuf.InvalidProtocolBufferException; import com.google.protobuf.Type; import io.grpc.Drainable; +import io.grpc.HasByteBuffer; import io.grpc.KnownLength; import io.grpc.Metadata; import io.grpc.MethodDescriptor.Marshaller; @@ -40,7 +41,9 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; +import java.nio.ByteBuffer; import java.util.Arrays; +import javax.annotation.Nullable; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -226,6 +229,16 @@ public void parseFromKnowLengthInputStream() throws Exception { assertEquals(expect, result); } + @Test + public void parseFromKnownLengthByteBufferInputStream() { + Marshaller marshaller = ProtoLiteUtils.marshaller(Type.getDefaultInstance()); + Type expect = Type.newBuilder().setName("expected name").build(); + + Type result = marshaller.parse( + new CustomKnownLengthByteBufferInputStream(expect.toByteString().asReadOnlyByteBuffer())); + assertEquals(expect, result); + } + @Test public void defaultMaxMessageSize() { assertEquals(GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE, ProtoLiteUtils.DEFAULT_MAX_MESSAGE_SIZE); @@ -253,4 +266,55 @@ public int read() throws IOException { return source[position++]; } } + + private static final class CustomKnownLengthByteBufferInputStream extends InputStream + implements KnownLength, HasByteBuffer { + private ByteBuffer source; + + private CustomKnownLengthByteBufferInputStream(ByteBuffer source) { + this.source = source; + } + + @Override + public int available() throws IOException { + return source.remaining(); + } + + @Override + public synchronized void mark(int readlimit) { + source.mark(); + } + + @Override + public synchronized void reset() throws IOException { + source.reset(); + } + + @Override + public boolean markSupported() { + return true; + } + + @Override + public int read() throws IOException { + throw new UnsupportedOperationException("should not be called"); + } + + @Override + public long skip(long n) throws IOException { + source.position((int) (source.position() + n)); + return n; + } + + @Override + public boolean getByteBufferSupported() { + return true; + } + + @Nullable + @Override + public ByteBuffer getByteBuffer() { + return source.slice(); + } + } } From 0e8caeeedb6b71d3a2b03ace98ec2de63c88f940 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 16:52:46 -0700 Subject: [PATCH 20/46] Modify test method name. --- core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java | 2 +- .../src/test/java/io/grpc/okhttp/OkHttpReadableBufferTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java b/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java index c99c58aab87..b28bb292db8 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java +++ b/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java @@ -118,7 +118,7 @@ public void partialReadToReadableBufferShouldSucceed() { } @Test - public void markAndResetWithMixedReadShouldSucceed() { + public void markAndResetWithReadShouldSucceed() { ReadableBuffer buffer = buffer(); int offset = 5; buffer.readBytes(new byte[offset], 0, offset); diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpReadableBufferTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpReadableBufferTest.java index f1145a1131b..4aeeae2fa8b 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpReadableBufferTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpReadableBufferTest.java @@ -58,7 +58,7 @@ public void partialReadToByteBufferShouldSucceed() { @Override @Test - public void markAndResetWithMixedReadShouldSucceed() { + public void markAndResetWithReadShouldSucceed() { // Not supported. } From ba4e91bc796f398ce19f4f7955c388d0b9f051c0 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 17 Aug 2020 16:53:17 -0700 Subject: [PATCH 21/46] Add test coverage for mark&reset and getByteBuffer for CompositeReadableByteBuffer. --- .../internal/CompositeReadableBufferTest.java | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) diff --git a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java index a73df727dd7..ab550a5c5b5 100644 --- a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java +++ b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java @@ -17,13 +17,19 @@ package io.grpc.internal; import static com.google.common.base.Charsets.UTF_8; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.nio.ByteBuffer; +import java.nio.InvalidMarkException; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -153,6 +159,128 @@ public void readStreamShouldSucceed() throws IOException { assertEquals(EXPECTED_VALUE, new String(bos.toByteArray(), UTF_8)); } + @Test + public void resetUnmarkedShouldThrow() { + try { + composite.reset(); + fail(); + } catch (InvalidMarkException expected) { + } + } + + @Test + public void markAndResetWithSkipBytesShouldSucceed() { + composite.mark(); + composite.skipBytes(EXPECTED_VALUE.length() / 2); + composite.reset(); + assertEquals(EXPECTED_VALUE.length(), composite.readableBytes()); + } + + @Test + public void markAndResetWithReadUnsignedByteShouldSucceed() { + composite.readUnsignedByte(); + composite.mark(); + int b = composite.readUnsignedByte(); + composite.reset(); + assertEquals(EXPECTED_VALUE.length() - 1, composite.readableBytes()); + assertEquals(b, composite.readUnsignedByte()); + } + + @Test + public void markAndResetWithReadByteArrayShouldSucceed() { + composite.mark(); + byte[] first = new byte[EXPECTED_VALUE.length()]; + composite.readBytes(first, 0, EXPECTED_VALUE.length()); + composite.reset(); + assertEquals(EXPECTED_VALUE.length(), composite.readableBytes()); + byte[] second = new byte[EXPECTED_VALUE.length()]; + composite.readBytes(second, 0, EXPECTED_VALUE.length()); + assertArrayEquals(first, second); + } + + @Test + public void markAndResetWithReadByteBufferShouldSucceed() { + byte[] first = new byte[EXPECTED_VALUE.length()]; + composite.mark(); + composite.readBytes(ByteBuffer.wrap(first)); + composite.reset(); + byte[] second = new byte[EXPECTED_VALUE.length()]; + assertEquals(EXPECTED_VALUE.length(), composite.readableBytes()); + composite.readBytes(ByteBuffer.wrap(second)); + assertArrayEquals(first, second); + } + + @Test + public void markAndResetWithReadStreamShouldSucceed() throws IOException { + ByteArrayOutputStream first = new ByteArrayOutputStream(); + composite.mark(); + composite.readBytes(first, EXPECTED_VALUE.length() / 2); + composite.reset(); + assertEquals(EXPECTED_VALUE.length(), composite.readableBytes()); + ByteArrayOutputStream second = new ByteArrayOutputStream(); + composite.readBytes(second, EXPECTED_VALUE.length() / 2); + assertArrayEquals(first.toByteArray(), second.toByteArray()); + } + + @Test + public void markAndResetWithReadReadableBufferShouldSucceed() { + composite.readBytes(EXPECTED_VALUE.length() / 2); + int remaining = composite.readableBytes(); + composite.mark(); + ReadableBuffer first = composite.readBytes(1); + composite.reset(); + assertEquals(remaining, composite.readableBytes()); + ReadableBuffer second = composite.readBytes(1); + assertEquals(first.readUnsignedByte(), second.readUnsignedByte()); + } + + @Test + public void markAgainShouldOverwritePreviousMark() { + composite.mark(); + composite.skipBytes(EXPECTED_VALUE.length() / 2); + int remaining = composite.readableBytes(); + composite.mark(); + composite.skipBytes(1); + composite.reset(); + assertEquals(remaining, composite.readableBytes()); + } + + @Test + public void bufferAddedAfterMarkedShouldBeIncluded() { + composite = new CompositeReadableBuffer(); + composite.mark(); + splitAndAdd(EXPECTED_VALUE); + composite.skipBytes(EXPECTED_VALUE.length() / 2); + composite.reset(); + assertEquals(EXPECTED_VALUE.length(), composite.readableBytes()); + } + + @Test + public void canUseByteBufferOnlyAllComponentsSupportUsingByteBuffer() { + composite = new CompositeReadableBuffer(); + ReadableBuffer buffer1 = mock(ReadableBuffer.class); + ReadableBuffer buffer2 = mock(ReadableBuffer.class); + ReadableBuffer buffer3 = mock(ReadableBuffer.class); + when(buffer1.canUseByteBuffer()).thenReturn(true); + when(buffer2.canUseByteBuffer()).thenReturn(true); + when(buffer3.canUseByteBuffer()).thenReturn(false); + composite.addBuffer(buffer1); + assertTrue(composite.canUseByteBuffer()); + composite.addBuffer(buffer2); + assertTrue(composite.canUseByteBuffer()); + composite.addBuffer(buffer3); + assertFalse(composite.canUseByteBuffer()); + } + + @Test + public void getByteBufferDelegatesToComponents() { + composite = new CompositeReadableBuffer(); + ReadableBuffer buffer = mock(ReadableBuffer.class); + composite.addBuffer(buffer); + composite.getByteBuffer(); + verify(buffer).getByteBuffer(); + } + @Test public void closeShouldCloseBuffers() { composite = new CompositeReadableBuffer(); From 69618b2dee13b5e1e3ba94d8ddd1ca19c46d0ba1 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 20 Aug 2020 02:51:29 -0700 Subject: [PATCH 22/46] Add getByteBuffer support for ByteReadableBufferWrapper. --- .../main/java/io/grpc/internal/ReadableBuffers.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index fc5535a147a..5583d32590a 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -315,6 +315,16 @@ public void mark() { public void reset() { bytes.reset(); } + + @Override + public boolean canUseByteBuffer() { + return true; + } + + @Override + public ByteBuffer getByteBuffer() { + return ((ByteBuffer) bytes).slice(); + } } /** From 437857d834753c3d7de425579d5d745e568f0f36 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 21 Aug 2020 14:23:00 -0700 Subject: [PATCH 23/46] Only pull ByteBuffers when message is large. --- .../src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index 933f60bc2e3..c269de7179f 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -179,7 +179,9 @@ public T parse(InputStream stream) { int size = stream.available(); // TODO(chengyuanzhang): we may still want to go with the byte array approach for small // messages. - if (stream instanceof HasByteBuffer + if (size == 0) { + return defaultInstance; + } else if (size > 64 * 1024 && stream instanceof HasByteBuffer && stream.markSupported() && ((HasByteBuffer) stream).getByteBufferSupported()) { List buffers = new ArrayList<>(); stream.mark(size); @@ -214,8 +216,6 @@ public T parse(InputStream stream) { throw new RuntimeException("size inaccurate: " + size + " != " + position); } cis = CodedInputStream.newInstance(buf, 0, size); - } else if (size == 0) { - return defaultInstance; } } } catch (IOException e) { From 136350536508bf6825f6efa304925ee6328c84e2 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 28 Aug 2020 15:34:05 -0700 Subject: [PATCH 24/46] Run ByteBuffer codepath only in Java 9+. --- .../io/grpc/protobuf/lite/ProtoLiteUtils.java | 48 +++++++++++++++++-- .../protobuf/lite/ProtoLiteUtilsTest.java | 8 +++- 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index c269de7179f..6c554290570 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -37,6 +37,8 @@ import java.lang.ref.Reference; import java.lang.ref.WeakReference; import java.nio.ByteBuffer; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.List; @@ -52,12 +54,27 @@ public final class ProtoLiteUtils { private static final int BUF_SIZE = 8192; + private static final String JAVA_VERSION = getSystemProperty("java.specification.version"); + + /** + * Assume Java 9+ if it isn't Java 7 or Java 8. + */ + @VisibleForTesting + static final boolean IS_JAVA9_OR_HIGHER = + !"1.7".equals(JAVA_VERSION) && !"1.8".equals(JAVA_VERSION); + /** * The same value as {@link io.grpc.internal.GrpcUtil#DEFAULT_MAX_MESSAGE_SIZE}. */ @VisibleForTesting static final int DEFAULT_MAX_MESSAGE_SIZE = 4 * 1024 * 1024; + /** + * Threshold for passing {@link ByteBuffer}s directly into Protobuf. + */ + @VisibleForTesting + static final int MESSAGE_ZERO_COPY_THRESHOLD = 64 * 1024; + /** * Sets the global registry for proto marshalling shared across all servers and clients. * @@ -177,12 +194,14 @@ public T parse(InputStream stream) { try { if (stream instanceof KnownLength) { int size = stream.available(); - // TODO(chengyuanzhang): we may still want to go with the byte array approach for small - // messages. if (size == 0) { return defaultInstance; - } else if (size > 64 * 1024 && stream instanceof HasByteBuffer - && stream.markSupported() && ((HasByteBuffer) stream).getByteBufferSupported()) { + } + if (IS_JAVA9_OR_HIGHER + && size >= MESSAGE_ZERO_COPY_THRESHOLD + && stream instanceof HasByteBuffer + && ((HasByteBuffer) stream).getByteBufferSupported() + && stream.markSupported()) { List buffers = new ArrayList<>(); stream.mark(size); while (stream.available() != 0) { @@ -272,4 +291,25 @@ public T parseBytes(byte[] serialized) { } } } + + private static String getSystemProperty(final String key) { + String value; + if (System.getSecurityManager() == null) { + value = System.getProperty(key); + } else { + value = + AccessController.doPrivileged( + new PrivilegedAction() { + @Override + public String run() { + try { + return System.getProperty(key); + } catch (SecurityException e) { + return null; + } + } + }); + } + return value; + } } diff --git a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java index 5a9ee228945..1d5c1abc806 100644 --- a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java +++ b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java @@ -22,6 +22,7 @@ import static org.junit.Assert.assertSame; import static org.junit.Assert.fail; +import com.google.common.base.Strings; import com.google.common.io.ByteStreams; import com.google.protobuf.ByteString; import com.google.protobuf.Empty; @@ -44,6 +45,7 @@ import java.nio.ByteBuffer; import java.util.Arrays; import javax.annotation.Nullable; +import org.junit.Assume; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -231,8 +233,12 @@ public void parseFromKnowLengthInputStream() throws Exception { @Test public void parseFromKnownLengthByteBufferInputStream() { + Assume.assumeTrue(ProtoLiteUtils.IS_JAVA9_OR_HIGHER); Marshaller marshaller = ProtoLiteUtils.marshaller(Type.getDefaultInstance()); - Type expect = Type.newBuilder().setName("expected name").build(); + Type expect = + Type.newBuilder() + .setName(Strings.repeat("M", ProtoLiteUtils.MESSAGE_ZERO_COPY_THRESHOLD)) + .build(); Type result = marshaller.parse( new CustomKnownLengthByteBufferInputStream(expect.toByteString().asReadOnlyByteBuffer())); From c46eb73bc5f12d298ae3651fcec72601b85dfb43 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 28 Aug 2020 15:44:34 -0700 Subject: [PATCH 25/46] Slight improvement for avoiding array creation if not necessary. --- netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java index 967eb1e7fe5..8d4f0f4d95c 100644 --- a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java +++ b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java @@ -111,7 +111,7 @@ public boolean canUseByteBuffer() { @Override public ByteBuffer getByteBuffer() { - return buffer.nioBuffers()[0]; + return buffer.nioBufferCount() == 1 ? buffer.nioBuffer() : buffer.nioBuffers()[0]; } /** From 772b3bafdf98db2cc338a3acde17c8d3116bab6e Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 1 Sep 2020 02:39:34 -0700 Subject: [PATCH 26/46] Change ReadableBuffer#canUseByteBuffer to hasByteBuffer. --- .../io/grpc/internal/AbstractReadableBuffer.java | 2 +- .../io/grpc/internal/CompositeReadableBuffer.java | 4 ++-- .../io/grpc/internal/ForwardingReadableBuffer.java | 4 ++-- .../main/java/io/grpc/internal/ReadableBuffer.java | 4 ++-- .../main/java/io/grpc/internal/ReadableBuffers.java | 4 ++-- .../grpc/internal/CompositeReadableBufferTest.java | 12 ++++++------ .../io/grpc/internal/ReadableBufferTestBase.java | 4 ++-- .../java/io/grpc/internal/ReadableBuffersTest.java | 2 +- .../main/java/io/grpc/netty/NettyReadableBuffer.java | 2 +- .../java/io/grpc/netty/NettyReadableBufferTest.java | 2 +- 10 files changed, 20 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java index 659ccf4fbb4..46bc4a04926 100644 --- a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java @@ -56,7 +56,7 @@ public void reset() { } @Override - public boolean canUseByteBuffer() { + public boolean hasByteBuffer() { return false; } diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index 5f7e2c0a590..b9b26aed516 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -190,9 +190,9 @@ public void reset() { } @Override - public boolean canUseByteBuffer() { + public boolean hasByteBuffer() { for (ReadableBuffer buffer : readableBuffers) { - if (!buffer.canUseByteBuffer()) { + if (!buffer.hasByteBuffer()) { return false; } } diff --git a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java index 816d1b516d8..60fd979f562 100644 --- a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java @@ -108,8 +108,8 @@ public void reset() { } @Override - public boolean canUseByteBuffer() { - return buf.canUseByteBuffer(); + public boolean hasByteBuffer() { + return buf.hasByteBuffer(); } @Nullable diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffer.java b/core/src/main/java/io/grpc/internal/ReadableBuffer.java index 43b25dd799e..908efb1baff 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffer.java @@ -140,7 +140,7 @@ public interface ReadableBuffer extends Closeable { /** * Indicates whether or not {@link #getByteBuffer} operation is supported for this buffer. */ - boolean canUseByteBuffer(); + boolean hasByteBuffer(); /** * Gets a {@link ByteBuffer} that contains some bytes of the content next to be read, or {@code @@ -150,7 +150,7 @@ public interface ReadableBuffer extends Closeable { * mark may be changed. Operations for changing the position, limit, and mark of the returned * buffer does not affect the position, limit, and mark of this buffer. Buffers returned by this * method have independent position, limit and mark. This is an optional method, so callers - * should first check {@link #canUseByteBuffer}. + * should first check {@link #hasByteBuffer}. * * @throws UnsupportedOperationException the buffer does not support this method. */ diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index 94ff45fdffc..cf56d0eb60d 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -316,7 +316,7 @@ public void reset() { } @Override - public boolean canUseByteBuffer() { + public boolean hasByteBuffer() { return true; } @@ -387,7 +387,7 @@ public boolean markSupported() { @Override public boolean getByteBufferSupported() { - return buffer.canUseByteBuffer(); + return buffer.hasByteBuffer(); } @Nullable diff --git a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java index f8cc4052df8..10aac75add8 100644 --- a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java +++ b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java @@ -262,15 +262,15 @@ public void canUseByteBufferOnlyAllComponentsSupportUsingByteBuffer() { ReadableBuffer buffer1 = mock(ReadableBuffer.class); ReadableBuffer buffer2 = mock(ReadableBuffer.class); ReadableBuffer buffer3 = mock(ReadableBuffer.class); - when(buffer1.canUseByteBuffer()).thenReturn(true); - when(buffer2.canUseByteBuffer()).thenReturn(true); - when(buffer3.canUseByteBuffer()).thenReturn(false); + when(buffer1.hasByteBuffer()).thenReturn(true); + when(buffer2.hasByteBuffer()).thenReturn(true); + when(buffer3.hasByteBuffer()).thenReturn(false); composite.addBuffer(buffer1); - assertTrue(composite.canUseByteBuffer()); + assertTrue(composite.hasByteBuffer()); composite.addBuffer(buffer2); - assertTrue(composite.canUseByteBuffer()); + assertTrue(composite.hasByteBuffer()); composite.addBuffer(buffer3); - assertFalse(composite.canUseByteBuffer()); + assertFalse(composite.hasByteBuffer()); } @Test diff --git a/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java b/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java index 3a7cec24308..7d2763e6b5d 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java +++ b/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java @@ -153,7 +153,7 @@ public void markAndResetWithReadToReadableBufferShouldSucceed() { @Test public void getByteBufferDoesNotAffectBufferPosition() { ReadableBuffer buffer = buffer(); - Assume.assumeTrue(buffer.canUseByteBuffer()); + Assume.assumeTrue(buffer.hasByteBuffer()); ByteBuffer byteBuffer = buffer.getByteBuffer(); assertEquals(msg.length(), buffer.readableBytes()); byteBuffer.get(new byte[byteBuffer.remaining()]); @@ -163,7 +163,7 @@ public void getByteBufferDoesNotAffectBufferPosition() { @Test public void getByteBufferIsNotAffectedByBufferRead() { ReadableBuffer buffer = buffer(); - Assume.assumeTrue(buffer.canUseByteBuffer()); + Assume.assumeTrue(buffer.hasByteBuffer()); ByteBuffer byteBuffer = buffer.getByteBuffer(); int initialRemaining = byteBuffer.remaining(); buffer.readBytes(new byte[100], 0, 100); diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java index fc653bd225d..7be7da9b5af 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java @@ -152,7 +152,7 @@ public void bufferInputStream_markAndReset() throws IOException { @Test public void bufferInputStream_getByteBufferDelegatesToBuffer() { ReadableBuffer buffer = mock(ReadableBuffer.class); - when(buffer.canUseByteBuffer()).thenReturn(true); + when(buffer.hasByteBuffer()).thenReturn(true); InputStream inputStream = ReadableBuffers.openStream(buffer, true); assertTrue(((HasByteBuffer) inputStream).getByteBufferSupported()); ((HasByteBuffer) inputStream).getByteBuffer(); diff --git a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java index 8d4f0f4d95c..099618d601a 100644 --- a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java +++ b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java @@ -105,7 +105,7 @@ public void reset() { } @Override - public boolean canUseByteBuffer() { + public boolean hasByteBuffer() { return buffer.nioBufferCount() > 0; } diff --git a/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java b/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java index 65af8beb6e2..495cd5cb511 100644 --- a/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java @@ -59,7 +59,7 @@ public void closeMultipleTimesShouldReleaseBufferOnce() { @Test public void getByteBufferFromSingleNioBufferBackedBuffer() { - assertTrue(buffer.canUseByteBuffer()); + assertTrue(buffer.hasByteBuffer()); ByteBuffer byteBuffer = buffer.getByteBuffer(); byte[] arr = new byte[byteBuffer.remaining()]; byteBuffer.get(arr); From b1c99e551846a02ef5b9e23d0a79ef866cf3023c Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 1 Sep 2020 13:29:01 -0700 Subject: [PATCH 27/46] Removed unnecessary reset. --- .../src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java | 1 - 1 file changed, 1 deletion(-) diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index 6c554290570..d6729d12814 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -209,7 +209,6 @@ public T parse(InputStream stream) { stream.skip(buffer.remaining()); buffers.add(buffer); } - stream.reset(); cis = CodedInputStream.newInstance(buffers); } else if (size > 0 && size <= DEFAULT_MAX_MESSAGE_SIZE) { Reference ref; From 692076cae12eea3fff64701345b8000bb56e9fcd Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 1 Sep 2020 13:31:25 -0700 Subject: [PATCH 28/46] Simplify checking runtime java version. --- .../io/grpc/protobuf/lite/ProtoLiteUtils.java | 38 ++++++------------- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index d6729d12814..47a790f44fc 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -37,8 +37,6 @@ import java.lang.ref.Reference; import java.lang.ref.WeakReference; import java.nio.ByteBuffer; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.List; @@ -54,14 +52,11 @@ public final class ProtoLiteUtils { private static final int BUF_SIZE = 8192; - private static final String JAVA_VERSION = getSystemProperty("java.specification.version"); - /** * Assume Java 9+ if it isn't Java 7 or Java 8. */ @VisibleForTesting - static final boolean IS_JAVA9_OR_HIGHER = - !"1.7".equals(JAVA_VERSION) && !"1.8".equals(JAVA_VERSION); + static final boolean IS_JAVA9_OR_HIGHER; /** * The same value as {@link io.grpc.internal.GrpcUtil#DEFAULT_MAX_MESSAGE_SIZE}. @@ -75,6 +70,16 @@ public final class ProtoLiteUtils { @VisibleForTesting static final int MESSAGE_ZERO_COPY_THRESHOLD = 64 * 1024; + static { + boolean isJava9OrHigher = true; + try { + Class.forName("java.lang.StackWalker"); + } catch (ClassNotFoundException e) { + isJava9OrHigher = false; + } + IS_JAVA9_OR_HIGHER = isJava9OrHigher; + } + /** * Sets the global registry for proto marshalling shared across all servers and clients. * @@ -290,25 +295,4 @@ public T parseBytes(byte[] serialized) { } } } - - private static String getSystemProperty(final String key) { - String value; - if (System.getSecurityManager() == null) { - value = System.getProperty(key); - } else { - value = - AccessController.doPrivileged( - new PrivilegedAction() { - @Override - public String run() { - try { - return System.getProperty(key); - } catch (SecurityException e) { - return null; - } - } - }); - } - return value; - } } From 10c13b893a4c5c202fc2999abf8e1797addd07a5 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 1 Sep 2020 13:50:10 -0700 Subject: [PATCH 29/46] Add ExperimentalApi annotation. --- api/src/main/java/io/grpc/HasByteBuffer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/io/grpc/HasByteBuffer.java b/api/src/main/java/io/grpc/HasByteBuffer.java index 915b6b3c8a5..35abca53e6c 100644 --- a/api/src/main/java/io/grpc/HasByteBuffer.java +++ b/api/src/main/java/io/grpc/HasByteBuffer.java @@ -28,7 +28,7 @@ * interface from an {@link java.io.InputStream} to {@link ByteBuffer}s, without copying the * content to a byte array and read from it. */ -// TODO(chengyuanzhang): add ExperimentalApi annotation. +@ExperimentalApi("https://github.com/grpc/grpc-java/issues/7387") public interface HasByteBuffer { /** From f13c1657c94b087713f6268e43b35263b5a9d799 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Wed, 2 Sep 2020 02:09:40 -0700 Subject: [PATCH 30/46] Rename ReadableBuffer#hasByteBuffer to getByteBufferSupported. --- .../io/grpc/internal/AbstractReadableBuffer.java | 2 +- .../io/grpc/internal/CompositeReadableBuffer.java | 4 ++-- .../io/grpc/internal/ForwardingReadableBuffer.java | 4 ++-- .../main/java/io/grpc/internal/ReadableBuffer.java | 4 ++-- .../main/java/io/grpc/internal/ReadableBuffers.java | 4 ++-- .../grpc/internal/CompositeReadableBufferTest.java | 12 ++++++------ .../io/grpc/internal/ReadableBufferTestBase.java | 4 ++-- .../java/io/grpc/internal/ReadableBuffersTest.java | 2 +- .../main/java/io/grpc/netty/NettyReadableBuffer.java | 2 +- .../java/io/grpc/netty/NettyReadableBufferTest.java | 2 +- 10 files changed, 20 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java index 46bc4a04926..99812c3ffff 100644 --- a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java @@ -56,7 +56,7 @@ public void reset() { } @Override - public boolean hasByteBuffer() { + public boolean getByteBufferSupported() { return false; } diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index b9b26aed516..c1c2c78c5af 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -190,9 +190,9 @@ public void reset() { } @Override - public boolean hasByteBuffer() { + public boolean getByteBufferSupported() { for (ReadableBuffer buffer : readableBuffers) { - if (!buffer.hasByteBuffer()) { + if (!buffer.getByteBufferSupported()) { return false; } } diff --git a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java index 60fd979f562..0103f9fb39f 100644 --- a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java @@ -108,8 +108,8 @@ public void reset() { } @Override - public boolean hasByteBuffer() { - return buf.hasByteBuffer(); + public boolean getByteBufferSupported() { + return buf.getByteBufferSupported(); } @Nullable diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffer.java b/core/src/main/java/io/grpc/internal/ReadableBuffer.java index 908efb1baff..684039292aa 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffer.java @@ -140,7 +140,7 @@ public interface ReadableBuffer extends Closeable { /** * Indicates whether or not {@link #getByteBuffer} operation is supported for this buffer. */ - boolean hasByteBuffer(); + boolean getByteBufferSupported(); /** * Gets a {@link ByteBuffer} that contains some bytes of the content next to be read, or {@code @@ -150,7 +150,7 @@ public interface ReadableBuffer extends Closeable { * mark may be changed. Operations for changing the position, limit, and mark of the returned * buffer does not affect the position, limit, and mark of this buffer. Buffers returned by this * method have independent position, limit and mark. This is an optional method, so callers - * should first check {@link #hasByteBuffer}. + * should first check {@link #getByteBufferSupported}. * * @throws UnsupportedOperationException the buffer does not support this method. */ diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index cf56d0eb60d..f764d893fda 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -316,7 +316,7 @@ public void reset() { } @Override - public boolean hasByteBuffer() { + public boolean getByteBufferSupported() { return true; } @@ -387,7 +387,7 @@ public boolean markSupported() { @Override public boolean getByteBufferSupported() { - return buffer.hasByteBuffer(); + return buffer.getByteBufferSupported(); } @Nullable diff --git a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java index 10aac75add8..f26c8b1ef9a 100644 --- a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java +++ b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java @@ -262,15 +262,15 @@ public void canUseByteBufferOnlyAllComponentsSupportUsingByteBuffer() { ReadableBuffer buffer1 = mock(ReadableBuffer.class); ReadableBuffer buffer2 = mock(ReadableBuffer.class); ReadableBuffer buffer3 = mock(ReadableBuffer.class); - when(buffer1.hasByteBuffer()).thenReturn(true); - when(buffer2.hasByteBuffer()).thenReturn(true); - when(buffer3.hasByteBuffer()).thenReturn(false); + when(buffer1.getByteBufferSupported()).thenReturn(true); + when(buffer2.getByteBufferSupported()).thenReturn(true); + when(buffer3.getByteBufferSupported()).thenReturn(false); composite.addBuffer(buffer1); - assertTrue(composite.hasByteBuffer()); + assertTrue(composite.getByteBufferSupported()); composite.addBuffer(buffer2); - assertTrue(composite.hasByteBuffer()); + assertTrue(composite.getByteBufferSupported()); composite.addBuffer(buffer3); - assertFalse(composite.hasByteBuffer()); + assertFalse(composite.getByteBufferSupported()); } @Test diff --git a/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java b/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java index 7d2763e6b5d..86b8e2a399b 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java +++ b/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java @@ -153,7 +153,7 @@ public void markAndResetWithReadToReadableBufferShouldSucceed() { @Test public void getByteBufferDoesNotAffectBufferPosition() { ReadableBuffer buffer = buffer(); - Assume.assumeTrue(buffer.hasByteBuffer()); + Assume.assumeTrue(buffer.getByteBufferSupported()); ByteBuffer byteBuffer = buffer.getByteBuffer(); assertEquals(msg.length(), buffer.readableBytes()); byteBuffer.get(new byte[byteBuffer.remaining()]); @@ -163,7 +163,7 @@ public void getByteBufferDoesNotAffectBufferPosition() { @Test public void getByteBufferIsNotAffectedByBufferRead() { ReadableBuffer buffer = buffer(); - Assume.assumeTrue(buffer.hasByteBuffer()); + Assume.assumeTrue(buffer.getByteBufferSupported()); ByteBuffer byteBuffer = buffer.getByteBuffer(); int initialRemaining = byteBuffer.remaining(); buffer.readBytes(new byte[100], 0, 100); diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java index 7be7da9b5af..3007c19682a 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java @@ -152,7 +152,7 @@ public void bufferInputStream_markAndReset() throws IOException { @Test public void bufferInputStream_getByteBufferDelegatesToBuffer() { ReadableBuffer buffer = mock(ReadableBuffer.class); - when(buffer.hasByteBuffer()).thenReturn(true); + when(buffer.getByteBufferSupported()).thenReturn(true); InputStream inputStream = ReadableBuffers.openStream(buffer, true); assertTrue(((HasByteBuffer) inputStream).getByteBufferSupported()); ((HasByteBuffer) inputStream).getByteBuffer(); diff --git a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java index 099618d601a..75d70978610 100644 --- a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java +++ b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java @@ -105,7 +105,7 @@ public void reset() { } @Override - public boolean hasByteBuffer() { + public boolean getByteBufferSupported() { return buffer.nioBufferCount() > 0; } diff --git a/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java b/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java index 495cd5cb511..23c2f0632a9 100644 --- a/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java @@ -59,7 +59,7 @@ public void closeMultipleTimesShouldReleaseBufferOnce() { @Test public void getByteBufferFromSingleNioBufferBackedBuffer() { - assertTrue(buffer.hasByteBuffer()); + assertTrue(buffer.getByteBufferSupported()); ByteBuffer byteBuffer = buffer.getByteBuffer(); byte[] arr = new byte[byteBuffer.remaining()]; byteBuffer.get(arr); From 0f804b74a7770ead30b44995c607ce9db4c19752 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 20 Apr 2021 01:17:59 -0700 Subject: [PATCH 31/46] Revert changes for MessageMarshaller. --- .../io/grpc/protobuf/lite/ProtoLiteUtils.java | 46 +----------- .../protobuf/lite/ProtoLiteUtilsTest.java | 70 ------------------- 2 files changed, 3 insertions(+), 113 deletions(-) diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index 47a790f44fc..ddba5b8d5b1 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -25,7 +25,6 @@ import com.google.protobuf.MessageLite; import com.google.protobuf.Parser; import io.grpc.ExperimentalApi; -import io.grpc.HasByteBuffer; import io.grpc.KnownLength; import io.grpc.Metadata; import io.grpc.MethodDescriptor.Marshaller; @@ -36,9 +35,6 @@ import java.io.OutputStream; import java.lang.ref.Reference; import java.lang.ref.WeakReference; -import java.nio.ByteBuffer; -import java.util.ArrayList; -import java.util.List; /** * Utility methods for using protobuf with grpc. @@ -52,34 +48,12 @@ public final class ProtoLiteUtils { private static final int BUF_SIZE = 8192; - /** - * Assume Java 9+ if it isn't Java 7 or Java 8. - */ - @VisibleForTesting - static final boolean IS_JAVA9_OR_HIGHER; - /** * The same value as {@link io.grpc.internal.GrpcUtil#DEFAULT_MAX_MESSAGE_SIZE}. */ @VisibleForTesting static final int DEFAULT_MAX_MESSAGE_SIZE = 4 * 1024 * 1024; - /** - * Threshold for passing {@link ByteBuffer}s directly into Protobuf. - */ - @VisibleForTesting - static final int MESSAGE_ZERO_COPY_THRESHOLD = 64 * 1024; - - static { - boolean isJava9OrHigher = true; - try { - Class.forName("java.lang.StackWalker"); - } catch (ClassNotFoundException e) { - isJava9OrHigher = false; - } - IS_JAVA9_OR_HIGHER = isJava9OrHigher; - } - /** * Sets the global registry for proto marshalling shared across all servers and clients. * @@ -199,23 +173,7 @@ public T parse(InputStream stream) { try { if (stream instanceof KnownLength) { int size = stream.available(); - if (size == 0) { - return defaultInstance; - } - if (IS_JAVA9_OR_HIGHER - && size >= MESSAGE_ZERO_COPY_THRESHOLD - && stream instanceof HasByteBuffer - && ((HasByteBuffer) stream).getByteBufferSupported() - && stream.markSupported()) { - List buffers = new ArrayList<>(); - stream.mark(size); - while (stream.available() != 0) { - ByteBuffer buffer = ((HasByteBuffer) stream).getByteBuffer(); - stream.skip(buffer.remaining()); - buffers.add(buffer); - } - cis = CodedInputStream.newInstance(buffers); - } else if (size > 0 && size <= DEFAULT_MAX_MESSAGE_SIZE) { + if (size > 0 && size <= DEFAULT_MAX_MESSAGE_SIZE) { Reference ref; // buf should not be used after this method has returned. byte[] buf; @@ -239,6 +197,8 @@ public T parse(InputStream stream) { throw new RuntimeException("size inaccurate: " + size + " != " + position); } cis = CodedInputStream.newInstance(buf, 0, size); + } else if (size == 0) { + return defaultInstance; } } } catch (IOException e) { diff --git a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java index 801823f1b24..6ea836f96a7 100644 --- a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java +++ b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java @@ -22,7 +22,6 @@ import static org.junit.Assert.assertSame; import static org.junit.Assert.fail; -import com.google.common.base.Strings; import com.google.common.io.ByteStreams; import com.google.protobuf.ByteString; import com.google.protobuf.Empty; @@ -30,7 +29,6 @@ import com.google.protobuf.InvalidProtocolBufferException; import com.google.protobuf.Type; import io.grpc.Drainable; -import io.grpc.HasByteBuffer; import io.grpc.KnownLength; import io.grpc.Metadata; import io.grpc.MethodDescriptor.Marshaller; @@ -42,10 +40,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; -import java.nio.ByteBuffer; import java.util.Arrays; -import javax.annotation.Nullable; -import org.junit.Assume; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -232,20 +227,6 @@ public void parseFromKnowLengthInputStream() throws Exception { assertEquals(expect, result); } - @Test - public void parseFromKnownLengthByteBufferInputStream() { - Assume.assumeTrue(ProtoLiteUtils.IS_JAVA9_OR_HIGHER); - Marshaller marshaller = ProtoLiteUtils.marshaller(Type.getDefaultInstance()); - Type expect = - Type.newBuilder() - .setName(Strings.repeat("M", ProtoLiteUtils.MESSAGE_ZERO_COPY_THRESHOLD)) - .build(); - - Type result = marshaller.parse( - new CustomKnownLengthByteBufferInputStream(expect.toByteString().asReadOnlyByteBuffer())); - assertEquals(expect, result); - } - @Test public void defaultMaxMessageSize() { assertEquals(GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE, ProtoLiteUtils.DEFAULT_MAX_MESSAGE_SIZE); @@ -273,55 +254,4 @@ public int read() throws IOException { return source[position++]; } } - - private static final class CustomKnownLengthByteBufferInputStream extends InputStream - implements KnownLength, HasByteBuffer { - private ByteBuffer source; - - private CustomKnownLengthByteBufferInputStream(ByteBuffer source) { - this.source = source; - } - - @Override - public int available() throws IOException { - return source.remaining(); - } - - @Override - public synchronized void mark(int readlimit) { - source.mark(); - } - - @Override - public synchronized void reset() throws IOException { - source.reset(); - } - - @Override - public boolean markSupported() { - return true; - } - - @Override - public int read() throws IOException { - throw new UnsupportedOperationException("should not be called"); - } - - @Override - public long skip(long n) throws IOException { - source.position((int) (source.position() + n)); - return n; - } - - @Override - public boolean getByteBufferSupported() { - return true; - } - - @Nullable - @Override - public ByteBuffer getByteBuffer() { - return source.slice(); - } - } } From ad64a1d8edbe7574eb8491e05c4d75bfec91edb2 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 20 Apr 2021 02:05:28 -0700 Subject: [PATCH 32/46] Add Retainable interface that allows taking over resource ownership and prevent being closed too early. --- api/src/main/java/io/grpc/Retainable.java | 46 +++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 api/src/main/java/io/grpc/Retainable.java diff --git a/api/src/main/java/io/grpc/Retainable.java b/api/src/main/java/io/grpc/Retainable.java new file mode 100644 index 00000000000..b5e92964e0e --- /dev/null +++ b/api/src/main/java/io/grpc/Retainable.java @@ -0,0 +1,46 @@ +/* + * Copyright 2021 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc; + +/** + * A Retainable is a resource that can be retained, preventing it being closed too early. + * + *

A normal usage of this API is to take over the lifetime management of some resource, + * prevent it being closed or reclaimed by its original owner. This can be used to extend the + * lifetime of some resource not held by the current consumer. + */ +@ExperimentalApi("TODO") +public interface Retainable { + + /** + * Prevents the resource from being closed and reclaimed, until {@link #release} is called. + * After this call, the caller effectively becomes the owner of the resource and + * responsible for managing the lifetime. + * + *

Calling this method on an already retained or closed resource is a no-op. + */ + public void retain(); + + /** + * Releases the hold for preventing tbe underlying resource from being closed and reclaimed. + * Note calling this method does not automatically close the underlying resource, callers are + * still responsible for closing it. Otherwise, the underlying resource may be leaked. + * + *

Calling this method on an unretained or already reclaimed resource is a no-op. + */ + public void release(); +} From aa902aa13112278f9444eed34669d831901806eb Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 20 Apr 2021 02:06:15 -0700 Subject: [PATCH 33/46] Make BufferInputStream implements Retainable. Its close() method becomes a no-op if the underlying resource is retained. --- .../io/grpc/internal/ReadableBuffers.java | 17 +++++++++++++++- .../io/grpc/internal/ReadableBuffersTest.java | 20 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index f764d893fda..ad85a631a48 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -21,6 +21,7 @@ import com.google.common.base.Preconditions; import io.grpc.HasByteBuffer; import io.grpc.KnownLength; +import io.grpc.Retainable; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -330,8 +331,9 @@ public ByteBuffer getByteBuffer() { * An {@link InputStream} that is backed by a {@link ReadableBuffer}. */ private static final class BufferInputStream extends InputStream - implements KnownLength, HasByteBuffer { + implements KnownLength, HasByteBuffer, Retainable { final ReadableBuffer buffer; + private boolean retained; public BufferInputStream(ReadableBuffer buffer) { this.buffer = Preconditions.checkNotNull(buffer, "buffer"); @@ -396,8 +398,21 @@ public ByteBuffer getByteBuffer() { return buffer.getByteBuffer(); } + @Override + public void retain() { + retained = true; + } + + @Override + public void release() { + retained = false; + } + @Override public void close() throws IOException { + if (retained) { + return; + } buffer.close(); } } diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java index 3007c19682a..6b5e32f4180 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java @@ -27,6 +27,7 @@ import static org.mockito.Mockito.when; import io.grpc.HasByteBuffer; +import io.grpc.Retainable; import java.io.IOException; import java.io.InputStream; import org.junit.Test; @@ -158,4 +159,23 @@ public void bufferInputStream_getByteBufferDelegatesToBuffer() { ((HasByteBuffer) inputStream).getByteBuffer(); verify(buffer).getByteBuffer(); } + + @Test + public void bufferInputStream_retainThenClose_notCloseBuffer() throws IOException { + ReadableBuffer buffer = mock(ReadableBuffer.class); + InputStream inputStream = ReadableBuffers.openStream(buffer, true); + ((Retainable) inputStream).retain(); + inputStream.close(); + verify(buffer, never()).close(); + } + + @Test + public void bufferInputStream_retainReleaseThenClose_closeBuffer() throws IOException { + ReadableBuffer buffer = mock(ReadableBuffer.class); + InputStream inputStream = ReadableBuffers.openStream(buffer, true); + ((Retainable) inputStream).retain(); + ((Retainable) inputStream).release(); + inputStream.close(); + verify(buffer).close(); + } } From 6cf0739d37e9a8b1e318f618cbfcef3aed01967a Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 22 Apr 2021 12:20:50 -0700 Subject: [PATCH 34/46] Remove no longer needed constructors. --- .../main/java/io/grpc/internal/CompositeReadableBuffer.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index f3ad37dbc19..96e481df9e5 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -40,11 +40,6 @@ public class CompositeReadableBuffer extends AbstractReadableBuffer { private int readableBytes; private boolean marked; - public CompositeReadableBuffer(int initialCapacity) { - readableBuffers = new ArrayDeque<>(initialCapacity); - rewindableBuffers = new ArrayDeque<>(initialCapacity); - } - public CompositeReadableBuffer() { readableBuffers = new ArrayDeque<>(); rewindableBuffers = new ArrayDeque<>(); From bfe1d7d868cc12e4f5558e322e7e48d5fcae39e5 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 22 Apr 2021 12:32:09 -0700 Subject: [PATCH 35/46] Change return type to be more specific. --- .../src/main/java/io/grpc/internal/CompositeReadableBuffer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index 96e481df9e5..08e4f273fd3 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -167,7 +167,7 @@ public int read(ReadableBuffer buffer, int length, CompositeReadableBuffer dest, }; @Override - public ReadableBuffer readBytes(int length) { + public CompositeReadableBuffer readBytes(int length) { final CompositeReadableBuffer newBuffer = new CompositeReadableBuffer(); executeNoThrow(COMPOSITE_OP, length, newBuffer, 0); return newBuffer; From 4d908003ea9721494fa6f3d2efd7beb076b75a59 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 23 Apr 2021 19:11:31 -0700 Subject: [PATCH 36/46] Restore optimizations for avoid allocating new buffer wrappers. --- .../internal/CompositeReadableBuffer.java | 78 ++++++++++++++----- 1 file changed, 57 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index 08e4f273fd3..16b922548cb 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -40,6 +40,11 @@ public class CompositeReadableBuffer extends AbstractReadableBuffer { private int readableBytes; private boolean marked; + public CompositeReadableBuffer(int initialCapacity) { + readableBuffers = new ArrayDeque<>(initialCapacity); + rewindableBuffers = new ArrayDeque<>(initialCapacity); + } + public CompositeReadableBuffer() { readableBuffers = new ArrayDeque<>(); rewindableBuffers = new ArrayDeque<>(); @@ -156,20 +161,44 @@ public void readBytes(OutputStream dest, int length) throws IOException { execute(STREAM_OP, length, dest, 0); } - private static final NoThrowReadOperation COMPOSITE_OP = - new NoThrowReadOperation() { - @Override - public int read(ReadableBuffer buffer, int length, CompositeReadableBuffer dest, - int unused) { - dest.addBuffer(buffer.readBytes(length)); - return 0; - } - }; - @Override - public CompositeReadableBuffer readBytes(int length) { - final CompositeReadableBuffer newBuffer = new CompositeReadableBuffer(); - executeNoThrow(COMPOSITE_OP, length, newBuffer, 0); + public ReadableBuffer readBytes(int length) { + if (length <= 0) { + return ReadableBuffers.empty(); + } + checkReadable(length); + readableBytes -= length; + + ReadableBuffer newBuffer = null; + CompositeReadableBuffer newComposite = null; + do { + ReadableBuffer buffer = readableBuffers.peek(); + int readable = buffer.readableBytes(); + ReadableBuffer readBuffer; + if (readable > length) { + readBuffer = buffer.readBytes(length); + length = 0; + } else { + if (marked) { + readBuffer = buffer.readBytes(readable); + advanceBuffer(); + } else { + readBuffer = readableBuffers.poll(); + } + length -= readable; + } + if (newBuffer == null) { + newBuffer = readBuffer; + } else { + if (newComposite == null) { + newComposite = new CompositeReadableBuffer( + length == 0 ? 2 : Math.min(readableBuffers.size() + 2, 16)); + newComposite.addBuffer(newBuffer); + newBuffer = newComposite; + } + newComposite.addBuffer(readBuffer); + } + } while (length > 0); return newBuffer; } @@ -276,15 +305,22 @@ private int executeNoThrow(NoThrowReadOperation op, int length, T dest, i private void advanceBufferIfNecessary() { ReadableBuffer buffer = readableBuffers.peek(); if (buffer.readableBytes() == 0) { - if (marked) { - rewindableBuffers.add(readableBuffers.remove()); - ReadableBuffer next = readableBuffers.peek(); - if (next != null) { - next.mark(); - } - } else { - readableBuffers.remove().close(); + advanceBuffer(); + } + } + + /** + * Removes one buffer from the front and closes it. + */ + private void advanceBuffer() { + if (marked) { + rewindableBuffers.add(readableBuffers.remove()); + ReadableBuffer next = readableBuffers.peek(); + if (next != null) { + next.mark(); } + } else { + readableBuffers.remove().close(); } } From d8d030abd4aa12e24d0845b196e730235247561d Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 27 Apr 2021 13:17:03 -0700 Subject: [PATCH 37/46] Optimize by allocating rewindable buffer deque lazily. --- .../io/grpc/internal/CompositeReadableBuffer.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index 16b922548cb..6eac5867a08 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -36,18 +36,16 @@ public class CompositeReadableBuffer extends AbstractReadableBuffer { private final Deque readableBuffers; - private final Deque rewindableBuffers; + private Deque rewindableBuffers; private int readableBytes; private boolean marked; public CompositeReadableBuffer(int initialCapacity) { readableBuffers = new ArrayDeque<>(initialCapacity); - rewindableBuffers = new ArrayDeque<>(initialCapacity); } public CompositeReadableBuffer() { readableBuffers = new ArrayDeque<>(); - rewindableBuffers = new ArrayDeque<>(); } /** @@ -204,6 +202,9 @@ public ReadableBuffer readBytes(int length) { @Override public void mark() { + if (rewindableBuffers == null) { + rewindableBuffers = new ArrayDeque<>(Math.min(readableBuffers.size(), 16)); + } while (!rewindableBuffers.isEmpty()) { rewindableBuffers.remove().close(); } @@ -256,8 +257,10 @@ public void close() { while (!readableBuffers.isEmpty()) { readableBuffers.remove().close(); } - while (!rewindableBuffers.isEmpty()) { - rewindableBuffers.remove().close(); + if (rewindableBuffers != null) { + while (!rewindableBuffers.isEmpty()) { + rewindableBuffers.remove().close(); + } } } From f1f1c95cf0857a98bb8caa713bdc8a1221f024d7 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Wed, 5 May 2021 19:12:39 -0700 Subject: [PATCH 38/46] Change to Detachable API, which makes the original InputStream behaves like being exhausted after being detached. --- api/src/main/java/io/grpc/Detachable.java | 37 ++++++++ api/src/main/java/io/grpc/Retainable.java | 46 ---------- .../io/grpc/internal/ReadableBuffers.java | 43 ++++++--- .../io/grpc/internal/ReadableBuffersTest.java | 89 +++++++++++++++++-- 4 files changed, 146 insertions(+), 69 deletions(-) create mode 100644 api/src/main/java/io/grpc/Detachable.java delete mode 100644 api/src/main/java/io/grpc/Retainable.java diff --git a/api/src/main/java/io/grpc/Detachable.java b/api/src/main/java/io/grpc/Detachable.java new file mode 100644 index 00000000000..f10f99af2e7 --- /dev/null +++ b/api/src/main/java/io/grpc/Detachable.java @@ -0,0 +1,37 @@ +/* + * Copyright 2021 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc; + +/** + * A Detachable encapsulates an object that can be forked with underlying resources + * detached and transferred to a new instance. The forked instance takes over the ownership + * of resources and is responsible for releasing after use. The forked instance preserves + * states of detached resources. Resources can be consumed through the forked instance as if + * being continually consumed through the original instance. The original instance discards + * states of detached resources and is no longer consumable as if the resources are exhausted. + */ +@ExperimentalApi("TODO") +public interface Detachable { + + /** + * Fork a new instance of {@code T} with underlying resources detached from this instance and + * transferred to the new instance. + * + * @throws IllegalStateException if the underlying resources have already been detached. + */ + public T detach(); +} diff --git a/api/src/main/java/io/grpc/Retainable.java b/api/src/main/java/io/grpc/Retainable.java deleted file mode 100644 index b5e92964e0e..00000000000 --- a/api/src/main/java/io/grpc/Retainable.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Copyright 2021 The gRPC Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package io.grpc; - -/** - * A Retainable is a resource that can be retained, preventing it being closed too early. - * - *

A normal usage of this API is to take over the lifetime management of some resource, - * prevent it being closed or reclaimed by its original owner. This can be used to extend the - * lifetime of some resource not held by the current consumer. - */ -@ExperimentalApi("TODO") -public interface Retainable { - - /** - * Prevents the resource from being closed and reclaimed, until {@link #release} is called. - * After this call, the caller effectively becomes the owner of the resource and - * responsible for managing the lifetime. - * - *

Calling this method on an already retained or closed resource is a no-op. - */ - public void retain(); - - /** - * Releases the hold for preventing tbe underlying resource from being closed and reclaimed. - * Note calling this method does not automatically close the underlying resource, callers are - * still responsible for closing it. Otherwise, the underlying resource may be leaked. - * - *

Calling this method on an unretained or already reclaimed resource is a no-op. - */ - public void release(); -} diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index ad85a631a48..c9d05b6ea03 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -19,14 +19,15 @@ import static com.google.common.base.Charsets.UTF_8; import com.google.common.base.Preconditions; +import io.grpc.Detachable; import io.grpc.HasByteBuffer; import io.grpc.KnownLength; -import io.grpc.Retainable; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.nio.Buffer; import java.nio.ByteBuffer; +import java.nio.InvalidMarkException; import java.nio.charset.Charset; import javax.annotation.Nullable; @@ -216,6 +217,9 @@ public void mark() { @Override public void reset() { + if (mark == -1) { + throw new InvalidMarkException(); + } offset = mark; } } @@ -331,9 +335,9 @@ public ByteBuffer getByteBuffer() { * An {@link InputStream} that is backed by a {@link ReadableBuffer}. */ private static final class BufferInputStream extends InputStream - implements KnownLength, HasByteBuffer, Retainable { + implements KnownLength, HasByteBuffer, Detachable { final ReadableBuffer buffer; - private boolean retained; + private boolean detached; public BufferInputStream(ReadableBuffer buffer) { this.buffer = Preconditions.checkNotNull(buffer, "buffer"); @@ -341,12 +345,12 @@ public BufferInputStream(ReadableBuffer buffer) { @Override public int available() throws IOException { - return buffer.readableBytes(); + return detached ? 0 : buffer.readableBytes(); } @Override public int read() { - if (buffer.readableBytes() == 0) { + if (detached || buffer.readableBytes() == 0) { // EOF. return -1; } @@ -355,7 +359,7 @@ public int read() { @Override public int read(byte[] dest, int destOffset, int length) throws IOException { - if (buffer.readableBytes() == 0) { + if (detached || buffer.readableBytes() == 0) { // EOF. return -1; } @@ -367,6 +371,9 @@ public int read(byte[] dest, int destOffset, int length) throws IOException { @Override public long skip(long n) throws IOException { + if (detached) { + return 0; + } int length = (int) Math.min(buffer.readableBytes(), n); buffer.skipBytes(length); return length; @@ -374,11 +381,17 @@ public long skip(long n) throws IOException { @Override public void mark(int readlimit) { + if (detached) { + return; + } buffer.mark(); } @Override public void reset() throws IOException { + if (detached) { + throw new IOException("underlying buffer detached"); + } buffer.reset(); } @@ -395,22 +408,24 @@ public boolean getByteBufferSupported() { @Nullable @Override public ByteBuffer getByteBuffer() { + if (detached) { + return null; + } return buffer.getByteBuffer(); } @Override - public void retain() { - retained = true; - } - - @Override - public void release() { - retained = false; + public InputStream detach() { + if (detached) { + throw new IllegalStateException("already detached"); + } + detached = true; + return new BufferInputStream(buffer); } @Override public void close() throws IOException { - if (retained) { + if (detached) { return; } buffer.close(); diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java index 6b5e32f4180..e5d91d707b0 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java @@ -26,11 +26,13 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import io.grpc.Detachable; import io.grpc.HasByteBuffer; -import io.grpc.Retainable; import java.io.IOException; import java.io.InputStream; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -42,6 +44,9 @@ public class ReadableBuffersTest { private static final byte[] MSG_BYTES = "hello".getBytes(UTF_8); + @Rule + public final ExpectedException thrown = ExpectedException.none(); + @Test public void empty_returnsEmptyBuffer() { ReadableBuffer buffer = ReadableBuffers.empty(); @@ -160,22 +165,88 @@ public void bufferInputStream_getByteBufferDelegatesToBuffer() { verify(buffer).getByteBuffer(); } + @SuppressWarnings("unchecked") @Test - public void bufferInputStream_retainThenClose_notCloseBuffer() throws IOException { - ReadableBuffer buffer = mock(ReadableBuffer.class); + public void bufferInputStream_availableAfterDetached_returnsZeroByte() throws IOException { + ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); InputStream inputStream = ReadableBuffers.openStream(buffer, true); - ((Retainable) inputStream).retain(); - inputStream.close(); - verify(buffer, never()).close(); + assertEquals(5, inputStream.available()); + InputStream detachedStream = ((Detachable) inputStream).detach(); + assertEquals(0, inputStream.available()); + assertEquals(5, detachedStream.available()); + } + + @SuppressWarnings("unchecked") + @Test + public void bufferInputStream_skipAfterDetached() throws IOException { + ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); + InputStream inputStream = ReadableBuffers.openStream(buffer, true); + assertEquals(3, inputStream.skip(3)); + InputStream detachedStream = ((Detachable) inputStream).detach(); + assertEquals(0, inputStream.skip(2)); + assertEquals(2, detachedStream.skip(2)); + } + + @SuppressWarnings("unchecked") + @Test + public void bufferInputStream_readUnsignedByteAfterDetached() throws IOException { + ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); + InputStream inputStream = ReadableBuffers.openStream(buffer, true); + assertEquals((int) 'h', inputStream.read()); + InputStream detachedStream = ((Detachable) inputStream).detach(); + assertEquals(-1, inputStream.read()); + assertEquals((int) 'e', detachedStream.read()); } + @SuppressWarnings("unchecked") @Test - public void bufferInputStream_retainReleaseThenClose_closeBuffer() throws IOException { + public void bufferInputStream_partialReadAfterDetached() throws IOException { + ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); + InputStream inputStream = ReadableBuffers.openStream(buffer, true); + byte[] dest = new byte[3]; + assertEquals(3, inputStream.read(dest, /*destOffset*/ 0, /*length*/ 3)); + assertArrayEquals(new byte[]{'h', 'e', 'l'}, dest); + InputStream detachedStream = ((Detachable) inputStream).detach(); + byte[] newDest = new byte[2]; + assertEquals(2, detachedStream.read(newDest, /*destOffset*/ 0, /*length*/ 2)); + assertArrayEquals(new byte[]{'l', 'o'}, newDest); + } + + @SuppressWarnings("unchecked") + @Test + public void bufferInputStream_markDiscardedAfterDetached() throws IOException { + ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); + InputStream inputStream = ReadableBuffers.openStream(buffer, true); + inputStream.mark(5); + ((Detachable) inputStream).detach(); + thrown.expect(IOException.class); + thrown.expectMessage("underlying buffer detached"); + inputStream.reset(); + } + + @SuppressWarnings("unchecked") + @Test + public void bufferInputStream_markPreservedInForkedInputStream() throws IOException { + ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); + InputStream inputStream = ReadableBuffers.openStream(buffer, true); + inputStream.skip(2); + inputStream.mark(3); + InputStream detachedStream = ((Detachable) inputStream).detach(); + detachedStream.skip(3); + assertEquals(0, detachedStream.available()); + detachedStream.reset(); + assertEquals(3, detachedStream.available()); + } + + @SuppressWarnings("unchecked") + @Test + public void bufferInputStream_closeAfterDetached() throws IOException { ReadableBuffer buffer = mock(ReadableBuffer.class); InputStream inputStream = ReadableBuffers.openStream(buffer, true); - ((Retainable) inputStream).retain(); - ((Retainable) inputStream).release(); + InputStream detachedStream = ((Detachable) inputStream).detach(); inputStream.close(); + verify(buffer, never()).close(); + detachedStream.close(); verify(buffer).close(); } } From a2f3a9e74a5aa79e04d39b376520a6d0640b4e3a Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Wed, 5 May 2021 19:15:54 -0700 Subject: [PATCH 39/46] Change naming for getByteBufferSupported(). --- api/src/main/java/io/grpc/HasByteBuffer.java | 4 ++-- .../io/grpc/internal/AbstractReadableBuffer.java | 2 +- .../io/grpc/internal/CompositeReadableBuffer.java | 4 ++-- .../io/grpc/internal/ForwardingReadableBuffer.java | 4 ++-- .../main/java/io/grpc/internal/ReadableBuffer.java | 4 ++-- .../main/java/io/grpc/internal/ReadableBuffers.java | 6 +++--- .../grpc/internal/CompositeReadableBufferTest.java | 12 ++++++------ .../io/grpc/internal/ReadableBufferTestBase.java | 4 ++-- .../java/io/grpc/internal/ReadableBuffersTest.java | 4 ++-- .../main/java/io/grpc/netty/NettyReadableBuffer.java | 2 +- .../java/io/grpc/netty/NettyReadableBufferTest.java | 2 +- 11 files changed, 24 insertions(+), 24 deletions(-) diff --git a/api/src/main/java/io/grpc/HasByteBuffer.java b/api/src/main/java/io/grpc/HasByteBuffer.java index 35abca53e6c..97f2435524a 100644 --- a/api/src/main/java/io/grpc/HasByteBuffer.java +++ b/api/src/main/java/io/grpc/HasByteBuffer.java @@ -34,7 +34,7 @@ public interface HasByteBuffer { /** * Indicates whether or not {@link #getByteBuffer} operation is supported. */ - boolean getByteBufferSupported(); + boolean byteBufferSupported(); /** * Gets a {@link ByteBuffer} containing some bytes of the content next to be read, or {@code @@ -43,7 +43,7 @@ public interface HasByteBuffer { * stream. The returned buffer's content should not be modified, but the position, limit, and * mark may be changed. Operations for changing the position, limit, and mark of the returned * buffer does not affect the position, limit, and mark of this input stream. This is an optional - * method, so callers should first check {@link #getByteBufferSupported}. + * method, so callers should first check {@link #byteBufferSupported}. * * @throws UnsupportedOperationException if this operation is not supported. */ diff --git a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java index 99812c3ffff..d8bbb8ed73a 100644 --- a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java @@ -56,7 +56,7 @@ public void reset() { } @Override - public boolean getByteBufferSupported() { + public boolean byteBufferSupported() { return false; } diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index 6eac5867a08..cd3429c9f32 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -234,9 +234,9 @@ public void reset() { } @Override - public boolean getByteBufferSupported() { + public boolean byteBufferSupported() { for (ReadableBuffer buffer : readableBuffers) { - if (!buffer.getByteBufferSupported()) { + if (!buffer.byteBufferSupported()) { return false; } } diff --git a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java index 0103f9fb39f..a85afa92477 100644 --- a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java @@ -108,8 +108,8 @@ public void reset() { } @Override - public boolean getByteBufferSupported() { - return buf.getByteBufferSupported(); + public boolean byteBufferSupported() { + return buf.byteBufferSupported(); } @Nullable diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffer.java b/core/src/main/java/io/grpc/internal/ReadableBuffer.java index 684039292aa..80fd27dc7d5 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffer.java @@ -140,7 +140,7 @@ public interface ReadableBuffer extends Closeable { /** * Indicates whether or not {@link #getByteBuffer} operation is supported for this buffer. */ - boolean getByteBufferSupported(); + boolean byteBufferSupported(); /** * Gets a {@link ByteBuffer} that contains some bytes of the content next to be read, or {@code @@ -150,7 +150,7 @@ public interface ReadableBuffer extends Closeable { * mark may be changed. Operations for changing the position, limit, and mark of the returned * buffer does not affect the position, limit, and mark of this buffer. Buffers returned by this * method have independent position, limit and mark. This is an optional method, so callers - * should first check {@link #getByteBufferSupported}. + * should first check {@link #byteBufferSupported}. * * @throws UnsupportedOperationException the buffer does not support this method. */ diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index c9d05b6ea03..91966cb7b7d 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -321,7 +321,7 @@ public void reset() { } @Override - public boolean getByteBufferSupported() { + public boolean byteBufferSupported() { return true; } @@ -401,8 +401,8 @@ public boolean markSupported() { } @Override - public boolean getByteBufferSupported() { - return buffer.getByteBufferSupported(); + public boolean byteBufferSupported() { + return buffer.byteBufferSupported(); } @Nullable diff --git a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java index f26c8b1ef9a..b11d872415d 100644 --- a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java +++ b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java @@ -262,15 +262,15 @@ public void canUseByteBufferOnlyAllComponentsSupportUsingByteBuffer() { ReadableBuffer buffer1 = mock(ReadableBuffer.class); ReadableBuffer buffer2 = mock(ReadableBuffer.class); ReadableBuffer buffer3 = mock(ReadableBuffer.class); - when(buffer1.getByteBufferSupported()).thenReturn(true); - when(buffer2.getByteBufferSupported()).thenReturn(true); - when(buffer3.getByteBufferSupported()).thenReturn(false); + when(buffer1.byteBufferSupported()).thenReturn(true); + when(buffer2.byteBufferSupported()).thenReturn(true); + when(buffer3.byteBufferSupported()).thenReturn(false); composite.addBuffer(buffer1); - assertTrue(composite.getByteBufferSupported()); + assertTrue(composite.byteBufferSupported()); composite.addBuffer(buffer2); - assertTrue(composite.getByteBufferSupported()); + assertTrue(composite.byteBufferSupported()); composite.addBuffer(buffer3); - assertFalse(composite.getByteBufferSupported()); + assertFalse(composite.byteBufferSupported()); } @Test diff --git a/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java b/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java index 86b8e2a399b..97e0df38ae7 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java +++ b/core/src/test/java/io/grpc/internal/ReadableBufferTestBase.java @@ -153,7 +153,7 @@ public void markAndResetWithReadToReadableBufferShouldSucceed() { @Test public void getByteBufferDoesNotAffectBufferPosition() { ReadableBuffer buffer = buffer(); - Assume.assumeTrue(buffer.getByteBufferSupported()); + Assume.assumeTrue(buffer.byteBufferSupported()); ByteBuffer byteBuffer = buffer.getByteBuffer(); assertEquals(msg.length(), buffer.readableBytes()); byteBuffer.get(new byte[byteBuffer.remaining()]); @@ -163,7 +163,7 @@ public void getByteBufferDoesNotAffectBufferPosition() { @Test public void getByteBufferIsNotAffectedByBufferRead() { ReadableBuffer buffer = buffer(); - Assume.assumeTrue(buffer.getByteBufferSupported()); + Assume.assumeTrue(buffer.byteBufferSupported()); ByteBuffer byteBuffer = buffer.getByteBuffer(); int initialRemaining = byteBuffer.remaining(); buffer.readBytes(new byte[100], 0, 100); diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java index e5d91d707b0..db17979bc2c 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java @@ -158,9 +158,9 @@ public void bufferInputStream_markAndReset() throws IOException { @Test public void bufferInputStream_getByteBufferDelegatesToBuffer() { ReadableBuffer buffer = mock(ReadableBuffer.class); - when(buffer.getByteBufferSupported()).thenReturn(true); + when(buffer.byteBufferSupported()).thenReturn(true); InputStream inputStream = ReadableBuffers.openStream(buffer, true); - assertTrue(((HasByteBuffer) inputStream).getByteBufferSupported()); + assertTrue(((HasByteBuffer) inputStream).byteBufferSupported()); ((HasByteBuffer) inputStream).getByteBuffer(); verify(buffer).getByteBuffer(); } diff --git a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java index 75d70978610..92ca79524ad 100644 --- a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java +++ b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java @@ -105,7 +105,7 @@ public void reset() { } @Override - public boolean getByteBufferSupported() { + public boolean byteBufferSupported() { return buffer.nioBufferCount() > 0; } diff --git a/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java b/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java index 23c2f0632a9..1a0ac229a89 100644 --- a/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyReadableBufferTest.java @@ -59,7 +59,7 @@ public void closeMultipleTimesShouldReleaseBufferOnce() { @Test public void getByteBufferFromSingleNioBufferBackedBuffer() { - assertTrue(buffer.getByteBufferSupported()); + assertTrue(buffer.byteBufferSupported()); ByteBuffer byteBuffer = buffer.getByteBuffer(); byte[] arr = new byte[byteBuffer.remaining()]; byteBuffer.get(arr); From 61c344752c68d3f67720bf7e30e7106b44a22b88 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 6 May 2021 02:07:46 -0700 Subject: [PATCH 40/46] Eliminate generics, use Object and casts instead. --- api/src/main/java/io/grpc/Detachable.java | 6 +++--- .../io/grpc/internal/ReadableBuffers.java | 4 ++-- .../io/grpc/internal/ReadableBuffersTest.java | 21 +++++++------------ 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/api/src/main/java/io/grpc/Detachable.java b/api/src/main/java/io/grpc/Detachable.java index f10f99af2e7..5ba8b4a592d 100644 --- a/api/src/main/java/io/grpc/Detachable.java +++ b/api/src/main/java/io/grpc/Detachable.java @@ -25,13 +25,13 @@ * states of detached resources and is no longer consumable as if the resources are exhausted. */ @ExperimentalApi("TODO") -public interface Detachable { +public interface Detachable { /** - * Fork a new instance of {@code T} with underlying resources detached from this instance and + * Fork a new instance with underlying resources detached from this instance and * transferred to the new instance. * * @throws IllegalStateException if the underlying resources have already been detached. */ - public T detach(); + public Object detach(); } diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index 91966cb7b7d..09b9e8778e8 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -335,7 +335,7 @@ public ByteBuffer getByteBuffer() { * An {@link InputStream} that is backed by a {@link ReadableBuffer}. */ private static final class BufferInputStream extends InputStream - implements KnownLength, HasByteBuffer, Detachable { + implements KnownLength, HasByteBuffer, Detachable { final ReadableBuffer buffer; private boolean detached; @@ -415,7 +415,7 @@ public ByteBuffer getByteBuffer() { } @Override - public InputStream detach() { + public Object detach() { if (detached) { throw new IllegalStateException("already detached"); } diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java index db17979bc2c..7a1398c4d91 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java @@ -165,40 +165,36 @@ public void bufferInputStream_getByteBufferDelegatesToBuffer() { verify(buffer).getByteBuffer(); } - @SuppressWarnings("unchecked") @Test public void bufferInputStream_availableAfterDetached_returnsZeroByte() throws IOException { ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); InputStream inputStream = ReadableBuffers.openStream(buffer, true); assertEquals(5, inputStream.available()); - InputStream detachedStream = ((Detachable) inputStream).detach(); + InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); assertEquals(0, inputStream.available()); assertEquals(5, detachedStream.available()); } - @SuppressWarnings("unchecked") @Test public void bufferInputStream_skipAfterDetached() throws IOException { ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); InputStream inputStream = ReadableBuffers.openStream(buffer, true); assertEquals(3, inputStream.skip(3)); - InputStream detachedStream = ((Detachable) inputStream).detach(); + InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); assertEquals(0, inputStream.skip(2)); assertEquals(2, detachedStream.skip(2)); } - @SuppressWarnings("unchecked") @Test public void bufferInputStream_readUnsignedByteAfterDetached() throws IOException { ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); InputStream inputStream = ReadableBuffers.openStream(buffer, true); assertEquals((int) 'h', inputStream.read()); - InputStream detachedStream = ((Detachable) inputStream).detach(); + InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); assertEquals(-1, inputStream.read()); assertEquals((int) 'e', detachedStream.read()); } - @SuppressWarnings("unchecked") @Test public void bufferInputStream_partialReadAfterDetached() throws IOException { ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); @@ -206,44 +202,41 @@ public void bufferInputStream_partialReadAfterDetached() throws IOException { byte[] dest = new byte[3]; assertEquals(3, inputStream.read(dest, /*destOffset*/ 0, /*length*/ 3)); assertArrayEquals(new byte[]{'h', 'e', 'l'}, dest); - InputStream detachedStream = ((Detachable) inputStream).detach(); + InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); byte[] newDest = new byte[2]; assertEquals(2, detachedStream.read(newDest, /*destOffset*/ 0, /*length*/ 2)); assertArrayEquals(new byte[]{'l', 'o'}, newDest); } - @SuppressWarnings("unchecked") @Test public void bufferInputStream_markDiscardedAfterDetached() throws IOException { ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); InputStream inputStream = ReadableBuffers.openStream(buffer, true); inputStream.mark(5); - ((Detachable) inputStream).detach(); + ((Detachable) inputStream).detach(); thrown.expect(IOException.class); thrown.expectMessage("underlying buffer detached"); inputStream.reset(); } - @SuppressWarnings("unchecked") @Test public void bufferInputStream_markPreservedInForkedInputStream() throws IOException { ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); InputStream inputStream = ReadableBuffers.openStream(buffer, true); inputStream.skip(2); inputStream.mark(3); - InputStream detachedStream = ((Detachable) inputStream).detach(); + InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); detachedStream.skip(3); assertEquals(0, detachedStream.available()); detachedStream.reset(); assertEquals(3, detachedStream.available()); } - @SuppressWarnings("unchecked") @Test public void bufferInputStream_closeAfterDetached() throws IOException { ReadableBuffer buffer = mock(ReadableBuffer.class); InputStream inputStream = ReadableBuffers.openStream(buffer, true); - InputStream detachedStream = ((Detachable) inputStream).detach(); + InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); inputStream.close(); verify(buffer, never()).close(); detachedStream.close(); From 15a0314e6e0db13a97efc0f586197950441db447 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 6 May 2021 11:08:35 -0700 Subject: [PATCH 41/46] Return Detachable instead of Object. --- api/src/main/java/io/grpc/Detachable.java | 2 +- core/src/main/java/io/grpc/internal/ReadableBuffers.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/io/grpc/Detachable.java b/api/src/main/java/io/grpc/Detachable.java index 5ba8b4a592d..f97ca6f250b 100644 --- a/api/src/main/java/io/grpc/Detachable.java +++ b/api/src/main/java/io/grpc/Detachable.java @@ -33,5 +33,5 @@ public interface Detachable { * * @throws IllegalStateException if the underlying resources have already been detached. */ - public Object detach(); + public Detachable detach(); } diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index 09b9e8778e8..f308296a37c 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -415,7 +415,7 @@ public ByteBuffer getByteBuffer() { } @Override - public Object detach() { + public Detachable detach() { if (detached) { throw new IllegalStateException("already detached"); } From f8afce5968165c33ad1e51a6b95c2a5e8a665452 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 6 May 2021 12:40:57 -0700 Subject: [PATCH 42/46] Hook BufferInputStream's markSupported() with underlying buffers. --- .../grpc/internal/AbstractReadableBuffer.java | 5 +++++ .../grpc/internal/CompositeReadableBuffer.java | 10 ++++++++++ .../grpc/internal/ForwardingReadableBuffer.java | 5 +++++ .../java/io/grpc/internal/ReadableBuffer.java | 5 +++++ .../java/io/grpc/internal/ReadableBuffers.java | 12 +++++++++++- .../internal/CompositeReadableBufferTest.java | 17 +++++++++++++++++ .../java/io/grpc/netty/NettyReadableBuffer.java | 5 +++++ 7 files changed, 58 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java index d8bbb8ed73a..16c046dfc36 100644 --- a/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/AbstractReadableBuffer.java @@ -47,6 +47,11 @@ public int arrayOffset() { throw new UnsupportedOperationException(); } + @Override + public boolean markSupported() { + return false; + } + @Override public void mark() {} diff --git a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java index cd3429c9f32..9baec34b189 100644 --- a/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java @@ -200,6 +200,16 @@ public ReadableBuffer readBytes(int length) { return newBuffer; } + @Override + public boolean markSupported() { + for (ReadableBuffer buffer : readableBuffers) { + if (!buffer.markSupported()) { + return false; + } + } + return true; + } + @Override public void mark() { if (rewindableBuffers == null) { diff --git a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java index a85afa92477..1d7b412e195 100644 --- a/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ForwardingReadableBuffer.java @@ -97,6 +97,11 @@ public int arrayOffset() { return buf.arrayOffset(); } + @Override + public boolean markSupported() { + return buf.markSupported(); + } + @Override public void mark() { buf.mark(); diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffer.java b/core/src/main/java/io/grpc/internal/ReadableBuffer.java index 80fd27dc7d5..b47501a9943 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffer.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffer.java @@ -124,6 +124,11 @@ public interface ReadableBuffer extends Closeable { */ int arrayOffset(); + /** + * Indicates whether or not {@link #mark} operation is supported for this buffer. + */ + boolean markSupported(); + /** * Marks the current position in this buffer. A subsequent call to the {@link #reset} method * repositions this stream at the last marked position so that subsequent reads re-read the same diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index f308296a37c..371871c211b 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -210,6 +210,11 @@ public int arrayOffset() { return offset; } + @Override + public boolean markSupported() { + return true; + } + @Override public void mark() { mark = offset; @@ -310,6 +315,11 @@ public int arrayOffset() { return bytes.arrayOffset() + bytes.position(); } + @Override + public boolean markSupported() { + return true; + } + @Override public void mark() { bytes.mark(); @@ -397,7 +407,7 @@ public void reset() throws IOException { @Override public boolean markSupported() { - return true; + return buffer.markSupported(); } @Override diff --git a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java index b11d872415d..011d83b548a 100644 --- a/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java +++ b/core/src/test/java/io/grpc/internal/CompositeReadableBufferTest.java @@ -160,6 +160,23 @@ public void readStreamShouldSucceed() throws IOException { assertEquals(EXPECTED_VALUE, new String(bos.toByteArray(), UTF_8)); } + @Test + public void markSupportedOnlyAllComponentsSupportMark() { + composite = new CompositeReadableBuffer(); + ReadableBuffer buffer1 = mock(ReadableBuffer.class); + ReadableBuffer buffer2 = mock(ReadableBuffer.class); + ReadableBuffer buffer3 = mock(ReadableBuffer.class); + when(buffer1.markSupported()).thenReturn(true); + when(buffer2.markSupported()).thenReturn(true); + when(buffer3.markSupported()).thenReturn(false); + composite.addBuffer(buffer1); + assertTrue(composite.markSupported()); + composite.addBuffer(buffer2); + assertTrue(composite.markSupported()); + composite.addBuffer(buffer3); + assertFalse(composite.markSupported()); + } + @Test public void resetUnmarkedShouldThrow() { try { diff --git a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java index 92ca79524ad..cce58f1e60d 100644 --- a/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java +++ b/netty/src/main/java/io/grpc/netty/NettyReadableBuffer.java @@ -94,6 +94,11 @@ public int arrayOffset() { return buffer.arrayOffset() + buffer.readerIndex(); } + @Override + public boolean markSupported() { + return true; + } + @Override public void mark() { buffer.markReaderIndex(); From 2a6cbb6d7f7dd06b8f30cc0929430ac3643656c2 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 11 May 2021 19:07:58 -0700 Subject: [PATCH 43/46] Update Detachable interface definition, make it more specific to InputStream. --- api/src/main/java/io/grpc/Detachable.java | 23 +++++++++++-------- .../io/grpc/internal/ReadableBuffers.java | 2 +- .../io/grpc/internal/ReadableBuffersTest.java | 12 +++++----- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/api/src/main/java/io/grpc/Detachable.java b/api/src/main/java/io/grpc/Detachable.java index f97ca6f250b..3ddcd65e053 100644 --- a/api/src/main/java/io/grpc/Detachable.java +++ b/api/src/main/java/io/grpc/Detachable.java @@ -16,22 +16,25 @@ package io.grpc; +import java.io.InputStream; + /** - * A Detachable encapsulates an object that can be forked with underlying resources - * detached and transferred to a new instance. The forked instance takes over the ownership - * of resources and is responsible for releasing after use. The forked instance preserves - * states of detached resources. Resources can be consumed through the forked instance as if - * being continually consumed through the original instance. The original instance discards - * states of detached resources and is no longer consumable as if the resources are exhausted. + * A Detachable encapsulates some readable data source that can be detached and transferred + * to an {@link InputStream}. The detached InputStream takes over the ownership of the + * underlying data source. That's said, the detached InputStream is responsible for releasing its + * resources after use. The detached InputStream preserves internal states of the underlying + * data source. Data can be consumed through the detached InputStream as if being continually + * consumed through the original instance. The original instance discards internal states of + * detached data source and is no longer consumable as if the data source is exhausted. */ @ExperimentalApi("TODO") public interface Detachable { /** - * Fork a new instance with underlying resources detached from this instance and - * transferred to the new instance. + * Detaches the underlying data source from this instance and transfers to an {@link + * InputStream}. Detaching data from an already-detached instance gives an InputStream with + * zero bytes of data. * - * @throws IllegalStateException if the underlying resources have already been detached. */ - public Detachable detach(); + InputStream detach(); } diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index 371871c211b..75e8642026f 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -425,7 +425,7 @@ public ByteBuffer getByteBuffer() { } @Override - public Detachable detach() { + public InputStream detach() { if (detached) { throw new IllegalStateException("already detached"); } diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java index 7a1398c4d91..0ad802a0ab6 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java @@ -170,7 +170,7 @@ public void bufferInputStream_availableAfterDetached_returnsZeroByte() throws IO ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); InputStream inputStream = ReadableBuffers.openStream(buffer, true); assertEquals(5, inputStream.available()); - InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); + InputStream detachedStream = ((Detachable) inputStream).detach(); assertEquals(0, inputStream.available()); assertEquals(5, detachedStream.available()); } @@ -180,7 +180,7 @@ public void bufferInputStream_skipAfterDetached() throws IOException { ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); InputStream inputStream = ReadableBuffers.openStream(buffer, true); assertEquals(3, inputStream.skip(3)); - InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); + InputStream detachedStream = ((Detachable) inputStream).detach(); assertEquals(0, inputStream.skip(2)); assertEquals(2, detachedStream.skip(2)); } @@ -190,7 +190,7 @@ public void bufferInputStream_readUnsignedByteAfterDetached() throws IOException ReadableBuffer buffer = ReadableBuffers.wrap(MSG_BYTES); InputStream inputStream = ReadableBuffers.openStream(buffer, true); assertEquals((int) 'h', inputStream.read()); - InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); + InputStream detachedStream = ((Detachable) inputStream).detach(); assertEquals(-1, inputStream.read()); assertEquals((int) 'e', detachedStream.read()); } @@ -202,7 +202,7 @@ public void bufferInputStream_partialReadAfterDetached() throws IOException { byte[] dest = new byte[3]; assertEquals(3, inputStream.read(dest, /*destOffset*/ 0, /*length*/ 3)); assertArrayEquals(new byte[]{'h', 'e', 'l'}, dest); - InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); + InputStream detachedStream = ((Detachable) inputStream).detach(); byte[] newDest = new byte[2]; assertEquals(2, detachedStream.read(newDest, /*destOffset*/ 0, /*length*/ 2)); assertArrayEquals(new byte[]{'l', 'o'}, newDest); @@ -225,7 +225,7 @@ public void bufferInputStream_markPreservedInForkedInputStream() throws IOExcept InputStream inputStream = ReadableBuffers.openStream(buffer, true); inputStream.skip(2); inputStream.mark(3); - InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); + InputStream detachedStream = ((Detachable) inputStream).detach(); detachedStream.skip(3); assertEquals(0, detachedStream.available()); detachedStream.reset(); @@ -236,7 +236,7 @@ public void bufferInputStream_markPreservedInForkedInputStream() throws IOExcept public void bufferInputStream_closeAfterDetached() throws IOException { ReadableBuffer buffer = mock(ReadableBuffer.class); InputStream inputStream = ReadableBuffers.openStream(buffer, true); - InputStream detachedStream = (InputStream) ((Detachable) inputStream).detach(); + InputStream detachedStream = ((Detachable) inputStream).detach(); inputStream.close(); verify(buffer, never()).close(); detachedStream.close(); From e534f4a48b77d49510404f6c91186cba9acc9938 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Wed, 12 May 2021 00:50:56 -0700 Subject: [PATCH 44/46] Replace the internal buffer of BufferInputStream with an empty buffer after being detached. Further operations on the detached BufferInputStream just delegate to the empty buffer. --- .../io/grpc/internal/ReadableBuffers.java | 32 ++++--------------- .../io/grpc/internal/ReadableBuffersTest.java | 6 ++-- 2 files changed, 11 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ReadableBuffers.java b/core/src/main/java/io/grpc/internal/ReadableBuffers.java index 75e8642026f..c54cb0e67d0 100644 --- a/core/src/main/java/io/grpc/internal/ReadableBuffers.java +++ b/core/src/main/java/io/grpc/internal/ReadableBuffers.java @@ -346,8 +346,7 @@ public ByteBuffer getByteBuffer() { */ private static final class BufferInputStream extends InputStream implements KnownLength, HasByteBuffer, Detachable { - final ReadableBuffer buffer; - private boolean detached; + private ReadableBuffer buffer; public BufferInputStream(ReadableBuffer buffer) { this.buffer = Preconditions.checkNotNull(buffer, "buffer"); @@ -355,12 +354,12 @@ public BufferInputStream(ReadableBuffer buffer) { @Override public int available() throws IOException { - return detached ? 0 : buffer.readableBytes(); + return buffer.readableBytes(); } @Override public int read() { - if (detached || buffer.readableBytes() == 0) { + if (buffer.readableBytes() == 0) { // EOF. return -1; } @@ -369,7 +368,7 @@ public int read() { @Override public int read(byte[] dest, int destOffset, int length) throws IOException { - if (detached || buffer.readableBytes() == 0) { + if (buffer.readableBytes() == 0) { // EOF. return -1; } @@ -381,9 +380,6 @@ public int read(byte[] dest, int destOffset, int length) throws IOException { @Override public long skip(long n) throws IOException { - if (detached) { - return 0; - } int length = (int) Math.min(buffer.readableBytes(), n); buffer.skipBytes(length); return length; @@ -391,17 +387,11 @@ public long skip(long n) throws IOException { @Override public void mark(int readlimit) { - if (detached) { - return; - } buffer.mark(); } @Override public void reset() throws IOException { - if (detached) { - throw new IOException("underlying buffer detached"); - } buffer.reset(); } @@ -418,26 +408,18 @@ public boolean byteBufferSupported() { @Nullable @Override public ByteBuffer getByteBuffer() { - if (detached) { - return null; - } return buffer.getByteBuffer(); } @Override public InputStream detach() { - if (detached) { - throw new IllegalStateException("already detached"); - } - detached = true; - return new BufferInputStream(buffer); + ReadableBuffer detachedBuffer = buffer; + buffer = buffer.readBytes(0); + return new BufferInputStream(detachedBuffer); } @Override public void close() throws IOException { - if (detached) { - return; - } buffer.close(); } } diff --git a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java index 0ad802a0ab6..0947f65da12 100644 --- a/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java +++ b/core/src/test/java/io/grpc/internal/ReadableBuffersTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -30,6 +31,7 @@ import io.grpc.HasByteBuffer; import java.io.IOException; import java.io.InputStream; +import java.nio.InvalidMarkException; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -214,8 +216,7 @@ public void bufferInputStream_markDiscardedAfterDetached() throws IOException { InputStream inputStream = ReadableBuffers.openStream(buffer, true); inputStream.mark(5); ((Detachable) inputStream).detach(); - thrown.expect(IOException.class); - thrown.expectMessage("underlying buffer detached"); + thrown.expect(InvalidMarkException.class); inputStream.reset(); } @@ -235,6 +236,7 @@ public void bufferInputStream_markPreservedInForkedInputStream() throws IOExcept @Test public void bufferInputStream_closeAfterDetached() throws IOException { ReadableBuffer buffer = mock(ReadableBuffer.class); + when(buffer.readBytes(anyInt())).thenReturn(mock(ReadableBuffer.class)); InputStream inputStream = ReadableBuffers.openStream(buffer, true); InputStream detachedStream = ((Detachable) inputStream).detach(); inputStream.close(); From ad8940fe590547c27e41a2164826fec735aef6ed Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 14 May 2021 01:58:00 -0700 Subject: [PATCH 45/46] Add ExperimentalApi link. --- api/src/main/java/io/grpc/Detachable.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/io/grpc/Detachable.java b/api/src/main/java/io/grpc/Detachable.java index 3ddcd65e053..67f18ab9077 100644 --- a/api/src/main/java/io/grpc/Detachable.java +++ b/api/src/main/java/io/grpc/Detachable.java @@ -27,7 +27,7 @@ * consumed through the original instance. The original instance discards internal states of * detached data source and is no longer consumable as if the data source is exhausted. */ -@ExperimentalApi("TODO") +@ExperimentalApi("https://github.com/grpc/grpc-java/issues/7387") public interface Detachable { /** From a6c1b132589de4085b5ef182515b865539811a40 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 14 May 2021 12:42:07 -0700 Subject: [PATCH 46/46] Update Javadoc for Detachable. --- api/src/main/java/io/grpc/Detachable.java | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/io/grpc/Detachable.java b/api/src/main/java/io/grpc/Detachable.java index 67f18ab9077..c0cbf016f5b 100644 --- a/api/src/main/java/io/grpc/Detachable.java +++ b/api/src/main/java/io/grpc/Detachable.java @@ -19,13 +19,18 @@ import java.io.InputStream; /** - * A Detachable encapsulates some readable data source that can be detached and transferred - * to an {@link InputStream}. The detached InputStream takes over the ownership of the - * underlying data source. That's said, the detached InputStream is responsible for releasing its - * resources after use. The detached InputStream preserves internal states of the underlying - * data source. Data can be consumed through the detached InputStream as if being continually - * consumed through the original instance. The original instance discards internal states of - * detached data source and is no longer consumable as if the data source is exhausted. + * An extension of {@link InputStream} that allows the underlying data source to be detached and + * transferred to a new instance of the same kind. The detached InputStream takes over the + * ownership of the underlying data source. That's said, the detached InputStream is responsible + * for releasing its resources after use. The detached InputStream preserves internal states of + * the underlying data source. Data can be consumed through the detached InputStream as if being + * continually consumed through the original instance. The original instance discards internal + * states of detached data source and is no longer consumable as if the data source is exhausted. + * + *

A normal usage of this API is to extend the lifetime of the data source owned by the + * original instance for doing extra processing before releasing it. For example, when combined + * with {@link HasByteBuffer}, a custom {@link io.grpc.MethodDescriptor.Marshaller} can take + * over the ownership of buffers containing inbound data and perform delayed deserialization. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/7387") public interface Detachable {