From 061d8acb4d5e68f30a6d1a499fef22c7c79ae7f4 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 14 Dec 2023 10:20:58 +1100 Subject: [PATCH 01/43] Review ByteBufferAccumulator #10541 + Added `isRetained()` to `Retainable` + Made `Chunk` is-a `RetainableByteBuffer` + retain rather than copy chunks where possible --- .../eclipse/jetty/http2/HTTP2Connection.java | 6 +++++ .../org/eclipse/jetty/http2/api/Stream.java | 2 +- .../jetty/http3/HTTP3StreamConnection.java | 6 +++++ .../org/eclipse/jetty/http3/api/Stream.java | 2 +- .../jetty/io/ByteBufferAccumulator.java | 9 ++++++++ .../java/org/eclipse/jetty/io/Content.java | 23 +------------------ .../java/org/eclipse/jetty/io/Retainable.java | 21 +++++++++++++---- .../jetty/io/RetainableByteBuffer.java | 6 ----- .../jetty/io/content/AsyncContent.java | 6 +++++ .../jetty/io/internal/ByteBufferChunk.java | 12 ++++++++++ .../io/internal/ContentSourceByteBuffer.java | 2 +- 11 files changed, 59 insertions(+), 36 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java index 20e6b36350c1..5debc18cf19e 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java @@ -478,6 +478,12 @@ public boolean canRetain() return retainable.canRetain(); } + @Override + public boolean isRetained() + { + return retainable.isRetained(); + } + @Override public void retain() { diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java index dc3ecf7d5275..ccc5892f49a2 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java @@ -438,7 +438,7 @@ public default void onClosed(Stream stream) /** *

A {@link Retainable} wrapper of a {@link DataFrame}.

*/ - public abstract static class Data implements Retainable + abstract class Data implements Retainable { public static Data eof(int streamId) { diff --git a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/HTTP3StreamConnection.java b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/HTTP3StreamConnection.java index a2917482d52a..842a566387af 100644 --- a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/HTTP3StreamConnection.java +++ b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/HTTP3StreamConnection.java @@ -438,6 +438,12 @@ public boolean canRetain() return retainable.canRetain(); } + @Override + public boolean isRetained() + { + return retainable.isRetained(); + } + @Override public void retain() { diff --git a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/api/Stream.java b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/api/Stream.java index d2e83791404a..9953e2ea04e8 100644 --- a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/api/Stream.java +++ b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/api/Stream.java @@ -384,7 +384,7 @@ public default void onFailure(Stream.Server stream, long error, Throwable failur * * @see Stream#readData() */ - public abstract static class Data implements Retainable + abstract class Data implements Retainable { public static final Data EOF = new EOFData(); diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java index 2096be6e91bb..92c4b8ff038f 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java @@ -105,6 +105,15 @@ public void copyBuffer(ByteBuffer source) } } + public void addBuffer(RetainableByteBuffer buffer) + { + if (buffer != null && buffer.hasRemaining()) + { + buffer.retain(); + _buffers.add(buffer); + } + } + /** * Take the combined buffer containing all content written to the accumulator. * The caller is responsible for releasing this {@link RetainableByteBuffer}. diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index e53a802597d4..35cf23ee0691 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -557,7 +557,7 @@ static void write(Sink sink, boolean last, String utf8Content, Callback callback * to release the {@code ByteBuffer} back into a pool), or the * {@link #release()} method overridden.

*/ - public interface Chunk extends Retainable + public interface Chunk extends RetainableByteBuffer { /** *

An empty, non-last, chunk.

@@ -793,11 +793,6 @@ static boolean isFailure(Chunk chunk, boolean last) return chunk != null && chunk.getFailure() != null && chunk.isLast() == last; } - /** - * @return the ByteBuffer of this Chunk - */ - ByteBuffer getByteBuffer(); - /** * Get a failure (which may be from a {@link Source#fail(Throwable) failure} or * a {@link Source#fail(Throwable, boolean) warning}), if any, associated with the chunk. @@ -820,22 +815,6 @@ default Throwable getFailure() */ boolean isLast(); - /** - * @return the number of bytes remaining in this Chunk - */ - default int remaining() - { - return getByteBuffer().remaining(); - } - - /** - * @return whether this Chunk has remaining bytes - */ - default boolean hasRemaining() - { - return getByteBuffer().hasRemaining(); - } - /** *

Copies the bytes from this Chunk to the given byte array.

* diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java index 0e32781f6279..f9b1ab6bf1e4 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java @@ -62,6 +62,15 @@ default boolean canRetain() return false; } + /** + * @return whether this instance is retained + * @see ReferenceCounter#isRetained() + */ + default boolean isRetained() + { + return false; + } + /** *

Retains this resource, potentially incrementing a reference count if there are resources that will be released.

*/ @@ -103,6 +112,12 @@ public boolean canRetain() return getWrapped().canRetain(); } + @Override + public boolean isRetained() + { + return getWrapped().isRetained(); + } + @Override public void retain() { @@ -195,11 +210,7 @@ public boolean release() return ref == 0; } - /** - *

Returns whether {@link #retain()} has been called at least one more time than {@link #release()}.

- * - * @return whether this buffer is retained - */ + @Override public boolean isRetained() { return references.get() > 1; diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 78e954833502..46b5aa122c31 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -107,12 +107,6 @@ public boolean release() }; } - /** - * @return whether this instance is retained - * @see ReferenceCounter#isRetained() - */ - boolean isRetained(); - /** * Get the wrapped, not {@code null}, {@code ByteBuffer}. * @return the wrapped, not {@code null}, {@code ByteBuffer} diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java index 6bd5eeebc32a..db377b852ed0 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java @@ -301,6 +301,12 @@ public boolean canRetain() return referenceCounter != null; } + @Override + public boolean isRetained() + { + return referenceCounter.isRetained(); + } + @Override public void retain() { diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ByteBufferChunk.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ByteBufferChunk.java index ad011ac93c69..356c043700fa 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ByteBufferChunk.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ByteBufferChunk.java @@ -65,6 +65,12 @@ public WithReferenceCount(ByteBuffer byteBuffer, boolean last) super(byteBuffer, last); } + @Override + public boolean isRetained() + { + return references.isRetained(); + } + @Override public boolean canRetain() { @@ -148,6 +154,12 @@ public WithRetainable(ByteBuffer byteBuffer, boolean last, Retainable retainable this.retainable = retainable; } + @Override + public boolean isRetained() + { + return retainable.isRetained(); + } + @Override public boolean canRetain() { diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java index 9db5923b287f..b9e9a754fe48 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java @@ -52,7 +52,7 @@ public void run() return; } - accumulator.copyBuffer(chunk.getByteBuffer()); + accumulator.addBuffer(chunk); chunk.release(); if (chunk.isLast()) From 59a93077ae4e18535578fa2a3c7bea52a6b8fa0c Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 14 Dec 2023 11:00:20 +1100 Subject: [PATCH 02/43] Review ByteBufferAccumulator #10541 + Added `isRetained()` to `Retainable` + Made `Chunk` is-a `RetainableByteBuffer` + retain rather than copy chunks where possible --- .../jetty/io/ByteBufferAccumulator.java | 61 +++++++++++-------- .../jetty/io/ByteBufferAggregator.java | 1 + 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java index 92c4b8ff038f..81d560caf4a6 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java @@ -25,12 +25,11 @@ * Accumulates data into a list of ByteBuffers which can then be combined into a single buffer or written to an OutputStream. * The buffer list automatically grows as data is written to it, the buffers are taken from the * supplied {@link ByteBufferPool} or freshly allocated if one is not supplied. - * - * The method {@link #ensureBuffer(int, int)} is used to write directly to the last buffer stored in the buffer list, + *

+ * The method {@link #ensureBuffer(int, int)} can be used access a buffer that can be written to directly as the last buffer list, * if there is less than a certain amount of space available in that buffer then a new one will be allocated and returned instead. - * @see #ensureBuffer(int, int) + * @see ByteBufferAggregator */ -// TODO: rename to *Aggregator to avoid confusion with RBBP.Accumulator? public class ByteBufferAccumulator implements AutoCloseable { private final List _buffers = new ArrayList<>(); @@ -109,8 +108,15 @@ public void addBuffer(RetainableByteBuffer buffer) { if (buffer != null && buffer.hasRemaining()) { - buffer.retain(); - _buffers.add(buffer); + if (buffer.canRetain()) + { + buffer.retain(); + _buffers.add(buffer); + } + else + { + copyBuffer(buffer.getByteBuffer()); + } } } @@ -122,26 +128,31 @@ public void addBuffer(RetainableByteBuffer buffer) */ public RetainableByteBuffer takeRetainableByteBuffer() { - RetainableByteBuffer combinedBuffer; - if (_buffers.size() == 1) + return switch (_buffers.size()) { - combinedBuffer = _buffers.get(0); - _buffers.clear(); - return combinedBuffer; - } - - int length = getLength(); - combinedBuffer = _bufferPool.acquire(length, _direct); - ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); - BufferUtil.clearToFill(byteBuffer); - for (RetainableByteBuffer buffer : _buffers) - { - byteBuffer.put(buffer.getByteBuffer()); - buffer.release(); - } - BufferUtil.flipToFlush(byteBuffer, 0); - _buffers.clear(); - return combinedBuffer; + case 0 -> RetainableByteBuffer.EMPTY; + case 1 -> + { + RetainableByteBuffer buffer = _buffers.get(0); + _buffers.clear(); + yield buffer; + } + default -> + { + int length = getLength(); + RetainableByteBuffer combinedBuffer = _bufferPool.acquire(length, _direct); + ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); + BufferUtil.clearToFill(byteBuffer); + for (RetainableByteBuffer buffer : _buffers) + { + byteBuffer.put(buffer.getByteBuffer()); + buffer.release(); + } + BufferUtil.flipToFlush(byteBuffer, 0); + _buffers.clear(); + yield combinedBuffer; + } + }; } public ByteBuffer takeByteBuffer() diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java index 5670d08dc603..9b9638ee160e 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java @@ -25,6 +25,7 @@ * Once the buffer is full, the aggregator will not aggregate any more bytes until its buffer is taken out, * after which a new aggregate/take buffer cycle can start.

*

The buffers are taken from the supplied {@link ByteBufferPool} or freshly allocated if one is not supplied.

+ * @see ByteBufferAccumulator */ public class ByteBufferAggregator { From 854e92c466568ad2ca24de7f7f4ba559f58a0821 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 14 Dec 2023 14:03:40 +1100 Subject: [PATCH 03/43] Review ByteBufferAccumulator #10541 + Added `isRetained()` to `Retainable` + Made `Chunk` is-a `RetainableByteBuffer` + retain rather than copy chunks where possible --- .../jetty/io/RetainableByteBuffer.java | 2 +- .../jetty/io/ByteBufferAccumulatorTest.java | 29 +++++++++++++++++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 46b5aa122c31..2ea813667b57 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -84,7 +84,7 @@ public ByteBuffer getByteBuffer() @Override public boolean isRetained() { - throw new UnsupportedOperationException(); + return retainable.isRetained(); } @Override diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java index 4de6dfc3bc3f..48090a530762 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java @@ -26,7 +26,9 @@ import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; public class ByteBufferAccumulatorTest { @@ -175,7 +177,7 @@ public void testEmptyToBuffer() { RetainableByteBuffer combinedBuffer = accumulator.toRetainableByteBuffer(); assertThat(combinedBuffer.remaining(), is(0)); - assertThat(bufferPool.getAcquireCount(), is(1L)); + assertThat(bufferPool.getAcquireCount(), is(0L)); accumulator.close(); assertThat(bufferPool.getAcquireCount(), is(0L)); } @@ -186,7 +188,7 @@ public void testEmptyTakeBuffer() RetainableByteBuffer combinedBuffer = accumulator.takeRetainableByteBuffer(); assertThat(combinedBuffer.remaining(), is(0)); accumulator.close(); - assertThat(bufferPool.getAcquireCount(), is(1L)); + assertThat(bufferPool.getAcquireCount(), is(0L)); combinedBuffer.release(); assertThat(bufferPool.getAcquireCount(), is(0L)); } @@ -277,6 +279,29 @@ public void testCopy() assertThat(bufferPool.getAcquireCount(), is(0L)); } + @Test + public void testAdd() + { + Retainable.ReferenceCounter counter = new Retainable.ReferenceCounter(); + RetainableByteBuffer buffer = RetainableByteBuffer.wrap(BufferUtil.toBuffer("Ground Hog Day\n"), counter); + accumulator.addBuffer(buffer); + assertTrue(buffer.isRetained()); + accumulator.addBuffer(buffer); + accumulator.addBuffer(buffer); + accumulator.addBuffer(buffer); + + ByteBuffer taken = accumulator.takeByteBuffer(); + assertFalse(buffer.isRetained()); + assertTrue(buffer.release()); + + assertThat(BufferUtil.toString(taken), is(""" + Ground Hog Day + Ground Hog Day + Ground Hog Day + Ground Hog Day + """)); + } + private ByteBuffer randomBytes(int size) { byte[] data = new byte[size]; From f75ded5e6d1fa19a007017986416348bb7995d83 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 15 Dec 2023 09:12:36 +1100 Subject: [PATCH 04/43] Review ByteBufferAccumulator #10541 + Added `isRetained()` to `Retainable` + Made `Chunk` is-a `RetainableByteBuffer` + retain rather than copy chunks where possible + Merged aggregator into accumulator + Deleted ByteBufferAggregator.java --- .../jetty/io/ByteBufferAccumulator.java | 108 +++++++++++-- .../jetty/io/ByteBufferAggregator.java | 148 ------------------ .../jetty/io/content/BufferedContentSink.java | 8 +- .../jetty/io/ByteBufferAccumulatorTest.java | 60 +++++-- .../jetty/io/ByteBufferAggregatorTest.java | 88 ----------- 5 files changed, 151 insertions(+), 261 deletions(-) delete mode 100644 jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java delete mode 100644 jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAggregatorTest.java diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java index 81d560caf4a6..62eb6978b3a9 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java @@ -35,6 +35,7 @@ public class ByteBufferAccumulator implements AutoCloseable private final List _buffers = new ArrayList<>(); private final ByteBufferPool _bufferPool; private final boolean _direct; + private final long _maxLength; public ByteBufferAccumulator() { @@ -42,9 +43,15 @@ public ByteBufferAccumulator() } public ByteBufferAccumulator(ByteBufferPool bufferPool, boolean direct) + { + this(bufferPool, direct, -1); + } + + public ByteBufferAccumulator(ByteBufferPool bufferPool, boolean direct, long maxLength) { _bufferPool = (bufferPool == null) ? new ByteBufferPool.NonPooling() : bufferPool; _direct = direct; + _maxLength = maxLength; } /** @@ -52,9 +59,9 @@ public ByteBufferAccumulator(ByteBufferPool bufferPool, boolean direct) * This will add up the remaining of each buffer in the accumulator. * @return the total length of the content in the accumulator. */ - public int getLength() + public long getLength() { - int length = 0; + long length = 0; for (RetainableByteBuffer buffer : _buffers) length = Math.addExact(length, buffer.remaining()); return length; @@ -87,6 +94,14 @@ public RetainableByteBuffer ensureBuffer(int minSize, int minAllocationSize) return buffer; } + private boolean maxLengthExceeded(long extraLength) + { + if (_maxLength < 0) + return false; + long length = Math.addExact(getLength(), extraLength); + return length > _maxLength; + } + public void copyBytes(byte[] buf, int offset, int length) { copyBuffer(BufferUtil.toBuffer(buf, offset, length)); @@ -94,6 +109,8 @@ public void copyBytes(byte[] buf, int offset, int length) public void copyBuffer(ByteBuffer source) { + if (maxLengthExceeded(source.remaining())) + throw new IllegalArgumentException("maxLength exceeded"); while (source.hasRemaining()) { RetainableByteBuffer buffer = ensureBuffer(source.remaining()); @@ -108,6 +125,9 @@ public void addBuffer(RetainableByteBuffer buffer) { if (buffer != null && buffer.hasRemaining()) { + if (maxLengthExceeded(buffer.remaining())) + throw new IllegalArgumentException("maxLength exceeded"); + if (buffer.canRetain()) { buffer.retain(); @@ -120,6 +140,75 @@ public void addBuffer(RetainableByteBuffer buffer) } } + /** + * Aggregates the given ByteBuffer into the last buffer of this accumulation, growing the buffer in size if + * necessary. This copies bytes up to the specified maximum size into the + * last buffer in the accumulation, at which time this method returns {@code true} + * and {@link #takeRetainableByteBuffer()} must be called for this method to accept aggregating again. + * @param source the buffer to copy into this aggregator; its position is updated according to + * the number of aggregated bytes + * @return true if the aggregator's buffer is full and should be taken, false otherwise + */ + public boolean aggregate(ByteBuffer source) + { + if (BufferUtil.isEmpty(source)) + return false; + + // How much of the buffer can be aggregated? + int toCopy = source.remaining(); + boolean full = false; + if (_maxLength >= 0) + { + long space = _maxLength - getLength(); + if (space == 0) + return true; + if (toCopy >= space) + { + full = true; + toCopy = (int)space; + } + } + + // Do we need to allocate a new buffer? + RetainableByteBuffer buffer = _buffers.isEmpty() ? null : _buffers.get(_buffers.size() - 1); + if (buffer == null || buffer.isRetained() || BufferUtil.space(buffer.getByteBuffer()) < toCopy) + { + int prefix = buffer == null ? 0 : buffer.remaining(); + int minSize = prefix + toCopy; + int allocSize = (int)Math.min(_maxLength, ceilToNextPowerOfTwo(minSize)); + RetainableByteBuffer next = _bufferPool.acquire(allocSize, _direct); + + if (prefix > 0) + BufferUtil.append(next.getByteBuffer(), buffer.getByteBuffer()); + + if (buffer == null) + _buffers.add(next); + else + { + _buffers.set(_buffers.size() - 1, next); + buffer.release(); + } + buffer = next; + } + + // Aggregate the bytes into the prepared space + ByteBuffer target = buffer.getByteBuffer(); + int p = BufferUtil.flipToFill(target); + int tp = target.position(); + target.put(tp, source, 0, toCopy); + source.position(source.position() + toCopy); + target.position(tp + toCopy); + BufferUtil.flipToFlush(target, p); + + return full; + } + + private static int ceilToNextPowerOfTwo(int val) + { + int result = 1 << (Integer.SIZE - Integer.numberOfLeadingZeros(val - 1)); + return result > 0 ? result : Integer.MAX_VALUE; + } + /** * Take the combined buffer containing all content written to the accumulator. * The caller is responsible for releasing this {@link RetainableByteBuffer}. @@ -139,8 +228,10 @@ public RetainableByteBuffer takeRetainableByteBuffer() } default -> { - int length = getLength(); - RetainableByteBuffer combinedBuffer = _bufferPool.acquire(length, _direct); + long length = getLength(); + if (length > Integer.MAX_VALUE) + throw new IllegalStateException("too large for ByteBuffer"); + RetainableByteBuffer combinedBuffer = _bufferPool.acquire((int)length, _direct); ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); BufferUtil.clearToFill(byteBuffer); for (RetainableByteBuffer buffer : _buffers) @@ -182,11 +273,12 @@ public RetainableByteBuffer toRetainableByteBuffer() */ public byte[] toByteArray() { - int length = getLength(); + long length = getLength(); if (length == 0) return new byte[0]; - - byte[] bytes = new byte[length]; + if (length > Integer.MAX_VALUE) + throw new IllegalStateException("too large for array"); + byte[] bytes = new byte[(int)length]; ByteBuffer buffer = BufferUtil.toBuffer(bytes); BufferUtil.clear(buffer); writeTo(buffer); @@ -197,9 +289,7 @@ public void writeTo(ByteBuffer byteBuffer) { int pos = BufferUtil.flipToFill(byteBuffer); for (RetainableByteBuffer buffer : _buffers) - { byteBuffer.put(buffer.getByteBuffer().slice()); - } BufferUtil.flipToFlush(byteBuffer, pos); } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java deleted file mode 100644 index 9b9638ee160e..000000000000 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java +++ /dev/null @@ -1,148 +0,0 @@ -// -// ======================================================================== -// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. -// -// This program and the accompanying materials are made available under the -// terms of the Eclipse Public License v. 2.0 which is available at -// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 -// which is available at https://www.apache.org/licenses/LICENSE-2.0. -// -// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 -// ======================================================================== -// - -package org.eclipse.jetty.io; - -import java.nio.ByteBuffer; - -import org.eclipse.jetty.util.BufferUtil; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -/** - *

Aggregates data into a single ByteBuffer of a specified maximum size.

- *

The buffer automatically grows as data is written to it, up until it reaches the specified maximum size. - * Once the buffer is full, the aggregator will not aggregate any more bytes until its buffer is taken out, - * after which a new aggregate/take buffer cycle can start.

- *

The buffers are taken from the supplied {@link ByteBufferPool} or freshly allocated if one is not supplied.

- * @see ByteBufferAccumulator - */ -public class ByteBufferAggregator -{ - private static final Logger LOG = LoggerFactory.getLogger(ByteBufferAggregator.class); - - private final ByteBufferPool _bufferPool; - private final boolean _direct; - private final int _maxSize; - private RetainableByteBuffer _retainableByteBuffer; - private int _aggregatedSize; - private int _currentSize; - - /** - * Creates a ByteBuffer aggregator. - * @param bufferPool The {@link ByteBufferPool} from which to acquire the buffers - * @param direct whether to get direct buffers - * @param startSize the starting size of the buffer - * @param maxSize the maximum size of the buffer which must be greater than {@code startSize} - */ - public ByteBufferAggregator(ByteBufferPool bufferPool, boolean direct, int startSize, int maxSize) - { - if (maxSize <= 0) - throw new IllegalArgumentException("maxSize must be > 0, was: " + maxSize); - if (startSize <= 0) - throw new IllegalArgumentException("startSize must be > 0, was: " + startSize); - if (startSize > maxSize) - throw new IllegalArgumentException("maxSize (" + maxSize + ") must be >= startSize (" + startSize + ")"); - _bufferPool = (bufferPool == null) ? new ByteBufferPool.NonPooling() : bufferPool; - _direct = direct; - _maxSize = maxSize; - _currentSize = startSize; - } - - /** - * Get the currently aggregated length. - * @return The current total aggregated bytes. - */ - public int length() - { - return _aggregatedSize; - } - - /** - * Aggregates the given ByteBuffer. This copies bytes up to the specified maximum size, at which - * time this method returns {@code true} and {@link #takeRetainableByteBuffer()} must be called - * for this method to accept aggregating again. - * @param buffer the buffer to copy into this aggregator; its position is updated according to - * the number of aggregated bytes - * @return true if the aggregator's buffer is full and should be taken, false otherwise - */ - public boolean aggregate(ByteBuffer buffer) - { - tryExpandBufferCapacity(buffer.remaining()); - if (_retainableByteBuffer == null) - { - _retainableByteBuffer = _bufferPool.acquire(_currentSize, _direct); - BufferUtil.flipToFill(_retainableByteBuffer.getByteBuffer()); - } - int copySize = Math.min(_currentSize - _aggregatedSize, buffer.remaining()); - - ByteBuffer byteBuffer = _retainableByteBuffer.getByteBuffer(); - byteBuffer.put(byteBuffer.position(), buffer, buffer.position(), copySize); - byteBuffer.position(byteBuffer.position() + copySize); - buffer.position(buffer.position() + copySize); - _aggregatedSize += copySize; - return _aggregatedSize == _maxSize; - } - - private void tryExpandBufferCapacity(int remaining) - { - if (LOG.isDebugEnabled()) - LOG.debug("tryExpandBufferCapacity remaining: {} _currentSize: {} _accumulatedSize={}", remaining, _currentSize, _aggregatedSize); - if (_currentSize == _maxSize) - return; - int capacityLeft = _currentSize - _aggregatedSize; - if (remaining <= capacityLeft) - return; - int need = remaining - capacityLeft; - _currentSize = Math.min(_maxSize, ceilToNextPowerOfTwo(_currentSize + need)); - - if (_retainableByteBuffer != null) - { - BufferUtil.flipToFlush(_retainableByteBuffer.getByteBuffer(), 0); - RetainableByteBuffer newBuffer = _bufferPool.acquire(_currentSize, _direct); - BufferUtil.flipToFill(newBuffer.getByteBuffer()); - newBuffer.getByteBuffer().put(_retainableByteBuffer.getByteBuffer()); - _retainableByteBuffer.release(); - _retainableByteBuffer = newBuffer; - } - } - - private static int ceilToNextPowerOfTwo(int val) - { - int result = 1 << (Integer.SIZE - Integer.numberOfLeadingZeros(val - 1)); - return result > 0 ? result : Integer.MAX_VALUE; - } - - /** - * Takes the buffer out of the aggregator. Once the buffer has been taken out, - * the aggregator resets itself and a new buffer will be acquired from the pool - * during the next {@link #aggregate(ByteBuffer)} call. - * @return the aggregated buffer, or null if nothing has been buffered yet - */ - public RetainableByteBuffer takeRetainableByteBuffer() - { - if (_retainableByteBuffer == null) - return null; - BufferUtil.flipToFlush(_retainableByteBuffer.getByteBuffer(), 0); - RetainableByteBuffer result = _retainableByteBuffer; - _retainableByteBuffer = null; - _aggregatedSize = 0; - return result; - } - - @Override - public String toString() - { - return "%s@%x{a=%d c=%d m=%d b=%s}".formatted(getClass().getSimpleName(), hashCode(), _aggregatedSize, _currentSize, _maxSize, _retainableByteBuffer); - } -} diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java index acb85e2ddb60..278c5834edff 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java @@ -17,7 +17,7 @@ import java.nio.ByteBuffer; import java.nio.channels.WritePendingException; -import org.eclipse.jetty.io.ByteBufferAggregator; +import org.eclipse.jetty.io.ByteBufferAccumulator; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.RetainableByteBuffer; @@ -43,15 +43,13 @@ public class BufferedContentSink implements Content.Sink private static final Logger LOG = LoggerFactory.getLogger(BufferedContentSink.class); - private static final int START_BUFFER_SIZE = 1024; - private final Content.Sink _delegate; private final ByteBufferPool _bufferPool; private final boolean _direct; private final int _maxBufferSize; private final int _maxAggregationSize; private final Flusher _flusher; - private ByteBufferAggregator _aggregator; + private ByteBufferAccumulator _aggregator; private boolean _firstWrite = true; private boolean _lastWritten; @@ -99,7 +97,7 @@ public void write(boolean last, ByteBuffer byteBuffer, Callback callback) { // current buffer can be aggregated if (_aggregator == null) - _aggregator = new ByteBufferAggregator(_bufferPool, _direct, Math.min(START_BUFFER_SIZE, _maxBufferSize), _maxBufferSize); + _aggregator = new ByteBufferAccumulator(_bufferPool, _direct, _maxBufferSize); aggregateAndFlush(last, current, callback); } else diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java index 48090a530762..746cdf7adf20 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java @@ -99,11 +99,11 @@ public void testToBuffer() } // Check we have the same content as the original buffer. - assertThat(accumulator.getLength(), is(size)); + assertThat(accumulator.getLength(), is((long)size)); assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); RetainableByteBuffer combinedBuffer = accumulator.toRetainableByteBuffer(); assertThat(bufferPool.getAcquireCount(), is(1L)); - assertThat(accumulator.getLength(), is(size)); + assertThat(accumulator.getLength(), is((long)size)); assertThat(combinedBuffer.getByteBuffer(), is(content)); // Close the accumulator and make sure all is returned to bufferPool. @@ -129,11 +129,11 @@ public void testTakeBuffer() } // Check we have the same content as the original buffer. - assertThat(accumulator.getLength(), is(size)); + assertThat(accumulator.getLength(), is((long)size)); assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); RetainableByteBuffer combinedBuffer = accumulator.takeRetainableByteBuffer(); assertThat(bufferPool.getAcquireCount(), is(1L)); - assertThat(accumulator.getLength(), is(0)); + assertThat(accumulator.getLength(), is(0L)); accumulator.close(); assertThat(bufferPool.getAcquireCount(), is(1L)); assertThat(combinedBuffer.getByteBuffer(), is(content)); @@ -160,11 +160,11 @@ public void testToByteArray() } // Check we have the same content as the original buffer. - assertThat(accumulator.getLength(), is(size)); + assertThat(accumulator.getLength(), is((long)size)); assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); byte[] combinedBuffer = accumulator.toByteArray(); assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); - assertThat(accumulator.getLength(), is(size)); + assertThat(accumulator.getLength(), is((long)size)); assertThat(BufferUtil.toBuffer(combinedBuffer), is(content)); // Close the accumulator and make sure all is returned to bufferPool. @@ -211,9 +211,9 @@ public void testWriteTo() // Check we have the same content as the original buffer. assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); - RetainableByteBuffer combinedBuffer = bufferPool.acquire(accumulator.getLength(), false); + RetainableByteBuffer combinedBuffer = bufferPool.acquire((int)accumulator.getLength(), false); accumulator.writeTo(combinedBuffer.getByteBuffer()); - assertThat(accumulator.getLength(), is(size)); + assertThat(accumulator.getLength(), is((long)size)); assertThat(combinedBuffer.getByteBuffer(), is(content)); combinedBuffer.release(); @@ -240,7 +240,7 @@ public void testWriteToBufferTooSmall() // Writing to a buffer too small gives buffer overflow. assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); - ByteBuffer combinedBuffer = BufferUtil.toBuffer(new byte[accumulator.getLength() - 1]); + ByteBuffer combinedBuffer = BufferUtil.toBuffer(new byte[(int)accumulator.getLength() - 1]); BufferUtil.clear(combinedBuffer); assertThrows(BufferOverflowException.class, () -> accumulator.writeTo(combinedBuffer)); @@ -268,9 +268,9 @@ public void testCopy() // Check we have the same content as the original buffer. assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); - RetainableByteBuffer combinedBuffer = bufferPool.acquire(accumulator.getLength(), false); + RetainableByteBuffer combinedBuffer = bufferPool.acquire((int)accumulator.getLength(), false); accumulator.writeTo(combinedBuffer.getByteBuffer()); - assertThat(accumulator.getLength(), is(size)); + assertThat(accumulator.getLength(), is((long)size)); assertThat(combinedBuffer.getByteBuffer(), is(content)); combinedBuffer.release(); @@ -348,4 +348,42 @@ public long getAcquireCount() return _acquires.get(); } } + + @Test + public void testAggregateFullInSingleShot() + { + try (ByteBufferAccumulator accumulator = new ByteBufferAccumulator(bufferPool, true, 16)) + { + ByteBuffer byteBuffer1 = ByteBuffer.wrap(new byte[16]); + assertThat(accumulator.aggregate(byteBuffer1), is(true)); + assertThat(byteBuffer1.remaining(), is(0)); + + ByteBuffer byteBuffer2 = ByteBuffer.wrap(new byte[16]); + assertThat(accumulator.aggregate(byteBuffer2), is(true)); + assertThat(byteBuffer2.remaining(), is(16)); + + RetainableByteBuffer retainableByteBuffer = accumulator.takeRetainableByteBuffer(); + assertThat(retainableByteBuffer.getByteBuffer().remaining(), is(16)); + assertThat(retainableByteBuffer.release(), is(true)); + } + } + + @Test + public void testAggregateFullInMultipleShots() + { + try (ByteBufferAccumulator accumulator = new ByteBufferAccumulator(bufferPool, true, 16)) + { + ByteBuffer byteBuffer1 = ByteBuffer.wrap(new byte[15]); + assertThat(accumulator.aggregate(byteBuffer1), is(false)); + assertThat(byteBuffer1.remaining(), is(0)); + + ByteBuffer byteBuffer2 = ByteBuffer.wrap(new byte[16]); + assertThat(accumulator.aggregate(byteBuffer2), is(true)); + assertThat(byteBuffer2.remaining(), is(15)); + + RetainableByteBuffer retainableByteBuffer = accumulator.takeRetainableByteBuffer(); + assertThat(retainableByteBuffer.getByteBuffer().remaining(), is(16)); + assertThat(retainableByteBuffer.release(), is(true)); + } + } } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAggregatorTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAggregatorTest.java deleted file mode 100644 index 80509a364522..000000000000 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAggregatorTest.java +++ /dev/null @@ -1,88 +0,0 @@ -// -// ======================================================================== -// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. -// -// This program and the accompanying materials are made available under the -// terms of the Eclipse Public License v. 2.0 which is available at -// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 -// which is available at https://www.apache.org/licenses/LICENSE-2.0. -// -// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 -// ======================================================================== -// - -package org.eclipse.jetty.io; - -import java.nio.ByteBuffer; - -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.is; -import static org.junit.jupiter.api.Assertions.assertThrows; - -public class ByteBufferAggregatorTest -{ - private ArrayByteBufferPool.Tracking bufferPool; - - @BeforeEach - public void before() - { - bufferPool = new ArrayByteBufferPool.Tracking(); - } - - @AfterEach - public void tearDown() - { - assertThat("Leaks: " + bufferPool.dumpLeaks(), bufferPool.getLeaks().size(), is(0)); - } - - @Test - public void testConstructor() - { - assertThrows(IllegalArgumentException.class, () -> new ByteBufferAggregator(bufferPool, true, 0, 0)); - assertThrows(IllegalArgumentException.class, () -> new ByteBufferAggregator(bufferPool, true, 0, 1)); - assertThrows(IllegalArgumentException.class, () -> new ByteBufferAggregator(bufferPool, true, 1, 0)); - assertThrows(IllegalArgumentException.class, () -> new ByteBufferAggregator(bufferPool, true, 0, -1)); - assertThrows(IllegalArgumentException.class, () -> new ByteBufferAggregator(bufferPool, true, -1, 0)); - assertThrows(IllegalArgumentException.class, () -> new ByteBufferAggregator(bufferPool, true, 2, 1)); - } - - @Test - public void testFullInSingleShot() - { - ByteBufferAggregator aggregator = new ByteBufferAggregator(bufferPool, true, 1, 16); - - ByteBuffer byteBuffer1 = ByteBuffer.wrap(new byte[16]); - assertThat(aggregator.aggregate(byteBuffer1), is(true)); - assertThat(byteBuffer1.remaining(), is(0)); - - ByteBuffer byteBuffer2 = ByteBuffer.wrap(new byte[16]); - assertThat(aggregator.aggregate(byteBuffer2), is(true)); - assertThat(byteBuffer2.remaining(), is(16)); - - RetainableByteBuffer retainableByteBuffer = aggregator.takeRetainableByteBuffer(); - assertThat(retainableByteBuffer.getByteBuffer().remaining(), is(16)); - assertThat(retainableByteBuffer.release(), is(true)); - } - - @Test - public void testFullInMultipleShots() - { - ByteBufferAggregator aggregator = new ByteBufferAggregator(bufferPool, true, 1, 16); - - ByteBuffer byteBuffer1 = ByteBuffer.wrap(new byte[15]); - assertThat(aggregator.aggregate(byteBuffer1), is(false)); - assertThat(byteBuffer1.remaining(), is(0)); - - ByteBuffer byteBuffer2 = ByteBuffer.wrap(new byte[16]); - assertThat(aggregator.aggregate(byteBuffer2), is(true)); - assertThat(byteBuffer2.remaining(), is(15)); - - RetainableByteBuffer retainableByteBuffer = aggregator.takeRetainableByteBuffer(); - assertThat(retainableByteBuffer.getByteBuffer().remaining(), is(16)); - assertThat(retainableByteBuffer.release(), is(true)); - } -} From 2970ce657d8ee7eede32881691aeccc23ec05c14 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 15 Dec 2023 09:33:38 +1100 Subject: [PATCH 05/43] Review ByteBufferAccumulator #10541 + Added `isRetained()` to `Retainable` + Made `Chunk` is-a `RetainableByteBuffer` + retain rather than copy chunks where possible + Merged aggregator into accumulator + Deleted ByteBufferAggregator.java --- .../jetty/io/content/BufferedContentSink.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java index 278c5834edff..4f48cd208ce5 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java @@ -49,7 +49,7 @@ public class BufferedContentSink implements Content.Sink private final int _maxBufferSize; private final int _maxAggregationSize; private final Flusher _flusher; - private ByteBufferAccumulator _aggregator; + private ByteBufferAccumulator _accumulator; private boolean _firstWrite = true; private boolean _lastWritten; @@ -96,8 +96,8 @@ public void write(boolean last, ByteBuffer byteBuffer, Callback callback) if (current.remaining() <= _maxAggregationSize) { // current buffer can be aggregated - if (_aggregator == null) - _aggregator = new ByteBufferAccumulator(_bufferPool, _direct, _maxBufferSize); + if (_accumulator == null) + _accumulator = new ByteBufferAccumulator(_bufferPool, _direct, _maxBufferSize); aggregateAndFlush(last, current, callback); } else @@ -125,7 +125,7 @@ private void flush(boolean last, ByteBuffer currentBuffer, Callback callback) if (LOG.isDebugEnabled()) LOG.debug("given buffer is greater than _maxBufferSize"); - RetainableByteBuffer aggregatedBuffer = _aggregator == null ? null : _aggregator.takeRetainableByteBuffer(); + RetainableByteBuffer aggregatedBuffer = _accumulator == null ? null : _accumulator.takeRetainableByteBuffer(); if (aggregatedBuffer == null) { if (LOG.isDebugEnabled()) @@ -168,15 +168,15 @@ public void failed(Throwable x) */ private void aggregateAndFlush(boolean last, ByteBuffer currentBuffer, Callback callback) { - boolean full = _aggregator.aggregate(currentBuffer); + boolean full = _accumulator.aggregate(currentBuffer); boolean empty = !currentBuffer.hasRemaining(); boolean flush = full || currentBuffer == FLUSH_BUFFER; boolean complete = last && empty; if (LOG.isDebugEnabled()) - LOG.debug("aggregated current buffer, full={}, complete={}, bytes left={}, aggregator={}", full, complete, currentBuffer.remaining(), _aggregator); + LOG.debug("aggregated current buffer, full={}, complete={}, bytes left={}, aggregator={}", full, complete, currentBuffer.remaining(), _accumulator); if (complete) { - RetainableByteBuffer aggregatedBuffer = _aggregator.takeRetainableByteBuffer(); + RetainableByteBuffer aggregatedBuffer = _accumulator.takeRetainableByteBuffer(); if (aggregatedBuffer != null) { if (LOG.isDebugEnabled()) @@ -192,7 +192,7 @@ private void aggregateAndFlush(boolean last, ByteBuffer currentBuffer, Callback } else if (flush) { - RetainableByteBuffer aggregatedBuffer = _aggregator.takeRetainableByteBuffer(); + RetainableByteBuffer aggregatedBuffer = _accumulator.takeRetainableByteBuffer(); if (LOG.isDebugEnabled()) LOG.debug("writing aggregated buffer: {} bytes, then {}", aggregatedBuffer.remaining(), currentBuffer.remaining()); @@ -230,7 +230,7 @@ public void failed(Throwable x) else { if (LOG.isDebugEnabled()) - LOG.debug("buffer fully aggregated, delaying writing - aggregator: {}", _aggregator); + LOG.debug("buffer fully aggregated, delaying writing - aggregator: {}", _accumulator); _flusher.offer(callback); } } From 30102db50bdf0e4299b6361ebc849ed767ee09de Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 18 Dec 2023 09:49:57 +1100 Subject: [PATCH 06/43] Review ByteBufferAccumulator #10541 + Added `isRetained()` to `Retainable` + Made `Chunk` is-a `RetainableByteBuffer` + retain rather than copy chunks where possible + Merged aggregator into accumulator + Deleted ByteBufferAggregator.java --- .../org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java | 8 ++++---- .../java/org/eclipse/jetty/io/ByteBufferAccumulator.java | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java index 711e705ba55d..c0b87ed22cb2 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java @@ -44,7 +44,7 @@ import org.eclipse.jetty.http2.frames.ResetFrame; import org.eclipse.jetty.http2.server.RawHTTP2ServerConnectionFactory; import org.eclipse.jetty.io.ArrayByteBufferPool; -import org.eclipse.jetty.io.ByteBufferAggregator; +import org.eclipse.jetty.io.ByteBufferAccumulator; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.Server; @@ -245,7 +245,7 @@ public void onDataAvailable(Stream stream) CountDownLatch latch1 = new CountDownLatch(1); Stream stream1 = clientSession.newStream(new HeadersFrame(request1, null, false), new Stream.Listener() { - private final ByteBufferAggregator aggregator = new ByteBufferAggregator(client.getByteBufferPool(), true, data1.length, data1.length * 2); + private final ByteBufferAccumulator accumulator = new ByteBufferAccumulator(client.getByteBufferPool(), true, data1.length * 2); @Override public void onHeaders(Stream stream, HeadersFrame frame) @@ -262,14 +262,14 @@ public void onDataAvailable(Stream stream) DataFrame frame = data.frame(); if (LOGGER.isDebugEnabled()) LOGGER.debug("CLIENT1 received {}", frame); - assertFalse(aggregator.aggregate(frame.getByteBuffer())); + assertFalse(accumulator.aggregate(frame.getByteBuffer())); data.release(); if (!data.frame().isEndStream()) { stream.demand(); return; } - RetainableByteBuffer buffer = aggregator.takeRetainableByteBuffer(); + RetainableByteBuffer buffer = accumulator.takeRetainableByteBuffer(); assertNotNull(buffer); assertEquals(buffer1.slice(), buffer.getByteBuffer()); buffer.release(); diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java index 62eb6978b3a9..acabefa08279 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java @@ -28,7 +28,6 @@ *

* The method {@link #ensureBuffer(int, int)} can be used access a buffer that can be written to directly as the last buffer list, * if there is less than a certain amount of space available in that buffer then a new one will be allocated and returned instead. - * @see ByteBufferAggregator */ public class ByteBufferAccumulator implements AutoCloseable { From 9740820bc59c6e71929658fdf96b3d3699b5b0c1 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 19 Dec 2023 12:12:40 +1100 Subject: [PATCH 07/43] Review ByteBufferAccumulator #10541 + Added `isRetained()` to `Retainable` + Made `Chunk` is-a `RetainableByteBuffer` + retain rather than copy chunks where possible + Merged aggregator into accumulator + Deleted ByteBufferAggregator.java --- .../jetty/io/ByteBufferAccumulator.java | 21 ++- .../jetty/io/ByteBufferAggregator.java | 149 ++++++++++++++++++ .../jetty/io/ByteBufferAccumulatorTest.java | 20 +-- 3 files changed, 177 insertions(+), 13 deletions(-) create mode 100644 jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java index acabefa08279..edaaad8817f9 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java @@ -26,7 +26,7 @@ * The buffer list automatically grows as data is written to it, the buffers are taken from the * supplied {@link ByteBufferPool} or freshly allocated if one is not supplied. *

- * The method {@link #ensureBuffer(int, int)} can be used access a buffer that can be written to directly as the last buffer list, + * The method {@link #accessInternalBuffer(int, int)} can be used access a buffer that can be written to directly as the last buffer list, * if there is less than a certain amount of space available in that buffer then a new one will be allocated and returned instead. */ public class ByteBufferAccumulator implements AutoCloseable @@ -70,10 +70,12 @@ public long getLength() * Get the last buffer of the accumulator, this can be written to directly to avoid copying into the accumulator. * @param minAllocationSize new buffers will be allocated to have at least this size. * @return a buffer with at least {@code minSize} space to write into. + * @deprecated Use {@link #copyBuffer(ByteBuffer)}, {@link #copyBytes(byte[], int, int)} or {@link #addBuffer(RetainableByteBuffer)} */ + @Deprecated public RetainableByteBuffer ensureBuffer(int minAllocationSize) { - return ensureBuffer(1, minAllocationSize); + return accessInternalBuffer(1, minAllocationSize); } /** @@ -81,8 +83,21 @@ public RetainableByteBuffer ensureBuffer(int minAllocationSize) * @param minSize the smallest amount of remaining space before a new buffer is allocated. * @param minAllocationSize new buffers will be allocated to have at least this size. * @return a buffer with at least {@code minSize} space to write into. + * @deprecated Use {@link #copyBuffer(ByteBuffer)}, {@link #copyBytes(byte[], int, int)} or {@link #addBuffer(RetainableByteBuffer)} */ + @Deprecated public RetainableByteBuffer ensureBuffer(int minSize, int minAllocationSize) + { + return accessInternalBuffer(minSize, minAllocationSize); + } + + /** + * Get the last buffer of the accumulator, this can be written to directly to avoid copying into the accumulator. + * @param minSize the smallest amount of remaining space before a new buffer is allocated. + * @param minAllocationSize new buffers will be allocated to have at least this size. + * @return a buffer with at least {@code minSize} space to write into. + */ + RetainableByteBuffer accessInternalBuffer(int minSize, int minAllocationSize) { RetainableByteBuffer buffer = _buffers.isEmpty() ? null : _buffers.get(_buffers.size() - 1); if (buffer == null || BufferUtil.space(buffer.getByteBuffer()) < minSize) @@ -112,7 +127,7 @@ public void copyBuffer(ByteBuffer source) throw new IllegalArgumentException("maxLength exceeded"); while (source.hasRemaining()) { - RetainableByteBuffer buffer = ensureBuffer(source.remaining()); + RetainableByteBuffer buffer = accessInternalBuffer(1, source.remaining()); ByteBuffer byteBuffer = buffer.getByteBuffer(); int pos = BufferUtil.flipToFill(byteBuffer); BufferUtil.put(source, byteBuffer); diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java new file mode 100644 index 000000000000..d69628fd8eb8 --- /dev/null +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAggregator.java @@ -0,0 +1,149 @@ +// +// ======================================================================== +// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.io; + +import java.nio.ByteBuffer; + +import org.eclipse.jetty.util.BufferUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + *

Aggregates data into a single ByteBuffer of a specified maximum size.

+ *

The buffer automatically grows as data is written to it, up until it reaches the specified maximum size. + * Once the buffer is full, the aggregator will not aggregate any more bytes until its buffer is taken out, + * after which a new aggregate/take buffer cycle can start.

+ *

The buffers are taken from the supplied {@link ByteBufferPool} or freshly allocated if one is not supplied.

+ * @deprecated Use {@link ByteBufferAccumulator} + */ +@Deprecated +public class ByteBufferAggregator +{ + private static final Logger LOG = LoggerFactory.getLogger(ByteBufferAggregator.class); + + private final ByteBufferPool _bufferPool; + private final boolean _direct; + private final int _maxSize; + private RetainableByteBuffer _retainableByteBuffer; + private int _aggregatedSize; + private int _currentSize; + + /** + * Creates a ByteBuffer aggregator. + * @param bufferPool The {@link ByteBufferPool} from which to acquire the buffers + * @param direct whether to get direct buffers + * @param startSize the starting size of the buffer + * @param maxSize the maximum size of the buffer which must be greater than {@code startSize} + */ + public ByteBufferAggregator(ByteBufferPool bufferPool, boolean direct, int startSize, int maxSize) + { + if (maxSize <= 0) + throw new IllegalArgumentException("maxSize must be > 0, was: " + maxSize); + if (startSize <= 0) + throw new IllegalArgumentException("startSize must be > 0, was: " + startSize); + if (startSize > maxSize) + throw new IllegalArgumentException("maxSize (" + maxSize + ") must be >= startSize (" + startSize + ")"); + _bufferPool = (bufferPool == null) ? new ByteBufferPool.NonPooling() : bufferPool; + _direct = direct; + _maxSize = maxSize; + _currentSize = startSize; + } + + /** + * Get the currently aggregated length. + * @return The current total aggregated bytes. + */ + public int length() + { + return _aggregatedSize; + } + + /** + * Aggregates the given ByteBuffer. This copies bytes up to the specified maximum size, at which + * time this method returns {@code true} and {@link #takeRetainableByteBuffer()} must be called + * for this method to accept aggregating again. + * @param buffer the buffer to copy into this aggregator; its position is updated according to + * the number of aggregated bytes + * @return true if the aggregator's buffer is full and should be taken, false otherwise + */ + public boolean aggregate(ByteBuffer buffer) + { + tryExpandBufferCapacity(buffer.remaining()); + if (_retainableByteBuffer == null) + { + _retainableByteBuffer = _bufferPool.acquire(_currentSize, _direct); + BufferUtil.flipToFill(_retainableByteBuffer.getByteBuffer()); + } + int copySize = Math.min(_currentSize - _aggregatedSize, buffer.remaining()); + + ByteBuffer byteBuffer = _retainableByteBuffer.getByteBuffer(); + byteBuffer.put(byteBuffer.position(), buffer, buffer.position(), copySize); + byteBuffer.position(byteBuffer.position() + copySize); + buffer.position(buffer.position() + copySize); + _aggregatedSize += copySize; + return _aggregatedSize == _maxSize; + } + + private void tryExpandBufferCapacity(int remaining) + { + if (LOG.isDebugEnabled()) + LOG.debug("tryExpandBufferCapacity remaining: {} _currentSize: {} _accumulatedSize={}", remaining, _currentSize, _aggregatedSize); + if (_currentSize == _maxSize) + return; + int capacityLeft = _currentSize - _aggregatedSize; + if (remaining <= capacityLeft) + return; + int need = remaining - capacityLeft; + _currentSize = Math.min(_maxSize, ceilToNextPowerOfTwo(_currentSize + need)); + + if (_retainableByteBuffer != null) + { + BufferUtil.flipToFlush(_retainableByteBuffer.getByteBuffer(), 0); + RetainableByteBuffer newBuffer = _bufferPool.acquire(_currentSize, _direct); + BufferUtil.flipToFill(newBuffer.getByteBuffer()); + newBuffer.getByteBuffer().put(_retainableByteBuffer.getByteBuffer()); + _retainableByteBuffer.release(); + _retainableByteBuffer = newBuffer; + } + } + + private static int ceilToNextPowerOfTwo(int val) + { + int result = 1 << (Integer.SIZE - Integer.numberOfLeadingZeros(val - 1)); + return result > 0 ? result : Integer.MAX_VALUE; + } + + /** + * Takes the buffer out of the aggregator. Once the buffer has been taken out, + * the aggregator resets itself and a new buffer will be acquired from the pool + * during the next {@link #aggregate(ByteBuffer)} call. + * @return the aggregated buffer, or null if nothing has been buffered yet + */ + public RetainableByteBuffer takeRetainableByteBuffer() + { + if (_retainableByteBuffer == null) + return null; + BufferUtil.flipToFlush(_retainableByteBuffer.getByteBuffer(), 0); + RetainableByteBuffer result = _retainableByteBuffer; + _retainableByteBuffer = null; + _aggregatedSize = 0; + return result; + } + + @Override + public String toString() + { + return "%s@%x{a=%d c=%d m=%d b=%s}".formatted(getClass().getSimpleName(), hashCode(), _aggregatedSize, _currentSize, _maxSize, _retainableByteBuffer); + } +} diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java index 746cdf7adf20..89c6f651c1e0 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java @@ -51,19 +51,19 @@ public void testToBuffer() ByteBuffer slice = content.slice(); // We completely fill up the internal buffer with the first write. - RetainableByteBuffer internalBuffer = accumulator.ensureBuffer(1, allocationSize); + RetainableByteBuffer internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); ByteBuffer byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), greaterThanOrEqualTo(allocationSize)); writeInFlushMode(slice, byteBuffer); assertThat(BufferUtil.space(byteBuffer), is(0)); // If we ask for min size of 0 we get the same buffer which is full. - internalBuffer = accumulator.ensureBuffer(0, allocationSize); + internalBuffer = accumulator.accessInternalBuffer(0, allocationSize); byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), is(0)); // If we need at least 1 minSpace we must allocate a new buffer. - internalBuffer = accumulator.ensureBuffer(1, allocationSize); + internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), greaterThan(0)); assertThat(BufferUtil.space(byteBuffer), greaterThanOrEqualTo(allocationSize)); @@ -79,20 +79,20 @@ public void testToBuffer() // If we request anything under the amount remaining we get back the same buffer. for (int i = 0; i <= 13; i++) { - internalBuffer = accumulator.ensureBuffer(i, allocationSize); + internalBuffer = accumulator.accessInternalBuffer(i, allocationSize); byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), is(13)); } // If we request over 13 then we get a new buffer. - internalBuffer = accumulator.ensureBuffer(14, allocationSize); + internalBuffer = accumulator.accessInternalBuffer(14, allocationSize); byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), greaterThanOrEqualTo(1024)); // Copy the rest of the content. while (slice.hasRemaining()) { - internalBuffer = accumulator.ensureBuffer(1, allocationSize); + internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), greaterThanOrEqualTo(1)); writeInFlushMode(slice, byteBuffer); @@ -122,7 +122,7 @@ public void testTakeBuffer() // Copy the content. while (slice.hasRemaining()) { - RetainableByteBuffer internalBuffer = accumulator.ensureBuffer(1, allocationSize); + RetainableByteBuffer internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); ByteBuffer byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), greaterThanOrEqualTo(1)); writeInFlushMode(slice, byteBuffer); @@ -154,7 +154,7 @@ public void testToByteArray() // Copy the content. while (slice.hasRemaining()) { - RetainableByteBuffer internalBuffer = accumulator.ensureBuffer(1, allocationSize); + RetainableByteBuffer internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); ByteBuffer byteBuffer = internalBuffer.getByteBuffer(); writeInFlushMode(slice, byteBuffer); } @@ -204,7 +204,7 @@ public void testWriteTo() // Copy the content. while (slice.hasRemaining()) { - RetainableByteBuffer internalBuffer = accumulator.ensureBuffer(1, allocationSize); + RetainableByteBuffer internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); ByteBuffer byteBuffer = internalBuffer.getByteBuffer(); writeInFlushMode(slice, byteBuffer); } @@ -233,7 +233,7 @@ public void testWriteToBufferTooSmall() // Copy the content. while (slice.hasRemaining()) { - RetainableByteBuffer internalBuffer = accumulator.ensureBuffer(1, allocationSize); + RetainableByteBuffer internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); ByteBuffer byteBuffer = internalBuffer.getByteBuffer(); writeInFlushMode(slice, byteBuffer); } From 2ea7810af74cc5908280812357ba3731dc51071a Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 21 Dec 2023 09:53:19 +1100 Subject: [PATCH 08/43] Review ByteBufferAccumulator #10541 + Added `isRetained()` to `Retainable` + Made `Chunk` is-a `RetainableByteBuffer` + retain rather than copy chunks where possible + Merged aggregator into accumulator + Deprecated ByteBufferAggregator.java + Deprecated ByteBufferCallbackAccumulator --- .../jetty/io/ByteBufferAccumulator.java | 108 ++++++++++++++++-- .../io/ByteBufferCallbackAccumulator.java | 2 + .../org/eclipse/jetty/util/BufferUtil.java | 2 +- .../eclipse/jetty/util/Utf8StringBuilder.java | 4 + .../core/messages/ByteArrayMessageSink.java | 8 +- .../core/messages/ByteBufferMessageSink.java | 12 +- .../PermessageDeflateDemandTest.java | 8 +- 7 files changed, 122 insertions(+), 22 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java index edaaad8817f9..52226e5d0ab7 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java @@ -18,8 +18,10 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.Callback; /** * Accumulates data into a list of ByteBuffers which can then be combined into a single buffer or written to an OutputStream. @@ -116,25 +118,45 @@ private boolean maxLengthExceeded(long extraLength) return length > _maxLength; } + /** + * Copy bytes from an array into this accumulator + * @param buf The byte array to copy from + * @param offset The offset into the array to start copying from + * @param length The number of bytes to copy + * @throws IllegalArgumentException if the max length is exceeded. + */ public void copyBytes(byte[] buf, int offset, int length) + throws IllegalArgumentException { copyBuffer(BufferUtil.toBuffer(buf, offset, length)); } - public void copyBuffer(ByteBuffer source) + /** + * Copy bytes from a {@link ByteBuffer} into this accumulator + * @param buffer The {@code ByteBuffer} to copy from, whose position is updated. + * @throws IllegalArgumentException if the max length is exceeded. + */ + public void copyBuffer(ByteBuffer buffer) + throws IllegalArgumentException { - if (maxLengthExceeded(source.remaining())) + if (maxLengthExceeded(buffer.remaining())) throw new IllegalArgumentException("maxLength exceeded"); - while (source.hasRemaining()) + while (buffer.hasRemaining()) { - RetainableByteBuffer buffer = accessInternalBuffer(1, source.remaining()); - ByteBuffer byteBuffer = buffer.getByteBuffer(); + RetainableByteBuffer target = accessInternalBuffer(1, buffer.remaining()); + ByteBuffer byteBuffer = target.getByteBuffer(); int pos = BufferUtil.flipToFill(byteBuffer); - BufferUtil.put(source, byteBuffer); + BufferUtil.put(buffer, byteBuffer); BufferUtil.flipToFlush(byteBuffer, pos); } } + /** + * Add (without copying if possible) a {@link RetainableByteBuffer} + * @param buffer The {@code RetainableByteBuffer} to add to the accumulator, which will either be + * {@link Retainable#retain() retained} or copied if it {@link Retainable#canRetain() cannot be retained}. + * The buffers position will not be updated. + */ public void addBuffer(RetainableByteBuffer buffer) { if (buffer != null && buffer.hasRemaining()) @@ -149,11 +171,29 @@ public void addBuffer(RetainableByteBuffer buffer) } else { - copyBuffer(buffer.getByteBuffer()); + copyBuffer(buffer.getByteBuffer().slice()); } } } + /** + * Add without copying a {@link ByteBuffer}. + * @param buffer The {@code ByteBuffer} to add. + * @param releaseCallback A callback that is {@link Callback#succeeded() succeeded} when the buffer is no longer held, + * or {@link Callback#failed(Throwable) failed} if the {@link #fail(Throwable)} method is called. + */ + public void addBuffer(ByteBuffer buffer, Callback releaseCallback) + { + if (BufferUtil.isEmpty(buffer)) + { + releaseCallback.succeeded(); + } + else + { + _buffers.add(new FailableRetainableByteBuffer(buffer, releaseCallback)); + } + } + /** * Aggregates the given ByteBuffer into the last buffer of this accumulation, growing the buffer in size if * necessary. This copies bytes up to the specified maximum size into the @@ -299,6 +339,13 @@ public byte[] toByteArray() return bytes; } + public byte[] takeByteArray() + { + byte[] out = toByteArray(); + close(); + return out; + } + public void writeTo(ByteBuffer byteBuffer) { int pos = BufferUtil.flipToFill(byteBuffer); @@ -321,4 +368,51 @@ public void close() _buffers.forEach(RetainableByteBuffer::release); _buffers.clear(); } + + public void fail(Throwable cause) + { + _buffers.forEach(r -> + { + if (r instanceof FailableRetainableByteBuffer frbb) + frbb.fail(cause); + else + r.release(); + }); + _buffers.clear(); + } + + private static class FailableRetainableByteBuffer implements RetainableByteBuffer + { + private final ByteBuffer _buffer; + private final AtomicReference _callback; + + private FailableRetainableByteBuffer(ByteBuffer buffer, Callback callback) + { + _buffer = buffer; + _callback = new AtomicReference<>(callback); + } + + @Override + public ByteBuffer getByteBuffer() + { + return _buffer; + } + + @Override + public boolean release() + { + Callback callback = _callback.getAndSet(null); + if (callback == null) + return false; + callback.succeeded(); + return true; + } + + public void fail(Throwable cause) + { + Callback callback = _callback.getAndSet(null); + if (callback != null) + callback.failed(cause); + } + } } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferCallbackAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferCallbackAccumulator.java index 04df7e921ebe..4d24091dea2c 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferCallbackAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferCallbackAccumulator.java @@ -25,7 +25,9 @@ * these into a single {@link ByteBuffer} or byte array and succeed the callbacks.

* *

This class is not thread safe and callers must do mutual exclusion.

+ * @deprecated use {@link ByteBufferAccumulator} */ +@Deprecated public class ByteBufferCallbackAccumulator { private final List _entries = new ArrayList<>(); diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java index 6f78d3300cc5..e0efa79e59a6 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java @@ -428,7 +428,7 @@ public static boolean compact(ByteBuffer buffer) /** * Put data from one buffer into another, avoiding over/under flows * - * @param from Buffer to take bytes from in flush mode + * @param from Buffer to take bytes from in flush mode, whose position is modified with the bytes taken. * @param to Buffer to put bytes to in fill mode. * @return number of bytes moved */ diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8StringBuilder.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8StringBuilder.java index 3da2f322c1c7..c6d8e0d8a640 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8StringBuilder.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8StringBuilder.java @@ -233,7 +233,11 @@ protected void bufferAppend(char c) protected void bufferReset() { + // If the buffer is much larger than necessary, trim it to empty + boolean trim = _buffer.capacity() > (_buffer.length() * 8); _buffer.setLength(0); + if (trim) + _buffer.trimToSize(); } public void appendByte(byte b) throws IOException diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java index e6be28115aae..b7a3055c350b 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java @@ -17,7 +17,7 @@ import java.lang.invoke.MethodType; import java.nio.ByteBuffer; -import org.eclipse.jetty.io.ByteBufferCallbackAccumulator; +import org.eclipse.jetty.io.ByteBufferAccumulator; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.websocket.core.CoreSession; @@ -32,7 +32,7 @@ */ public class ByteArrayMessageSink extends AbstractMessageSink { - private ByteBufferCallbackAccumulator accumulator; + private ByteBufferAccumulator accumulator; /** * Creates a new {@link ByteArrayMessageSink}. @@ -83,8 +83,8 @@ public void accept(Frame frame, Callback callback) } if (accumulator == null) - accumulator = new ByteBufferCallbackAccumulator(); - accumulator.addEntry(payload, callback); + accumulator = new ByteBufferAccumulator(); + accumulator.addBuffer(payload, callback); if (frame.isFin()) { diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java index afcec2dda7ef..19a131b5eb83 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java @@ -17,7 +17,7 @@ import java.lang.invoke.MethodType; import java.nio.ByteBuffer; -import org.eclipse.jetty.io.ByteBufferCallbackAccumulator; +import org.eclipse.jetty.io.ByteBufferAccumulator; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.util.Callback; @@ -33,7 +33,7 @@ */ public class ByteBufferMessageSink extends AbstractMessageSink { - private ByteBufferCallbackAccumulator accumulator; + private ByteBufferAccumulator accumulator; /** * Creates a new {@link ByteBufferMessageSink}. @@ -64,7 +64,7 @@ public void accept(Frame frame, Callback callback) { try { - long size = (accumulator == null ? 0 : accumulator.getLength()) + frame.getPayloadLength(); + int size = Math.addExact(accumulator == null ? 0 : Math.toIntExact(accumulator.getLength()), frame.getPayloadLength()); long maxSize = getCoreSession().getMaxBinaryMessageSize(); if (maxSize > 0 && size > maxSize) { @@ -87,13 +87,13 @@ public void accept(Frame frame, Callback callback) } if (accumulator == null) - accumulator = new ByteBufferCallbackAccumulator(); - accumulator.addEntry(frame.getPayload(), callback); + accumulator = new ByteBufferAccumulator(); + accumulator.addBuffer(frame.getPayload(), callback); if (frame.isFin()) { ByteBufferPool bufferPool = getCoreSession().getByteBufferPool(); - RetainableByteBuffer buffer = bufferPool.acquire(accumulator.getLength(), false); + RetainableByteBuffer buffer = bufferPool.acquire(size, false); ByteBuffer byteBuffer = buffer.getByteBuffer(); accumulator.writeTo(byteBuffer); callback = Callback.from(buffer::release); diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java b/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java index ca91ccfbd521..41cafb9552d2 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java @@ -19,7 +19,7 @@ import java.util.concurrent.BlockingQueue; import java.util.concurrent.TimeUnit; -import org.eclipse.jetty.io.ByteBufferCallbackAccumulator; +import org.eclipse.jetty.io.ByteBufferAccumulator; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.util.BlockingArrayQueue; @@ -113,7 +113,7 @@ public static class ServerHandler implements FrameHandler public BlockingQueue textMessages = new BlockingArrayQueue<>(); public BlockingQueue binaryMessages = new BlockingArrayQueue<>(); private StringBuilder _stringBuilder = new StringBuilder(); - private ByteBufferCallbackAccumulator _byteBuilder = new ByteBufferCallbackAccumulator(); + private ByteBufferAccumulator _byteBuilder = new ByteBufferAccumulator(); @Override public void onOpen(CoreSession coreSession, Callback callback) @@ -154,11 +154,11 @@ public void onFrame(Frame frame, Callback callback) } break; case OpCode.BINARY: - _byteBuilder.addEntry(frame.getPayload(), callback); + _byteBuilder.addBuffer(frame.getPayload(), callback); if (frame.isFin()) { binaryMessages.add(BufferUtil.toBuffer(_byteBuilder.takeByteArray())); - _byteBuilder = new ByteBufferCallbackAccumulator(); + _byteBuilder = new ByteBufferAccumulator(); } break; default: From c70df80b99f66b09f7adf319f8f45839c5565386 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 22 Dec 2023 08:25:52 +1100 Subject: [PATCH 09/43] Review ByteBufferAccumulator #10541 + Added `isRetained()` to `Retainable` + Made `Chunk` is-a `RetainableByteBuffer` + retain rather than copy chunks where possible + Merged aggregator into accumulator + Deprecated ByteBufferAggregator.java + Deprecated ByteBufferCallbackAccumulator --- .../java/org/eclipse/jetty/io/ByteBufferAccumulator.java | 1 + .../websocket/core/messages/ByteBufferMessageSink.java | 8 ++------ 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java index 52226e5d0ab7..a60753165250 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java @@ -95,6 +95,7 @@ public RetainableByteBuffer ensureBuffer(int minSize, int minAllocationSize) /** * Get the last buffer of the accumulator, this can be written to directly to avoid copying into the accumulator. + * Used internally and for testing. * @param minSize the smallest amount of remaining space before a new buffer is allocated. * @param minAllocationSize new buffers will be allocated to have at least this size. * @return a buffer with at least {@code minSize} space to write into. diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java index 19a131b5eb83..8ecff446117d 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java @@ -18,7 +18,6 @@ import java.nio.ByteBuffer; import org.eclipse.jetty.io.ByteBufferAccumulator; -import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.websocket.core.CoreSession; @@ -92,12 +91,9 @@ public void accept(Frame frame, Callback callback) if (frame.isFin()) { - ByteBufferPool bufferPool = getCoreSession().getByteBufferPool(); - RetainableByteBuffer buffer = bufferPool.acquire(size, false); - ByteBuffer byteBuffer = buffer.getByteBuffer(); - accumulator.writeTo(byteBuffer); + RetainableByteBuffer buffer = accumulator.toRetainableByteBuffer(); callback = Callback.from(buffer::release); - invoke(getMethodHandle(), byteBuffer, callback); + invoke(getMethodHandle(), buffer.getByteBuffer(), callback); autoDemand(); } else From 2faa60ed9ade1a2b6cdfe2a4ecbf132867777f63 Mon Sep 17 00:00:00 2001 From: gregw Date: Sun, 24 Dec 2023 11:23:03 +1100 Subject: [PATCH 10/43] Review ByteBufferAccumulator #10541 + Added append and writeTo methods to RetainableByteBuffer + Made Aggregator and Accumulator subtypes of RetainableByteBuffer --- .../eclipse/jetty/io/ArrayByteBufferPool.java | 12 +- .../jetty/io/RetainableByteBuffer.java | 381 ++++++++++++++++++ .../jetty/io/RetainableByteBufferTest.java | 118 ++++++ .../org/eclipse/jetty/util/BufferUtil.java | 52 ++- 4 files changed, 534 insertions(+), 29 deletions(-) create mode 100644 jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java index 92ff52e4210f..dce14aaacc75 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java @@ -602,9 +602,19 @@ public Tracking(int minCapacity, int maxCapacity, int maxBucketSize) this(minCapacity, maxCapacity, maxBucketSize, -1L, -1L); } + public Tracking(int minCapacity, int factor, int maxCapacity, int maxBucketSize) + { + this(minCapacity, factor, maxCapacity, maxBucketSize, 0L, 0L); + } + public Tracking(int minCapacity, int maxCapacity, int maxBucketSize, long maxHeapMemory, long maxDirectMemory) { - super(minCapacity, -1, maxCapacity, maxBucketSize, maxHeapMemory, maxDirectMemory); + this(minCapacity, -1, maxCapacity, maxBucketSize, maxHeapMemory, maxDirectMemory); + } + + public Tracking(int minCapacity, int factor, int maxCapacity, int maxBucketSize, long maxHeapMemory, long maxDirectMemory) + { + super(minCapacity, factor, maxCapacity, maxBucketSize, maxHeapMemory, maxDirectMemory); } @Override diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 2ea813667b57..5cfa9e3d99cb 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -13,10 +13,16 @@ package org.eclipse.jetty.io; +import java.io.IOException; +import java.io.OutputStream; import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.io.internal.NonRetainableByteBuffer; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.Callback; /** *

A pooled {@link ByteBuffer} which maintains a reference count that is @@ -145,6 +151,16 @@ default int capacity() return getByteBuffer().capacity(); } + default int space() + { + return capacity() - remaining(); + } + + default boolean isFull() + { + return remaining() == capacity(); + } + /** * @see BufferUtil#clear(ByteBuffer) */ @@ -153,6 +169,371 @@ default void clear() BufferUtil.clear(getByteBuffer()); } + default int append(byte[] bytes, int offset, int length) + { + ByteBuffer to = getByteBuffer(); + + int pos = BufferUtil.flipToFill(to); + try + { + length = Math.min(length, to.remaining()); + to.put(bytes, offset, length); + } + finally + { + BufferUtil.flipToFlush(to, pos); + } + return length; + } + + default void append(ByteBuffer bytes) + { + BufferUtil.append(getByteBuffer(), bytes); + } + + default boolean append(ByteBuffer bytes, Runnable releaser) + { + append(bytes); + if (bytes.hasRemaining()) + return false; + releaser.run(); + return true; + } + + default void append(RetainableByteBuffer bytes) + { + bytes.writeTo(this); + } + + default void putTo(ByteBuffer toInfillMode) + { + ByteBuffer slice = getByteBuffer().slice(); + toInfillMode.put(slice); + } + + default boolean writeTo(ByteBuffer to) + { + ByteBuffer slice = getByteBuffer().slice(); + BufferUtil.append(to, slice); + return slice.remaining() == 0; + } + + default boolean writeTo(RetainableByteBuffer to) + { + if (remaining() == 0) + return true; + ByteBuffer slice = getByteBuffer().slice(); + to.append(slice); + return slice.remaining() == 0; + } + + default void writeTo(OutputStream out) throws IOException + { + BufferUtil.writeTo(getByteBuffer(), out); + } + + default void writeTo(Content.Sink sink, boolean last, Callback callback) + { + sink.write(last, getByteBuffer(), callback); + } + + class Aggregator implements RetainableByteBuffer + { + private final ByteBufferPool _pool; + private final boolean _direct; + private final int _growBy; + private final int _maxCapacity; + private RetainableByteBuffer _buffer; + + public Aggregator(ByteBufferPool pool, boolean direct, int growBy, int maxCapacity) + { + if (growBy < 0) + throw new IllegalArgumentException("growBy must be > 0"); + if (maxCapacity > 0 && growBy > maxCapacity) + throw new IllegalArgumentException("growBy must be < maxSize"); + + _pool = pool == null ? new ByteBufferPool.NonPooling() : pool; + _direct = direct; + _growBy = growBy; + _maxCapacity = maxCapacity <= 0 ? Integer.MAX_VALUE : maxCapacity; + _buffer = _pool.acquire(_growBy, _direct); + } + + @Override + public ByteBuffer getByteBuffer() + { + return _buffer.getByteBuffer(); + } + + @Override + public int capacity() + { + return Math.max(_buffer.capacity(), _maxCapacity); + } + + @Override + public int append(byte[] bytes, int offset, int length) + { + ensureSpace(length); + return RetainableByteBuffer.super.append(bytes, offset, length); + } + + @Override + public void append(ByteBuffer bytes) + { + ensureSpace(bytes.remaining()); + RetainableByteBuffer.super.append(bytes); + } + + @Override + public boolean append(ByteBuffer bytes, Runnable releaser) + { + ensureSpace(bytes.remaining()); + return RetainableByteBuffer.super.append(bytes, releaser); + } + + @Override + public void append(RetainableByteBuffer bytes) + { + ensureSpace(bytes.remaining()); + RetainableByteBuffer.super.append(bytes); + } + + private void ensureSpace(int spaceNeeded) + { + int capacity = _buffer.capacity(); + int space = capacity - _buffer.remaining(); + if (spaceNeeded <= space || capacity >= _maxCapacity) + return; + + int newCapacity = Math.multiplyExact(1 + Math.addExact(capacity, spaceNeeded) / _growBy, _growBy); + if (newCapacity > _maxCapacity) + { + newCapacity = Math.addExact(capacity, spaceNeeded - space); + if (newCapacity > _maxCapacity) + newCapacity = _maxCapacity; + } + + RetainableByteBuffer ensured = _pool.acquire(newCapacity, _direct); + ensured.append(_buffer); + _buffer.release(); + _buffer = ensured; + } + } + + class Accumulator implements RetainableByteBuffer + { + private final ByteBufferPool _pool; + private final boolean _direct; + private final long _maxLength; + private final List _buffers = new ArrayList<>(); + private boolean _lastAcquiredByUs; + + public Accumulator(ByteBufferPool pool, boolean direct, long maxLength) + { + _pool = pool == null ? new ByteBufferPool.NonPooling() : pool; + _direct = direct; + _maxLength = maxLength < 0 ? Long.MAX_VALUE : maxLength; + } + + @Override + public ByteBuffer getByteBuffer() + { + return switch (_buffers.size()) + { + case 0 -> RetainableByteBuffer.EMPTY.getByteBuffer(); + case 1 -> _buffers.get(0).getByteBuffer(); + default -> + { + int length = remaining(); + RetainableByteBuffer combinedBuffer = _pool.acquire(length, _direct); + ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); + BufferUtil.flipToFill(byteBuffer); + for (RetainableByteBuffer buffer : _buffers) + { + buffer.putTo(byteBuffer); + buffer.clear(); + buffer.release(); + } + BufferUtil.flipToFlush(byteBuffer, 0); + _buffers.clear(); + _buffers.add(combinedBuffer); + _lastAcquiredByUs = true; + yield combinedBuffer.getByteBuffer(); + } + }; + } + + @Override + public int remaining() + { + long length = 0; + for (RetainableByteBuffer buffer : _buffers) + length += buffer.remaining(); + return Math.toIntExact(length); + } + + @Override + public int capacity() + { + return Math.toIntExact(_maxLength); + } + + @Override + public boolean release() + { + clear(); + return true; + } + + @Override + public void clear() + { + for (RetainableByteBuffer buffer : _buffers) + buffer.release(); + _lastAcquiredByUs = false; + _buffers.clear(); + } + + @Override + public int append(byte[] bytes, int offset, int length) + { + length = ensureMaxLength(length); + + if (_lastAcquiredByUs) + { + RetainableByteBuffer last = _buffers.get(_buffers.size() - 1); + if (length <= last.space()) + { + last.append(bytes, offset, length); + return length; + } + } + + RetainableByteBuffer buffer = _pool.acquire(length, _direct); + buffer.append(bytes, offset, length); + _buffers.add(buffer); + _lastAcquiredByUs = true; + return length; + } + + @Override + public void append(ByteBuffer bytes) + { + int remaining = bytes.remaining(); + int length = ensureMaxLength(remaining); + // if the length was restricted by maxLength, slice the bytes smaller + if (length < remaining) + { + ByteBuffer slice = bytes.slice(); + slice.limit(slice.position() + length); + bytes.position(bytes.position() + length); + bytes = slice; + } + RetainableByteBuffer buffer = _pool.acquire(length, _direct); + buffer.append(bytes); + _buffers.add(buffer); + _lastAcquiredByUs = true; + } + + @Override + public boolean append(ByteBuffer bytes, Runnable releaser) + { + int remaining = bytes.remaining(); + int length = ensureMaxLength(remaining); + // if the length was restricted by maxLength, we can't split the releaser so append nothing + if (length < remaining) + return false; + + _buffers.add(new RetainableByteBuffer() + { + private final AtomicReference _releaser = new AtomicReference<>(releaser); + @Override + public ByteBuffer getByteBuffer() + { + return bytes; + } + + @Override + public boolean release() + { + Runnable releaser = _releaser.getAndSet(null); + if (releaser == null) + return false; + releaser.run(); + return true; + } + }); + _lastAcquiredByUs = false; + return true; + } + + @Override + public void append(RetainableByteBuffer bytes) + { + int remaining = bytes.remaining(); + int length = ensureMaxLength(remaining); + // if the length was restricted by maxLength, or we can't retain, then copy into new buffer + if (length < remaining || !bytes.canRetain()) + { + RetainableByteBuffer buffer = _pool.acquire(length, _direct); + bytes.writeTo(buffer); + bytes.clear(); + _buffers.add(buffer); + _lastAcquiredByUs = true; + return; + } + + bytes.retain(); + _buffers.add(bytes); + _lastAcquiredByUs = false; + } + + @Override + public void putTo(ByteBuffer toInfillMode) + { + for (RetainableByteBuffer buffer : _buffers) + buffer.putTo(toInfillMode); + } + + @Override + public boolean writeTo(ByteBuffer to) + { + return RetainableByteBuffer.super.writeTo(to); + } + + @Override + public boolean writeTo(RetainableByteBuffer to) + { + return RetainableByteBuffer.super.writeTo(to); + } + + @Override + public void writeTo(OutputStream out) throws IOException + { + RetainableByteBuffer.super.writeTo(out); + } + + @Override + public void writeTo(Content.Sink sink, boolean last, Callback callback) + { + RetainableByteBuffer.super.writeTo(sink, last, callback); + } + + private int ensureMaxLength(int increment) + { + long length = 0; + for (RetainableByteBuffer buffer : _buffers) + length += buffer.remaining(); + + long newLength = Math.addExact(length, increment); + if (newLength <= _maxLength) + return increment; + + return Math.toIntExact(_maxLength - length); + } + } + /** * A wrapper for {@link RetainableByteBuffer} instances */ diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java new file mode 100644 index 000000000000..6dc18a6882f5 --- /dev/null +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java @@ -0,0 +1,118 @@ +// +// ======================================================================== +// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.io; + +import java.nio.ByteBuffer; +import java.util.Arrays; +import java.util.stream.Stream; + +import org.eclipse.jetty.util.BufferUtil; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class RetainableByteBufferTest +{ + public static final int MIN_CAPACITY = 32; + public static final int MAX_CAPACITY = 64; + private static ByteBufferPool _pool; + + @BeforeAll + public static void beforeAll() + { + _pool = new ArrayByteBufferPool.Tracking(MIN_CAPACITY, MIN_CAPACITY, MAX_CAPACITY, Integer.MAX_VALUE); + } + + static Stream buffers() + { + return Stream.of( + Arguments.of(_pool.acquire(MIN_CAPACITY, true)), + Arguments.of(_pool.acquire(MIN_CAPACITY, false)), + Arguments.of(new RetainableByteBuffer.Aggregator(_pool, true, MIN_CAPACITY, MIN_CAPACITY)), + Arguments.of(new RetainableByteBuffer.Aggregator(_pool, false, MIN_CAPACITY, MIN_CAPACITY)), + Arguments.of(new RetainableByteBuffer.Accumulator(_pool, true, MIN_CAPACITY)), + Arguments.of(new RetainableByteBuffer.Accumulator(_pool, false, MIN_CAPACITY)) + ); + } + + @ParameterizedTest + @MethodSource("buffers") + public void testEmptyBuffer(RetainableByteBuffer buffer) + { + assertThat(buffer.remaining(), is(0)); + assertFalse(buffer.hasRemaining()); + assertThat(buffer.capacity(), greaterThanOrEqualTo(MIN_CAPACITY)); + assertFalse(buffer.isFull()); + + assertThat(buffer.getByteBuffer().remaining(), is(0)); + assertFalse(buffer.getByteBuffer().hasRemaining()); + } + + @ParameterizedTest + @MethodSource("buffers") + public void testAppendOneByte(RetainableByteBuffer buffer) + { + byte[] bytes = new byte[] {'-', 'X', '-'}; + while (!buffer.isFull()) + assertThat(buffer.append(bytes, 1, 1), is(1)); + + assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); + } + + @ParameterizedTest + @MethodSource("buffers") + public void testAppendMoreBytesThanCapacity(RetainableByteBuffer buffer) + { + byte[] bytes = new byte[MAX_CAPACITY * 2]; + Arrays.fill(bytes, (byte)'X'); + assertThat(buffer.append(bytes, 0, bytes.length), is(buffer.capacity())); + + assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); + assertTrue(buffer.isFull()); + } + + @ParameterizedTest + @MethodSource("buffers") + public void testAppendSmallByteBuffer(RetainableByteBuffer buffer) + { + byte[] bytes = new byte[] {'-', 'X', '-'}; + ByteBuffer from = ByteBuffer.wrap(bytes, 1, 1); + while (!buffer.isFull()) + { + ByteBuffer slice = from.slice(); + buffer.append(slice); + assertFalse(slice.hasRemaining()); + } + + assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); + } + + @ParameterizedTest + @MethodSource("buffers") + public void testAppendBigByteBuffer(RetainableByteBuffer buffer) + { + ByteBuffer from = BufferUtil.toBuffer("X".repeat(buffer.capacity() * 2)); + buffer.append(from); + assertTrue(from.hasRemaining()); + assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); + assertTrue(buffer.isFull()); + } +} diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java index e0efa79e59a6..4363e5891e9f 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java @@ -434,35 +434,31 @@ public static boolean compact(ByteBuffer buffer) */ public static int put(ByteBuffer from, ByteBuffer to) { - int put; - int remaining = from.remaining(); - if (remaining > 0) + int length = from.remaining(); + if (length == 0) + return 0; + + int space = to.remaining(); + if (space >= length) { - if (remaining <= to.remaining()) - { - to.put(from); - put = remaining; - from.position(from.limit()); - } - else if (from.hasArray()) - { - put = to.remaining(); - to.put(from.array(), from.arrayOffset() + from.position(), put); - from.position(from.position() + put); - } - else - { - put = to.remaining(); - ByteBuffer slice = from.slice(); - slice.limit(put); - to.put(slice); - from.position(from.position() + put); - } + to.put(from); + return length; + } + + if (to.hasArray()) + { + to.put(from.array(), from.arrayOffset() + from.position(), space); + from.position(from.position() + space); } else - put = 0; + { + ByteBuffer slice = from.slice(); + slice.limit(slice.position() + space); + to.put(slice); + from.position(from.position() + space); + } - return put; + return space; } /** @@ -535,9 +531,9 @@ public static void append(ByteBuffer to, byte b) /** * Appends a buffer to a buffer * - * @param to Buffer is flush mode - * @param b buffer to append - * @return The position of the valid data before the flipped position. + * @param to Buffer in flush mode, whose position will be incremented by the number of bytes appended + * @param b buffer to append to in flush mode, whose limit will be incremented by the number of bytes appended. + * @return The number of bytes appended. */ public static int append(ByteBuffer to, ByteBuffer b) { From aeb164a17c0389996f1cf92a94de77939f7908fc Mon Sep 17 00:00:00 2001 From: gregw Date: Sun, 24 Dec 2023 15:26:55 +1100 Subject: [PATCH 11/43] Review ByteBufferAccumulator #10541 + Added append and writeTo methods to RetainableByteBuffer + Made Aggregator and Accumulator subtypes of RetainableByteBuffer --- .../jetty/io/ByteBufferAccumulator.java | 2 + .../jetty/io/RetainableByteBuffer.java | 39 +++++- .../jetty/io/content/BufferedContentSink.java | 111 +++--------------- .../jetty/io/RetainableByteBufferTest.java | 8 +- 4 files changed, 61 insertions(+), 99 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java index a60753165250..054deb4c999c 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java @@ -30,7 +30,9 @@ *

* The method {@link #accessInternalBuffer(int, int)} can be used access a buffer that can be written to directly as the last buffer list, * if there is less than a certain amount of space available in that buffer then a new one will be allocated and returned instead. + * @deprecated use {@link RetainableByteBuffer.Accumulator} */ +@Deprecated public class ByteBufferAccumulator implements AutoCloseable { private final List _buffers = new ArrayList<>(); diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 5cfa9e3d99cb..0a0c13d7bb65 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -23,6 +23,7 @@ import org.eclipse.jetty.io.internal.NonRetainableByteBuffer; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.IteratingCallback; /** *

A pooled {@link ByteBuffer} which maintains a reference count that is @@ -237,6 +238,11 @@ default void writeTo(Content.Sink sink, boolean last, Callback callback) sink.write(last, getByteBuffer(), callback); } + static RetainableByteBuffer newAggregator(ByteBufferPool pool, boolean direct, int growBy, int maxCapacity) + { + return new Aggregator(pool, direct, growBy, maxCapacity); + } + class Aggregator implements RetainableByteBuffer { private final ByteBufferPool _pool; @@ -245,7 +251,7 @@ class Aggregator implements RetainableByteBuffer private final int _maxCapacity; private RetainableByteBuffer _buffer; - public Aggregator(ByteBufferPool pool, boolean direct, int growBy, int maxCapacity) + private Aggregator(ByteBufferPool pool, boolean direct, int growBy, int maxCapacity) { if (growBy < 0) throw new IllegalArgumentException("growBy must be > 0"); @@ -321,6 +327,11 @@ private void ensureSpace(int spaceNeeded) } } + static RetainableByteBuffer newAccumulator(ByteBufferPool bufferPool, boolean direct, int maxLength) + { + return new Accumulator(bufferPool, direct, maxLength); + } + class Accumulator implements RetainableByteBuffer { private final ByteBufferPool _pool; @@ -329,7 +340,7 @@ class Accumulator implements RetainableByteBuffer private final List _buffers = new ArrayList<>(); private boolean _lastAcquiredByUs; - public Accumulator(ByteBufferPool pool, boolean direct, long maxLength) + private Accumulator(ByteBufferPool pool, boolean direct, long maxLength) { _pool = pool == null ? new ByteBufferPool.NonPooling() : pool; _direct = direct; @@ -499,25 +510,47 @@ public void putTo(ByteBuffer toInfillMode) @Override public boolean writeTo(ByteBuffer to) { + // TODO return RetainableByteBuffer.super.writeTo(to); } @Override public boolean writeTo(RetainableByteBuffer to) { + // TODO return RetainableByteBuffer.super.writeTo(to); } @Override public void writeTo(OutputStream out) throws IOException { + // TODO RetainableByteBuffer.super.writeTo(out); } @Override public void writeTo(Content.Sink sink, boolean last, Callback callback) { - RetainableByteBuffer.super.writeTo(sink, last, callback); + switch (_buffers.size()) + { + case 0 -> callback.succeeded(); + case 1 -> _buffers.get(0).writeTo(sink, last, callback); + default -> new IteratingCallback() + { + private int i = 0; + + @Override + protected Action process() + { + if (i < _buffers.size()) + { + _buffers.get(i).writeTo(sink, ++i == _buffers.size() || !last, this); + return Action.SCHEDULED; + } + return Action.SUCCEEDED; + } + }.iterate(); + } } private int ensureMaxLength(int increment) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java index 4f48cd208ce5..d88aaf2b9b6f 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java @@ -15,15 +15,12 @@ import java.io.IOException; import java.nio.ByteBuffer; -import java.nio.channels.WritePendingException; -import org.eclipse.jetty.io.ByteBufferAccumulator; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; -import org.eclipse.jetty.util.IteratingCallback; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -48,8 +45,7 @@ public class BufferedContentSink implements Content.Sink private final boolean _direct; private final int _maxBufferSize; private final int _maxAggregationSize; - private final Flusher _flusher; - private ByteBufferAccumulator _accumulator; + private RetainableByteBuffer _accumulator; private boolean _firstWrite = true; private boolean _lastWritten; @@ -66,7 +62,6 @@ public BufferedContentSink(Content.Sink delegate, ByteBufferPool bufferPool, boo _direct = direct; _maxBufferSize = maxBufferSize; _maxAggregationSize = maxAggregationSize; - _flusher = new Flusher(delegate); } @Override @@ -97,7 +92,7 @@ public void write(boolean last, ByteBuffer byteBuffer, Callback callback) { // current buffer can be aggregated if (_accumulator == null) - _accumulator = new ByteBufferAccumulator(_bufferPool, _direct, _maxBufferSize); + _accumulator = RetainableByteBuffer.newAccumulator(_bufferPool, _direct, _maxBufferSize); aggregateAndFlush(last, current, callback); } else @@ -125,18 +120,18 @@ private void flush(boolean last, ByteBuffer currentBuffer, Callback callback) if (LOG.isDebugEnabled()) LOG.debug("given buffer is greater than _maxBufferSize"); - RetainableByteBuffer aggregatedBuffer = _accumulator == null ? null : _accumulator.takeRetainableByteBuffer(); - if (aggregatedBuffer == null) + if (_accumulator == null) { if (LOG.isDebugEnabled()) LOG.debug("nothing aggregated, flushing current buffer {}", currentBuffer); - _flusher.offer(last, currentBuffer, callback); + _delegate.write(last, currentBuffer, callback); } else if (BufferUtil.hasContent(currentBuffer)) { if (LOG.isDebugEnabled()) - LOG.debug("flushing aggregated buffer {}", aggregatedBuffer); - _flusher.offer(false, aggregatedBuffer.getByteBuffer(), new Callback.Nested(Callback.from(aggregatedBuffer::release)) + LOG.debug("flushing aggregated buffer {}", _accumulator); + + _accumulator.writeTo(_delegate, false, new Callback.Nested(Callback.from(_accumulator::release)) { @Override public void succeeded() @@ -144,7 +139,7 @@ public void succeeded() super.succeeded(); if (LOG.isDebugEnabled()) LOG.debug("succeeded writing aggregated buffer, flushing current buffer {}", currentBuffer); - _flusher.offer(last, currentBuffer, callback); + _delegate.write(last, currentBuffer, callback); } @Override @@ -159,7 +154,7 @@ public void failed(Throwable x) } else { - _flusher.offer(false, aggregatedBuffer.getByteBuffer(), Callback.from(aggregatedBuffer::release, callback)); + _accumulator.writeTo(_delegate, last, Callback.from(_accumulator::release, callback)); } } @@ -168,7 +163,8 @@ public void failed(Throwable x) */ private void aggregateAndFlush(boolean last, ByteBuffer currentBuffer, Callback callback) { - boolean full = _accumulator.aggregate(currentBuffer); + _accumulator.append(currentBuffer); + boolean full = _accumulator.isFull(); boolean empty = !currentBuffer.hasRemaining(); boolean flush = full || currentBuffer == FLUSH_BUFFER; boolean complete = last && empty; @@ -176,29 +172,27 @@ private void aggregateAndFlush(boolean last, ByteBuffer currentBuffer, Callback LOG.debug("aggregated current buffer, full={}, complete={}, bytes left={}, aggregator={}", full, complete, currentBuffer.remaining(), _accumulator); if (complete) { - RetainableByteBuffer aggregatedBuffer = _accumulator.takeRetainableByteBuffer(); - if (aggregatedBuffer != null) + if (_accumulator != null) { if (LOG.isDebugEnabled()) - LOG.debug("complete; writing aggregated buffer as the last one: {} bytes", aggregatedBuffer.remaining()); - _flusher.offer(true, aggregatedBuffer.getByteBuffer(), Callback.from(callback, aggregatedBuffer::release)); + LOG.debug("complete; writing aggregated buffer as the last one: {} bytes", _accumulator.remaining()); + _accumulator.writeTo(_delegate, true, Callback.from(callback, _accumulator::release)); } else { if (LOG.isDebugEnabled()) LOG.debug("complete; no aggregated buffer, writing last empty buffer"); - _flusher.offer(true, BufferUtil.EMPTY_BUFFER, callback); + _delegate.write(true, BufferUtil.EMPTY_BUFFER, callback); } } else if (flush) { - RetainableByteBuffer aggregatedBuffer = _accumulator.takeRetainableByteBuffer(); if (LOG.isDebugEnabled()) - LOG.debug("writing aggregated buffer: {} bytes, then {}", aggregatedBuffer.remaining(), currentBuffer.remaining()); + LOG.debug("writing aggregated buffer: {} bytes, then {}", _accumulator.remaining(), currentBuffer.remaining()); if (BufferUtil.hasContent(currentBuffer)) { - _flusher.offer(false, aggregatedBuffer.getByteBuffer(), new Callback.Nested(Callback.from(aggregatedBuffer::release)) + _accumulator.writeTo(_delegate, false, new Callback.Nested(Callback.from(_accumulator::release)) { @Override public void succeeded() @@ -207,7 +201,7 @@ public void succeeded() if (LOG.isDebugEnabled()) LOG.debug("written aggregated buffer, writing remaining of current: {} bytes{}", currentBuffer.remaining(), (last ? " (last write)" : "")); if (last) - _flusher.offer(true, currentBuffer, callback); + _delegate.write(true, currentBuffer, callback); else aggregateAndFlush(false, currentBuffer, callback); } @@ -224,81 +218,14 @@ public void failed(Throwable x) } else { - _flusher.offer(false, aggregatedBuffer.getByteBuffer(), Callback.from(aggregatedBuffer::release, callback)); + _accumulator.writeTo(_delegate, last, Callback.from(_accumulator::release, callback)); } } else { if (LOG.isDebugEnabled()) LOG.debug("buffer fully aggregated, delaying writing - aggregator: {}", _accumulator); - _flusher.offer(callback); - } - } - - private static class Flusher extends IteratingCallback - { - private static final ByteBuffer COMPLETE_CALLBACK = BufferUtil.allocate(0); - - private final Content.Sink _sink; - private boolean _last; - private ByteBuffer _buffer; - private Callback _callback; - private boolean _lastWritten; - - Flusher(Content.Sink sink) - { - _sink = sink; - } - - void offer(Callback callback) - { - offer(false, COMPLETE_CALLBACK, callback); - } - - void offer(boolean last, ByteBuffer byteBuffer, Callback callback) - { - if (_callback != null) - throw new WritePendingException(); - _last = last; - _buffer = byteBuffer; - _callback = callback; - iterate(); - } - - @Override - protected Action process() - { - if (_lastWritten) - return Action.SUCCEEDED; - if (_callback == null) - return Action.IDLE; - if (_buffer != COMPLETE_CALLBACK) - { - _lastWritten = _last; - _sink.write(_last, _buffer, this); - } - else - { - succeeded(); - } - return Action.SCHEDULED; - } - - @Override - public void succeeded() - { - _buffer = null; - Callback callback = _callback; - _callback = null; callback.succeeded(); - super.succeeded(); - } - - @Override - protected void onCompleteFailure(Throwable cause) - { - _buffer = null; - _callback.failed(cause); } } } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java index 6dc18a6882f5..c8bf3c0df71d 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java @@ -46,10 +46,10 @@ static Stream buffers() return Stream.of( Arguments.of(_pool.acquire(MIN_CAPACITY, true)), Arguments.of(_pool.acquire(MIN_CAPACITY, false)), - Arguments.of(new RetainableByteBuffer.Aggregator(_pool, true, MIN_CAPACITY, MIN_CAPACITY)), - Arguments.of(new RetainableByteBuffer.Aggregator(_pool, false, MIN_CAPACITY, MIN_CAPACITY)), - Arguments.of(new RetainableByteBuffer.Accumulator(_pool, true, MIN_CAPACITY)), - Arguments.of(new RetainableByteBuffer.Accumulator(_pool, false, MIN_CAPACITY)) + Arguments.of(RetainableByteBuffer.newAggregator(_pool, true, MIN_CAPACITY, MIN_CAPACITY)), + Arguments.of(RetainableByteBuffer.newAggregator(_pool, false, MIN_CAPACITY, MIN_CAPACITY)), + Arguments.of(RetainableByteBuffer.newAccumulator(_pool, true, MIN_CAPACITY)), + Arguments.of(RetainableByteBuffer.newAccumulator(_pool, false, MIN_CAPACITY)) ); } From 158b108a180eada58e500bea7971e71c7ab2495f Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 16 Jan 2024 18:34:19 +1100 Subject: [PATCH 12/43] Review ByteBufferAccumulator #10541 + Added append and writeTo methods to RetainableByteBuffer + Made Aggregator and Accumulator subtypes of RetainableByteBuffer --- .../jetty/io/RetainableByteBuffer.java | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 0a0c13d7bb65..1353eb8acc34 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -327,6 +327,14 @@ private void ensureSpace(int spaceNeeded) } } + /** + * Obtain a new accumulating {@link RetainableByteBuffer} that may internally accumulate multiple other + * {@link RetainableByteBuffer}s with zero-copy if the {@link #append(RetainableByteBuffer)} API is used + * @param bufferPool The pool from which to allocate buffers + * @param direct true if direct buffers should be used + * @param maxLength The maximum length of the accumulated buffers or -1 for no limit + * @return The accumulating {@link RetainableByteBuffer} + */ static RetainableByteBuffer newAccumulator(ByteBufferPool bufferPool, boolean direct, int maxLength) { return new Accumulator(bufferPool, direct, maxLength); @@ -510,22 +518,30 @@ public void putTo(ByteBuffer toInfillMode) @Override public boolean writeTo(ByteBuffer to) { - // TODO - return RetainableByteBuffer.super.writeTo(to); + for (RetainableByteBuffer buffer : _buffers) + { + if (!buffer.writeTo(to)) + return false; + } + return true; } @Override public boolean writeTo(RetainableByteBuffer to) { - // TODO - return RetainableByteBuffer.super.writeTo(to); + for (RetainableByteBuffer buffer : _buffers) + { + if (!buffer.writeTo(to)) + return false; + } + return true; } @Override public void writeTo(OutputStream out) throws IOException { - // TODO - RetainableByteBuffer.super.writeTo(out); + for (RetainableByteBuffer buffer : _buffers) + buffer.writeTo(out); } @Override From c68da104c1bb0bbbcfb6404d456d5a82019bc5e3 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 17 Jan 2024 15:02:48 +1100 Subject: [PATCH 13/43] Review ByteBufferAccumulator #10541 WIP on review feedback --- .../jetty/io/RetainableByteBuffer.java | 260 +++++++++--------- .../jetty/io/content/AsyncContent.java | 4 +- .../jetty/io/content/BufferedContentSink.java | 55 ++-- .../jetty/io/BufferedContentSinkTest.java | 6 + .../jetty/io/RetainableByteBufferTest.java | 22 +- .../org/eclipse/jetty/util/BufferUtil.java | 6 + 6 files changed, 196 insertions(+), 157 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 1353eb8acc34..02954b1a7d15 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -18,7 +18,6 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.io.internal.NonRetainableByteBuffer; import org.eclipse.jetty.util.BufferUtil; @@ -114,6 +113,29 @@ public boolean release() }; } + /** + *

Returns a {@code RetainableByteBuffer} that wraps + * the given {@code ByteBuffer} and {@link Runnable} releaser.

+ * + * @param byteBuffer the {@code ByteBuffer} to wrap + * @param releaser a {@link Runnable} to call when the buffer is released. + * @return a {@code RetainableByteBuffer} + */ + static RetainableByteBuffer wrap(ByteBuffer byteBuffer, Runnable releaser) + { + return new AbstractRetainableByteBuffer(byteBuffer) + { + @Override + public boolean release() + { + boolean released = super.release(); + if (released) + releaser.run(); + return released; + } + }; + } + /** * Get the wrapped, not {@code null}, {@code ByteBuffer}. * @return the wrapped, not {@code null}, {@code ByteBuffer} @@ -170,6 +192,12 @@ default void clear() BufferUtil.clear(getByteBuffer()); } + /** Append byte array to the buffer, potentially limited by capacity. + * @param bytes the byte array to append + * @param offset the offset into the array + * @param length the number of bytes to try to append + * @return the number of bytes actually appended. + */ default int append(byte[] bytes, int offset, int length) { ByteBuffer to = getByteBuffer(); @@ -187,45 +215,20 @@ default int append(byte[] bytes, int offset, int length) return length; } - default void append(ByteBuffer bytes) + default boolean append(ByteBuffer bytes) { BufferUtil.append(getByteBuffer(), bytes); + return !bytes.hasRemaining(); } - default boolean append(ByteBuffer bytes, Runnable releaser) - { - append(bytes); - if (bytes.hasRemaining()) - return false; - releaser.run(); - return true; - } - - default void append(RetainableByteBuffer bytes) + default boolean append(RetainableByteBuffer bytes) { - bytes.writeTo(this); + return bytes.remaining() == 0 || append(bytes.getByteBuffer()); } default void putTo(ByteBuffer toInfillMode) { - ByteBuffer slice = getByteBuffer().slice(); - toInfillMode.put(slice); - } - - default boolean writeTo(ByteBuffer to) - { - ByteBuffer slice = getByteBuffer().slice(); - BufferUtil.append(to, slice); - return slice.remaining() == 0; - } - - default boolean writeTo(RetainableByteBuffer to) - { - if (remaining() == 0) - return true; - ByteBuffer slice = getByteBuffer().slice(); - to.append(slice); - return slice.remaining() == 0; + toInfillMode.put(getByteBuffer()); } default void writeTo(OutputStream out) throws IOException @@ -238,11 +241,9 @@ default void writeTo(Content.Sink sink, boolean last, Callback callback) sink.write(last, getByteBuffer(), callback); } - static RetainableByteBuffer newAggregator(ByteBufferPool pool, boolean direct, int growBy, int maxCapacity) - { - return new Aggregator(pool, direct, growBy, maxCapacity); - } - + /** + * An aggregating {@link RetainableByteBuffer} that may grow when content is appended to it. + */ class Aggregator implements RetainableByteBuffer { private final ByteBufferPool _pool; @@ -251,18 +252,71 @@ class Aggregator implements RetainableByteBuffer private final int _maxCapacity; private RetainableByteBuffer _buffer; - private Aggregator(ByteBufferPool pool, boolean direct, int growBy, int maxCapacity) + /** + * Construct an aggregating {@link RetainableByteBuffer} that may grow when content is appended to it. + * {@link RetainableByteBuffer}s with zero-copy if the {@link #append(RetainableByteBuffer)} API is used + * @param pool The pool from which to allocate buffers + * @param direct true if direct buffers should be used + * @param maxCapacity The maximum requested length of the accumulated buffers or -1 for no limit. + * Note that the pool may provide a buffer that exceeds this capacity. + */ + public Aggregator(ByteBufferPool pool, boolean direct, int maxCapacity) + { + this(pool, direct, -1, maxCapacity); + } + + /** + * Construct an aggregating {@link RetainableByteBuffer} that may grow when content is appended to it. + * {@link RetainableByteBuffer}s with zero-copy if the {@link #append(RetainableByteBuffer)} API is used + * @param pool The pool from which to allocate buffers + * @param direct true if direct buffers should be used + * @param growBy the size to grow the buffer by or <= 0 for a heuristic + * @param maxCapacity The maximum requested length of the accumulated buffers or -1 for no limit. + * Note that the pool may provide a buffer that exceeds this capacity. + */ + public Aggregator(ByteBufferPool pool, boolean direct, int growBy, int maxCapacity) { - if (growBy < 0) - throw new IllegalArgumentException("growBy must be > 0"); - if (maxCapacity > 0 && growBy > maxCapacity) - throw new IllegalArgumentException("growBy must be < maxSize"); - _pool = pool == null ? new ByteBufferPool.NonPooling() : pool; _direct = direct; - _growBy = growBy; _maxCapacity = maxCapacity <= 0 ? Integer.MAX_VALUE : maxCapacity; - _buffer = _pool.acquire(_growBy, _direct); + + if (growBy <= 0) + { + _buffer = _pool.acquire(Math.min(1024, _maxCapacity), _direct); + _growBy = Math.min(_maxCapacity, _buffer.capacity()); + } + else + { + if (growBy > _maxCapacity) + throw new IllegalArgumentException("growBy(%d) must be <= maxCapacity(%d)".formatted(growBy, _maxCapacity)); + + _growBy = growBy; + _buffer = _pool.acquire(growBy, _direct); + } + } + + @Override + public boolean canRetain() + { + return _buffer.canRetain(); + } + + @Override + public boolean isRetained() + { + return _buffer.isRetained(); + } + + @Override + public void retain() + { + _buffer.retain(); + } + + @Override + public boolean release() + { + return _buffer.release(); } @Override @@ -285,24 +339,17 @@ public int append(byte[] bytes, int offset, int length) } @Override - public void append(ByteBuffer bytes) + public boolean append(ByteBuffer bytes) { ensureSpace(bytes.remaining()); - RetainableByteBuffer.super.append(bytes); + return RetainableByteBuffer.super.append(bytes); } @Override - public boolean append(ByteBuffer bytes, Runnable releaser) + public boolean append(RetainableByteBuffer bytes) { ensureSpace(bytes.remaining()); - return RetainableByteBuffer.super.append(bytes, releaser); - } - - @Override - public void append(RetainableByteBuffer bytes) - { - ensureSpace(bytes.remaining()); - RetainableByteBuffer.super.append(bytes); + return RetainableByteBuffer.super.append(bytes); } private void ensureSpace(int spaceNeeded) @@ -328,27 +375,25 @@ private void ensureSpace(int spaceNeeded) } /** - * Obtain a new accumulating {@link RetainableByteBuffer} that may internally accumulate multiple other + * An accumulating {@link RetainableByteBuffer} that may internally accumulate multiple other * {@link RetainableByteBuffer}s with zero-copy if the {@link #append(RetainableByteBuffer)} API is used - * @param bufferPool The pool from which to allocate buffers - * @param direct true if direct buffers should be used - * @param maxLength The maximum length of the accumulated buffers or -1 for no limit - * @return The accumulating {@link RetainableByteBuffer} */ - static RetainableByteBuffer newAccumulator(ByteBufferPool bufferPool, boolean direct, int maxLength) - { - return new Accumulator(bufferPool, direct, maxLength); - } - class Accumulator implements RetainableByteBuffer { private final ByteBufferPool _pool; private final boolean _direct; private final long _maxLength; private final List _buffers = new ArrayList<>(); - private boolean _lastAcquiredByUs; + private boolean _canAggregate; - private Accumulator(ByteBufferPool pool, boolean direct, long maxLength) + /** + * Construct an accumulating {@link RetainableByteBuffer} that may internally accumulate multiple other + * {@link RetainableByteBuffer}s with zero-copy if the {@link #append(RetainableByteBuffer)} API is used + * @param pool The pool from which to allocate buffers + * @param direct true if direct buffers should be used + * @param maxLength The maximum length of the accumulated buffers or -1 for no limit + */ + public Accumulator(ByteBufferPool pool, boolean direct, long maxLength) { _pool = pool == null ? new ByteBufferPool.NonPooling() : pool; _direct = direct; @@ -377,7 +422,7 @@ public ByteBuffer getByteBuffer() BufferUtil.flipToFlush(byteBuffer, 0); _buffers.clear(); _buffers.add(combinedBuffer); - _lastAcquiredByUs = true; + _canAggregate = true; yield combinedBuffer.getByteBuffer(); } }; @@ -410,7 +455,7 @@ public void clear() { for (RetainableByteBuffer buffer : _buffers) buffer.release(); - _lastAcquiredByUs = false; + _canAggregate = false; _buffers.clear(); } @@ -419,7 +464,7 @@ public int append(byte[] bytes, int offset, int length) { length = ensureMaxLength(length); - if (_lastAcquiredByUs) + if (_canAggregate) { RetainableByteBuffer last = _buffers.get(_buffers.size() - 1); if (length <= last.space()) @@ -432,12 +477,12 @@ public int append(byte[] bytes, int offset, int length) RetainableByteBuffer buffer = _pool.acquire(length, _direct); buffer.append(bytes, offset, length); _buffers.add(buffer); - _lastAcquiredByUs = true; + _canAggregate = true; return length; } @Override - public void append(ByteBuffer bytes) + public boolean append(ByteBuffer bytes) { int remaining = bytes.remaining(); int length = ensureMaxLength(remaining); @@ -452,60 +497,41 @@ public void append(ByteBuffer bytes) RetainableByteBuffer buffer = _pool.acquire(length, _direct); buffer.append(bytes); _buffers.add(buffer); - _lastAcquiredByUs = true; + _canAggregate = true; + return !bytes.hasRemaining(); } @Override - public boolean append(ByteBuffer bytes, Runnable releaser) + public boolean append(RetainableByteBuffer bytes) { int remaining = bytes.remaining(); int length = ensureMaxLength(remaining); - // if the length was restricted by maxLength, we can't split the releaser so append nothing - if (length < remaining) - return false; - _buffers.add(new RetainableByteBuffer() + // If we cannot retain, then try aggregation into the last buffer + if (!bytes.canRetain() && _canAggregate) { - private final AtomicReference _releaser = new AtomicReference<>(releaser); - @Override - public ByteBuffer getByteBuffer() - { - return bytes; - } - - @Override - public boolean release() - { - Runnable releaser = _releaser.getAndSet(null); - if (releaser == null) - return false; - releaser.run(); + if (_buffers.get(_buffers.size() - 1).append(bytes)) return true; - } - }); - _lastAcquiredByUs = false; - return true; - } + length -= remaining - bytes.remaining(); + remaining = bytes.remaining(); + } - @Override - public void append(RetainableByteBuffer bytes) - { - int remaining = bytes.remaining(); - int length = ensureMaxLength(remaining); // if the length was restricted by maxLength, or we can't retain, then copy into new buffer if (length < remaining || !bytes.canRetain()) { + RetainableByteBuffer buffer = _pool.acquire(length, _direct); - bytes.writeTo(buffer); + buffer.append(bytes); bytes.clear(); _buffers.add(buffer); - _lastAcquiredByUs = true; - return; + _canAggregate = true; + return false; } bytes.retain(); _buffers.add(bytes); - _lastAcquiredByUs = false; + _canAggregate = false; + return true; } @Override @@ -515,28 +541,6 @@ public void putTo(ByteBuffer toInfillMode) buffer.putTo(toInfillMode); } - @Override - public boolean writeTo(ByteBuffer to) - { - for (RetainableByteBuffer buffer : _buffers) - { - if (!buffer.writeTo(to)) - return false; - } - return true; - } - - @Override - public boolean writeTo(RetainableByteBuffer to) - { - for (RetainableByteBuffer buffer : _buffers) - { - if (!buffer.writeTo(to)) - return false; - } - return true; - } - @Override public void writeTo(OutputStream out) throws IOException { diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java index db377b852ed0..26932ec66dfe 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java @@ -304,7 +304,7 @@ public boolean canRetain() @Override public boolean isRetained() { - return referenceCounter.isRetained(); + return canRetain() && referenceCounter.isRetained(); } @Override @@ -317,6 +317,8 @@ public void retain() @Override public boolean release() { + if (hasRemaining()) + BufferUtil.clear(getByteBuffer()); if (!canRetain()) return true; boolean released = referenceCounter.release(); diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java index d88aaf2b9b6f..5d911b94de52 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java @@ -45,7 +45,7 @@ public class BufferedContentSink implements Content.Sink private final boolean _direct; private final int _maxBufferSize; private final int _maxAggregationSize; - private RetainableByteBuffer _accumulator; + private RetainableByteBuffer _aggregator; private boolean _firstWrite = true; private boolean _lastWritten; @@ -63,6 +63,12 @@ public BufferedContentSink(Content.Sink delegate, ByteBufferPool bufferPool, boo _maxBufferSize = maxBufferSize; _maxAggregationSize = maxAggregationSize; } + + private void releaseAggregator() + { + _aggregator.release(); + _aggregator = null; + } @Override public void write(boolean last, ByteBuffer byteBuffer, Callback callback) @@ -91,8 +97,8 @@ public void write(boolean last, ByteBuffer byteBuffer, Callback callback) if (current.remaining() <= _maxAggregationSize) { // current buffer can be aggregated - if (_accumulator == null) - _accumulator = RetainableByteBuffer.newAccumulator(_bufferPool, _direct, _maxBufferSize); + if (_aggregator == null) + _aggregator = new RetainableByteBuffer.Aggregator(_bufferPool, _direct, _maxBufferSize); aggregateAndFlush(last, current, callback); } else @@ -120,7 +126,7 @@ private void flush(boolean last, ByteBuffer currentBuffer, Callback callback) if (LOG.isDebugEnabled()) LOG.debug("given buffer is greater than _maxBufferSize"); - if (_accumulator == null) + if (_aggregator == null) { if (LOG.isDebugEnabled()) LOG.debug("nothing aggregated, flushing current buffer {}", currentBuffer); @@ -129,9 +135,9 @@ private void flush(boolean last, ByteBuffer currentBuffer, Callback callback) else if (BufferUtil.hasContent(currentBuffer)) { if (LOG.isDebugEnabled()) - LOG.debug("flushing aggregated buffer {}", _accumulator); + LOG.debug("flushing aggregated buffer {}", _aggregator); - _accumulator.writeTo(_delegate, false, new Callback.Nested(Callback.from(_accumulator::release)) + _aggregator.writeTo(_delegate, false, new Callback.Nested(Callback.from(this::releaseAggregator)) { @Override public void succeeded() @@ -154,7 +160,7 @@ public void failed(Throwable x) } else { - _accumulator.writeTo(_delegate, last, Callback.from(_accumulator::release, callback)); + _aggregator.writeTo(_delegate, last, last ? Callback.from(this::releaseAggregator, callback) : callback); } } @@ -163,20 +169,20 @@ public void failed(Throwable x) */ private void aggregateAndFlush(boolean last, ByteBuffer currentBuffer, Callback callback) { - _accumulator.append(currentBuffer); - boolean full = _accumulator.isFull(); + _aggregator.append(currentBuffer); + boolean full = _aggregator.isFull(); boolean empty = !currentBuffer.hasRemaining(); boolean flush = full || currentBuffer == FLUSH_BUFFER; boolean complete = last && empty; if (LOG.isDebugEnabled()) - LOG.debug("aggregated current buffer, full={}, complete={}, bytes left={}, aggregator={}", full, complete, currentBuffer.remaining(), _accumulator); + LOG.debug("aggregated current buffer, full={}, complete={}, bytes left={}, aggregator={}", full, complete, currentBuffer.remaining(), _aggregator); if (complete) { - if (_accumulator != null) + if (_aggregator != null) { if (LOG.isDebugEnabled()) - LOG.debug("complete; writing aggregated buffer as the last one: {} bytes", _accumulator.remaining()); - _accumulator.writeTo(_delegate, true, Callback.from(callback, _accumulator::release)); + LOG.debug("complete; writing aggregated buffer as the last one: {} bytes", _aggregator.remaining()); + _aggregator.writeTo(_delegate, true, Callback.from(this::releaseAggregator, callback)); } else { @@ -188,22 +194,30 @@ private void aggregateAndFlush(boolean last, ByteBuffer currentBuffer, Callback else if (flush) { if (LOG.isDebugEnabled()) - LOG.debug("writing aggregated buffer: {} bytes, then {}", _accumulator.remaining(), currentBuffer.remaining()); + LOG.debug("writing aggregated buffer: {} bytes, then {}", _aggregator.remaining(), currentBuffer.remaining()); - if (BufferUtil.hasContent(currentBuffer)) + if (BufferUtil.isEmpty(currentBuffer)) + { + _aggregator.writeTo(_delegate, last, last ? Callback.from(this::releaseAggregator, callback) : callback); + } + else { - _accumulator.writeTo(_delegate, false, new Callback.Nested(Callback.from(_accumulator::release)) + _aggregator.writeTo(_delegate, false, new Callback() { @Override public void succeeded() { - super.succeeded(); if (LOG.isDebugEnabled()) LOG.debug("written aggregated buffer, writing remaining of current: {} bytes{}", currentBuffer.remaining(), (last ? " (last write)" : "")); if (last) + { + releaseAggregator(); _delegate.write(true, currentBuffer, callback); + } else + { aggregateAndFlush(false, currentBuffer, callback); + } } @Override @@ -211,20 +225,15 @@ public void failed(Throwable x) { if (LOG.isDebugEnabled()) LOG.debug("failure writing aggregated buffer", x); - super.failed(x); callback.failed(x); } }); } - else - { - _accumulator.writeTo(_delegate, last, Callback.from(_accumulator::release, callback)); - } } else { if (LOG.isDebugEnabled()) - LOG.debug("buffer fully aggregated, delaying writing - aggregator: {}", _accumulator); + LOG.debug("buffer fully aggregated, delaying writing - aggregator: {}", _aggregator); callback.succeeded(); } } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java index 0731eff5d32b..43129edab0fa 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java @@ -270,6 +270,12 @@ public void testFlush(BiConsumer flusher) throws assertThat(BufferUtil.toString(chunk.getByteBuffer()), is("Hello World!")); chunk.release(); callback.get(5, TimeUnit.SECONDS); + + buffered.write(true, BufferUtil.EMPTY_BUFFER, Callback.NOOP); + chunk = async.read(); + assertThat(chunk.isLast(), is(true)); + assertThat(chunk.remaining(), is(0)); + chunk.release(); } } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java index c8bf3c0df71d..5ee7c1c1d5c6 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java @@ -18,6 +18,7 @@ import java.util.stream.Stream; import org.eclipse.jetty.util.BufferUtil; +import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -33,7 +34,7 @@ public class RetainableByteBufferTest { public static final int MIN_CAPACITY = 32; public static final int MAX_CAPACITY = 64; - private static ByteBufferPool _pool; + private static ArrayByteBufferPool.Tracking _pool; @BeforeAll public static void beforeAll() @@ -41,15 +42,21 @@ public static void beforeAll() _pool = new ArrayByteBufferPool.Tracking(MIN_CAPACITY, MIN_CAPACITY, MAX_CAPACITY, Integer.MAX_VALUE); } + @AfterAll + public static void afterAll() + { + assertThat("Leaks: " + _pool.dumpLeaks(), _pool.getLeaks().size(), is(0)); + } + static Stream buffers() { return Stream.of( Arguments.of(_pool.acquire(MIN_CAPACITY, true)), Arguments.of(_pool.acquire(MIN_CAPACITY, false)), - Arguments.of(RetainableByteBuffer.newAggregator(_pool, true, MIN_CAPACITY, MIN_CAPACITY)), - Arguments.of(RetainableByteBuffer.newAggregator(_pool, false, MIN_CAPACITY, MIN_CAPACITY)), - Arguments.of(RetainableByteBuffer.newAccumulator(_pool, true, MIN_CAPACITY)), - Arguments.of(RetainableByteBuffer.newAccumulator(_pool, false, MIN_CAPACITY)) + Arguments.of(new RetainableByteBuffer.Aggregator(_pool, true, MIN_CAPACITY, MIN_CAPACITY)), + Arguments.of(new RetainableByteBuffer.Aggregator(_pool, false, MIN_CAPACITY, MIN_CAPACITY)), + Arguments.of(new RetainableByteBuffer.Accumulator(_pool, true, MIN_CAPACITY)), + Arguments.of(new RetainableByteBuffer.Accumulator(_pool, false, MIN_CAPACITY)) ); } @@ -64,6 +71,7 @@ public void testEmptyBuffer(RetainableByteBuffer buffer) assertThat(buffer.getByteBuffer().remaining(), is(0)); assertFalse(buffer.getByteBuffer().hasRemaining()); + buffer.release(); } @ParameterizedTest @@ -75,6 +83,7 @@ public void testAppendOneByte(RetainableByteBuffer buffer) assertThat(buffer.append(bytes, 1, 1), is(1)); assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); + buffer.release(); } @ParameterizedTest @@ -87,6 +96,7 @@ public void testAppendMoreBytesThanCapacity(RetainableByteBuffer buffer) assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); assertTrue(buffer.isFull()); + buffer.release(); } @ParameterizedTest @@ -103,6 +113,7 @@ public void testAppendSmallByteBuffer(RetainableByteBuffer buffer) } assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); + buffer.release(); } @ParameterizedTest @@ -114,5 +125,6 @@ public void testAppendBigByteBuffer(RetainableByteBuffer buffer) assertTrue(from.hasRemaining()); assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); assertTrue(buffer.isFull()); + buffer.release(); } } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java index 4363e5891e9f..517081a5ed0a 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java @@ -681,6 +681,12 @@ else if (read < 0) } } + /** + * Write a {@link ByteBuffer} to an {@link OutputStream}, updating the position for the bytes written. + * @param buffer The buffer to write + * @param out The output stream + * @throws IOException if there was a problem writing. + */ public static void writeTo(ByteBuffer buffer, OutputStream out) throws IOException { if (buffer.hasArray()) From 7a91e7743826b48865da0347810f916a2d523e61 Mon Sep 17 00:00:00 2001 From: gregw Date: Sat, 20 Jan 2024 11:01:32 +0900 Subject: [PATCH 14/43] New implementation of BufferedContentSink --- .../jetty/io/content/BufferedContentSink.java | 205 +++++++++--------- .../jetty/io/BufferedContentSinkTest.java | 9 +- 2 files changed, 108 insertions(+), 106 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java index 5d911b94de52..fc89021dbfd2 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java @@ -15,12 +15,14 @@ import java.io.IOException; import java.nio.ByteBuffer; +import java.nio.channels.WritePendingException; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.IteratingCallback; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -40,6 +42,7 @@ public class BufferedContentSink implements Content.Sink private static final Logger LOG = LoggerFactory.getLogger(BufferedContentSink.class); + private final Flusher _flusher = new Flusher(); private final Content.Sink _delegate; private final ByteBufferPool _bufferPool; private final boolean _direct; @@ -66,7 +69,8 @@ public BufferedContentSink(Content.Sink delegate, ByteBufferPool bufferPool, boo private void releaseAggregator() { - _aggregator.release(); + if (_aggregator != null) + _aggregator.release(); _aggregator = null; } @@ -99,13 +103,12 @@ public void write(boolean last, ByteBuffer byteBuffer, Callback callback) // current buffer can be aggregated if (_aggregator == null) _aggregator = new RetainableByteBuffer.Aggregator(_bufferPool, _direct, _maxBufferSize); - aggregateAndFlush(last, current, callback); - } - else - { - // current buffer is greater than the max aggregation size - flush(last, current, callback); + aggregate(last, current, callback); + return; } + + // current buffer is greater than the max aggregation size + _flusher.flush(last, current, callback); } /** @@ -114,127 +117,125 @@ public void write(boolean last, ByteBuffer byteBuffer, Callback callback) */ public void flush(Callback callback) { - flush(false, FLUSH_BUFFER, callback); + _flusher.flush(false, FLUSH_BUFFER, callback); } /** - * Flushes the aggregated buffer if something was aggregated, then flushes the - * given buffer, bypassing the aggregator. + * Aggregates the given buffer, flushing the aggregated buffer if necessary. */ - private void flush(boolean last, ByteBuffer currentBuffer, Callback callback) + private void aggregate(boolean last, ByteBuffer byteBuffer, Callback callback) { + boolean full = !_aggregator.append(byteBuffer) || _aggregator.isFull(); + boolean empty = !byteBuffer.hasRemaining(); + boolean flush = full || byteBuffer == FLUSH_BUFFER; + boolean complete = last && empty; if (LOG.isDebugEnabled()) - LOG.debug("given buffer is greater than _maxBufferSize"); + LOG.debug("aggregated current buffer, full={}, complete={}, bytes left={}, aggregator={}", full, complete, byteBuffer.remaining(), _aggregator); + if (complete || flush) + _flusher.flush(last, byteBuffer, callback); + else + _flusher.callSucceeded(callback); + } + + private class Flusher extends IteratingCallback + { + private boolean _flush; + private boolean _last; + private ByteBuffer _byteBuffer; + private Callback _callback; + private boolean _lastWritten; - if (_aggregator == null) + private void flush(boolean last, ByteBuffer byteBuffer, Callback callback) { - if (LOG.isDebugEnabled()) - LOG.debug("nothing aggregated, flushing current buffer {}", currentBuffer); - _delegate.write(last, currentBuffer, callback); + if (_callback != null) + throw new WritePendingException(); + _flush = true; + _last = last; + _byteBuffer = byteBuffer; + _callback = callback; + iterate(); } - else if (BufferUtil.hasContent(currentBuffer)) + + private void callSucceeded(Callback callback) { - if (LOG.isDebugEnabled()) - LOG.debug("flushing aggregated buffer {}", _aggregator); + if (_callback != null) + throw new WritePendingException(); + _flush = false; + _callback = callback; + iterate(); + } - _aggregator.writeTo(_delegate, false, new Callback.Nested(Callback.from(this::releaseAggregator)) + @Override + protected Action process() + { + if (_flush && _aggregator != null && _aggregator.hasRemaining()) { - @Override - public void succeeded() - { - super.succeeded(); - if (LOG.isDebugEnabled()) - LOG.debug("succeeded writing aggregated buffer, flushing current buffer {}", currentBuffer); - _delegate.write(last, currentBuffer, callback); - } + boolean last = _last && BufferUtil.isEmpty(_byteBuffer); + _lastWritten |= last; + _aggregator.writeTo(_delegate, last, this); + return Action.SCHEDULED; + } - @Override - public void failed(Throwable x) + if (_flush && BufferUtil.hasContent(_byteBuffer)) + { + if (_aggregator != null && _byteBuffer.remaining() <= _maxAggregationSize) { - if (LOG.isDebugEnabled()) - LOG.debug("failure writing aggregated buffer", x); - super.failed(x); - callback.failed(x); + _aggregator.append(_byteBuffer); + this.succeeded(); + return Action.SCHEDULED; } - }); - } - else - { - _aggregator.writeTo(_delegate, last, last ? Callback.from(this::releaseAggregator, callback) : callback); - } - } + ByteBuffer buffer = _byteBuffer; + _byteBuffer = null; + _lastWritten |= _last; + _delegate.write(_last, buffer, this); + return Action.SCHEDULED; + } - /** - * Aggregates the given buffer, flushing the aggregated buffer if necessary. - */ - private void aggregateAndFlush(boolean last, ByteBuffer currentBuffer, Callback callback) - { - _aggregator.append(currentBuffer); - boolean full = _aggregator.isFull(); - boolean empty = !currentBuffer.hasRemaining(); - boolean flush = full || currentBuffer == FLUSH_BUFFER; - boolean complete = last && empty; - if (LOG.isDebugEnabled()) - LOG.debug("aggregated current buffer, full={}, complete={}, bytes left={}, aggregator={}", full, complete, currentBuffer.remaining(), _aggregator); - if (complete) - { - if (_aggregator != null) + _byteBuffer = null; + + if (_last && !_lastWritten) { - if (LOG.isDebugEnabled()) - LOG.debug("complete; writing aggregated buffer as the last one: {} bytes", _aggregator.remaining()); - _aggregator.writeTo(_delegate, true, Callback.from(this::releaseAggregator, callback)); + _lastWritten = true; + _delegate.write(_last, BufferUtil.EMPTY_BUFFER, this); + return Action.SCHEDULED; } - else + + if (_callback != null) { - if (LOG.isDebugEnabled()) - LOG.debug("complete; no aggregated buffer, writing last empty buffer"); - _delegate.write(true, BufferUtil.EMPTY_BUFFER, callback); + succeeded(); + return Action.SCHEDULED; } + + return _last ? Action.SUCCEEDED : Action.IDLE; } - else if (flush) - { - if (LOG.isDebugEnabled()) - LOG.debug("writing aggregated buffer: {} bytes, then {}", _aggregator.remaining(), currentBuffer.remaining()); - if (BufferUtil.isEmpty(currentBuffer)) - { - _aggregator.writeTo(_delegate, last, last ? Callback.from(this::releaseAggregator, callback) : callback); - } - else + @Override + public void succeeded() + { + if (BufferUtil.isEmpty(_byteBuffer)) { - _aggregator.writeTo(_delegate, false, new Callback() - { - @Override - public void succeeded() - { - if (LOG.isDebugEnabled()) - LOG.debug("written aggregated buffer, writing remaining of current: {} bytes{}", currentBuffer.remaining(), (last ? " (last write)" : "")); - if (last) - { - releaseAggregator(); - _delegate.write(true, currentBuffer, callback); - } - else - { - aggregateAndFlush(false, currentBuffer, callback); - } - } - - @Override - public void failed(Throwable x) - { - if (LOG.isDebugEnabled()) - LOG.debug("failure writing aggregated buffer", x); - callback.failed(x); - } - }); + Callback callback = _callback; + _callback = null; + if (callback != null) + callback.succeeded(); } + super.succeeded(); } - else + + @Override + protected void onCompleteFailure(Throwable cause) + { + releaseAggregator(); + Callback callback = _callback; + _callback = null; + if (callback != null) + callback.failed(cause); + } + + @Override + protected void onCompleteSuccess() { - if (LOG.isDebugEnabled()) - LOG.debug("buffer fully aggregated, delaying writing - aggregator: {}", _aggregator); - callback.succeeded(); + releaseAggregator(); } } } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java index 43129edab0fa..b77fe6715323 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java @@ -515,10 +515,11 @@ public void succeeded() @Test public void testByteByByteAsync() throws Exception { + // This test relies on selecting a size that will not be over allocated by the buffer pool try (AsyncContent async = new AsyncContent()) { - BufferedContentSink buffered = new BufferedContentSink(async, _bufferPool, true, 1024, 1024); - AtomicInteger count = new AtomicInteger(2048); + BufferedContentSink buffered = new BufferedContentSink(async, _bufferPool, true, 8 * 1024, 8 * 1024); + AtomicInteger count = new AtomicInteger(16 * 1024); CountDownLatch complete = new CountDownLatch(1); Callback callback = new Callback() { @@ -546,12 +547,12 @@ public void succeeded() Content.Chunk read = await().atMost(5, TimeUnit.SECONDS).until(async::read, Objects::nonNull); assertThat(read.isLast(), is(false)); - assertThat(read.remaining(), is(1024)); + assertThat(read.remaining(), is(8 * 1024)); assertThat(read.release(), is(true)); read = await().atMost(5, TimeUnit.SECONDS).until(async::read, Objects::nonNull); assertThat(read.isLast(), is(true)); - assertThat(read.remaining(), is(1024)); + assertThat(read.remaining(), is(8 * 1024)); assertThat(read.release(), is(true)); assertTrue(complete.await(5, TimeUnit.SECONDS)); From 7339dcd9d4d31a44269c1b854ed2d3256930995a Mon Sep 17 00:00:00 2001 From: gregw Date: Sat, 20 Jan 2024 11:21:44 +0900 Subject: [PATCH 15/43] javadoc --- .../jetty/io/RetainableByteBuffer.java | 2 +- .../ee10/servlet/ResponseHeadersTest.java | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 02954b1a7d15..aafe0793b7dc 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -270,7 +270,7 @@ public Aggregator(ByteBufferPool pool, boolean direct, int maxCapacity) * {@link RetainableByteBuffer}s with zero-copy if the {@link #append(RetainableByteBuffer)} API is used * @param pool The pool from which to allocate buffers * @param direct true if direct buffers should be used - * @param growBy the size to grow the buffer by or <= 0 for a heuristic + * @param growBy the size to grow the buffer by or <= 0 for a heuristic * @param maxCapacity The maximum requested length of the accumulated buffers or -1 for no limit. * Note that the pool may provide a buffer that exceeds this capacity. */ diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java index ac3ea5e7f856..af22e7b07723 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java @@ -14,6 +14,7 @@ package org.eclipse.jetty.ee10.servlet; import java.io.IOException; +import java.io.OutputStream; import java.io.PrintWriter; import java.net.URLDecoder; import java.nio.ByteBuffer; @@ -150,6 +151,44 @@ protected void doGet(HttpServletRequest req, HttpServletResponse response) throw assertThat("Response Header X-example", response.get("X-Example"), is(expected)); } + @Test + public void testCharsetAfterGetOutputStream() throws Exception + { + ServletContextHandler contextHandler = new ServletContextHandler(); + contextHandler.setContextPath("/"); + + HttpServlet charsetResetToJsonMimeTypeServlet = new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException + { + OutputStream out = response.getOutputStream(); + response.setCharacterEncoding("utf-8"); + response.setContentType("text/html"); + out.write("hello".getBytes(StandardCharsets.UTF_8)); + } + }; + + contextHandler.addServlet(charsetResetToJsonMimeTypeServlet, "/charset/hello/*"); + startServer(contextHandler); + + HttpTester.Request request = new HttpTester.Request(); + request.setMethod("GET"); + request.setURI("/charset/hello/"); + request.setVersion(HttpVersion.HTTP_1_1); + request.setHeader("Connection", "close"); + request.setHeader("Host", "test"); + + ByteBuffer responseBuffer = connector.getResponse(request.generate()); + // System.err.println(BufferUtil.toUTF8String(responseBuffer)); + HttpTester.Response response = HttpTester.parseResponse(responseBuffer); + + // Now test for properly formatted HTTP Response Headers. + assertThat("Response Code", response.getStatus(), is(200)); + // The Content-Type should not have a charset= portion + assertThat("Response Header Content-Type", response.get("Content-Type"), is("text/html;charset=UTF-8")); + } + @Test public void testCharsetResetToJsonMimeType() throws Exception { From 089145b812900cf6bf5a681c3cbb3b986cdd1cc8 Mon Sep 17 00:00:00 2001 From: gregw Date: Sun, 21 Jan 2024 07:33:52 +0900 Subject: [PATCH 16/43] do not clear on release --- .../java/org/eclipse/jetty/http/MultiPart.java | 2 +- .../org/eclipse/jetty/io/content/AsyncContent.java | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index bf9902492378..0551524b6ad6 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -1085,7 +1085,7 @@ else if (type != HttpTokens.Type.SPACE && type != HttpTokens.Type.HTAB) if (state == State.EPILOGUE) notifyComplete(); else - throw new EOFException("unexpected EOF"); + throw new EOFException("unexpected EOF in " + state); } } catch (Throwable x) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java index 26932ec66dfe..9055e1f76d13 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java @@ -317,8 +317,6 @@ public void retain() @Override public boolean release() { - if (hasRemaining()) - BufferUtil.clear(getByteBuffer()); if (!canRetain()) return true; boolean released = referenceCounter.release(); @@ -338,5 +336,17 @@ public void failed(Throwable x) { callback.failed(x); } + + @Override + public String toString() + { + return "%s@%x[rc=%d,l=%b,b=%s]".formatted( + getClass().getSimpleName(), + hashCode(), + referenceCounter.get(), + isLast(), + BufferUtil.toDetailString(getByteBuffer()) + ); + } } } From df3d95099b94aa43bcf3c6e64f0d6dec14b3c1c2 Mon Sep 17 00:00:00 2001 From: gregw Date: Sun, 21 Jan 2024 15:34:57 +0900 Subject: [PATCH 17/43] More fixes to BufferedContentSink --- .../jetty/io/RetainableByteBuffer.java | 16 ++++- .../jetty/io/content/BufferedContentSink.java | 62 +++++++++++-------- 2 files changed, 52 insertions(+), 26 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index aafe0793b7dc..1f34527e3b9d 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -291,7 +291,21 @@ public Aggregator(ByteBufferPool pool, boolean direct, int growBy, int maxCapaci throw new IllegalArgumentException("growBy(%d) must be <= maxCapacity(%d)".formatted(growBy, _maxCapacity)); _growBy = growBy; - _buffer = _pool.acquire(growBy, _direct); + _buffer = _pool.acquire(_growBy, _direct); + } + } + + @Override + public void clear() + { + if (isRetained()) + { + _buffer.release(); + _buffer = _pool.acquire(_growBy, _direct); + } + else + { + BufferUtil.clear(_buffer.getByteBuffer()); } } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java index fc89021dbfd2..4c8b442810c1 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java @@ -134,11 +134,18 @@ private void aggregate(boolean last, ByteBuffer byteBuffer, Callback callback) if (complete || flush) _flusher.flush(last, byteBuffer, callback); else - _flusher.callSucceeded(callback); + _flusher.serialize(callback); } private class Flusher extends IteratingCallback { + private enum Scheduled + { + FLUSHING_AGGREGATION, + FLUSHING_BUFFER, + } + + private Scheduled _scheduled; private boolean _flush; private boolean _last; private ByteBuffer _byteBuffer; @@ -156,7 +163,7 @@ private void flush(boolean last, ByteBuffer byteBuffer, Callback callback) iterate(); } - private void callSucceeded(Callback callback) + private void serialize(Callback callback) { if (_callback != null) throw new WritePendingException(); @@ -168,31 +175,46 @@ private void callSucceeded(Callback callback) @Override protected Action process() { + if (_scheduled != null) + { + switch (_scheduled) + { + case FLUSHING_AGGREGATION -> + { + _aggregator.clear(); + if (_byteBuffer != null && _byteBuffer.remaining() <= _maxAggregationSize && !_last) + { + _aggregator.append(_byteBuffer); + _byteBuffer = null; + _flush = false; + } + } + + case FLUSHING_BUFFER -> + _byteBuffer = null; + } + _scheduled = null; + } + if (_flush && _aggregator != null && _aggregator.hasRemaining()) { boolean last = _last && BufferUtil.isEmpty(_byteBuffer); _lastWritten |= last; _aggregator.writeTo(_delegate, last, this); + _scheduled = Scheduled.FLUSHING_AGGREGATION; return Action.SCHEDULED; } if (_flush && BufferUtil.hasContent(_byteBuffer)) { - if (_aggregator != null && _byteBuffer.remaining() <= _maxAggregationSize) - { - _aggregator.append(_byteBuffer); - this.succeeded(); - return Action.SCHEDULED; - } ByteBuffer buffer = _byteBuffer; _byteBuffer = null; _lastWritten |= _last; _delegate.write(_last, buffer, this); + _scheduled = Scheduled.FLUSHING_BUFFER; return Action.SCHEDULED; } - _byteBuffer = null; - if (_last && !_lastWritten) { _lastWritten = true; @@ -200,28 +222,18 @@ protected Action process() return Action.SCHEDULED; } - if (_callback != null) + Callback callback = _callback; + if (callback != null) { - succeeded(); + _callback = null; + callback.succeeded(); + this.succeeded(); return Action.SCHEDULED; } return _last ? Action.SUCCEEDED : Action.IDLE; } - @Override - public void succeeded() - { - if (BufferUtil.isEmpty(_byteBuffer)) - { - Callback callback = _callback; - _callback = null; - if (callback != null) - callback.succeeded(); - } - super.succeeded(); - } - @Override protected void onCompleteFailure(Throwable cause) { From 475e570822ad2fc55b1889a9ce5ef98d3b998cfb Mon Sep 17 00:00:00 2001 From: gregw Date: Sun, 21 Jan 2024 16:04:29 +0900 Subject: [PATCH 18/43] forced flush --- .../java/org/eclipse/jetty/io/content/BufferedContentSink.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java index 4c8b442810c1..d709df155946 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/BufferedContentSink.java @@ -205,7 +205,7 @@ protected Action process() return Action.SCHEDULED; } - if (_flush && BufferUtil.hasContent(_byteBuffer)) + if (_flush && (BufferUtil.hasContent(_byteBuffer) || _byteBuffer == FLUSH_BUFFER)) { ByteBuffer buffer = _byteBuffer; _byteBuffer = null; From 8a30dce0bae7e6bcf84bc4ed3d10b5377bc2c75b Mon Sep 17 00:00:00 2001 From: gregw Date: Sun, 21 Jan 2024 17:44:43 +0900 Subject: [PATCH 19/43] Fixed bug in BufferUtil --- .../src/main/java/org/eclipse/jetty/util/BufferUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java index 517081a5ed0a..5613420c883b 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java @@ -445,7 +445,7 @@ public static int put(ByteBuffer from, ByteBuffer to) return length; } - if (to.hasArray()) + if (from.hasArray()) { to.put(from.array(), from.arrayOffset() + from.position(), space); from.position(from.position() + space); From 4b6b2a820be79e96e8e2af08a1e75019a60bd9ce Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 22 Jan 2024 08:20:34 +0900 Subject: [PATCH 20/43] document long to int conversion --- .../jetty/io/RetainableByteBuffer.java | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 1f34527e3b9d..a5ce3c2407f6 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -442,19 +442,37 @@ public ByteBuffer getByteBuffer() }; } + /** + * {@inheritDoc} + * @throws ArithmeticException if the length of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE} + */ @Override public int remaining() + { + return Math.toIntExact(remainingLong()); + } + + public long remainingLong() { long length = 0; for (RetainableByteBuffer buffer : _buffers) length += buffer.remaining(); - return Math.toIntExact(length); + return length; } + /** + * {@inheritDoc} + * @throws ArithmeticException if the maxLength of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE}. + */ @Override public int capacity() { - return Math.toIntExact(_maxLength); + return Math.toIntExact(capacityLong()); + } + + public long capacityLong() + { + return _maxLength; } @Override From 6f8a2d96eed05aaa07cfed27b486f54ccdee36c6 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 1 Mar 2024 16:33:00 +0100 Subject: [PATCH 21/43] removed byte[] append API --- .../jetty/io/RetainableByteBuffer.java | 57 +------------------ .../jetty/io/RetainableByteBufferTest.java | 17 +++++- 2 files changed, 17 insertions(+), 57 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index a5ce3c2407f6..d7e86a0bbeea 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -192,29 +192,6 @@ default void clear() BufferUtil.clear(getByteBuffer()); } - /** Append byte array to the buffer, potentially limited by capacity. - * @param bytes the byte array to append - * @param offset the offset into the array - * @param length the number of bytes to try to append - * @return the number of bytes actually appended. - */ - default int append(byte[] bytes, int offset, int length) - { - ByteBuffer to = getByteBuffer(); - - int pos = BufferUtil.flipToFill(to); - try - { - length = Math.min(length, to.remaining()); - to.put(bytes, offset, length); - } - finally - { - BufferUtil.flipToFlush(to, pos); - } - return length; - } - default boolean append(ByteBuffer bytes) { BufferUtil.append(getByteBuffer(), bytes); @@ -345,13 +322,6 @@ public int capacity() return Math.max(_buffer.capacity(), _maxCapacity); } - @Override - public int append(byte[] bytes, int offset, int length) - { - ensureSpace(length); - return RetainableByteBuffer.super.append(bytes, offset, length); - } - @Override public boolean append(ByteBuffer bytes) { @@ -491,43 +461,22 @@ public void clear() _buffers.clear(); } - @Override - public int append(byte[] bytes, int offset, int length) - { - length = ensureMaxLength(length); - - if (_canAggregate) - { - RetainableByteBuffer last = _buffers.get(_buffers.size() - 1); - if (length <= last.space()) - { - last.append(bytes, offset, length); - return length; - } - } - - RetainableByteBuffer buffer = _pool.acquire(length, _direct); - buffer.append(bytes, offset, length); - _buffers.add(buffer); - _canAggregate = true; - return length; - } - @Override public boolean append(ByteBuffer bytes) { int remaining = bytes.remaining(); int length = ensureMaxLength(remaining); // if the length was restricted by maxLength, slice the bytes smaller + ByteBuffer out = bytes; if (length < remaining) { ByteBuffer slice = bytes.slice(); slice.limit(slice.position() + length); bytes.position(bytes.position() + length); - bytes = slice; + out = slice; } RetainableByteBuffer buffer = _pool.acquire(length, _direct); - buffer.append(bytes); + buffer.append(out); _buffers.add(buffer); _canAggregate = true; return !bytes.hasRemaining(); diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java index 5ee7c1c1d5c6..6cbe77240b7f 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java @@ -48,7 +48,7 @@ public static void afterAll() assertThat("Leaks: " + _pool.dumpLeaks(), _pool.getLeaks().size(), is(0)); } - static Stream buffers() + public static Stream buffers() { return Stream.of( Arguments.of(_pool.acquire(MIN_CAPACITY, true)), @@ -80,7 +80,7 @@ public void testAppendOneByte(RetainableByteBuffer buffer) { byte[] bytes = new byte[] {'-', 'X', '-'}; while (!buffer.isFull()) - assertThat(buffer.append(bytes, 1, 1), is(1)); + assertThat(buffer.append(ByteBuffer.wrap(bytes, 1, 1)), is(true)); assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); buffer.release(); @@ -92,7 +92,18 @@ public void testAppendMoreBytesThanCapacity(RetainableByteBuffer buffer) { byte[] bytes = new byte[MAX_CAPACITY * 2]; Arrays.fill(bytes, (byte)'X'); - assertThat(buffer.append(bytes, 0, bytes.length), is(buffer.capacity())); + ByteBuffer b = ByteBuffer.wrap(bytes); + + if (buffer.append(b)) + { + assertTrue(BufferUtil.isEmpty(b)); + assertThat(buffer.capacity(), greaterThanOrEqualTo(MAX_CAPACITY * 2)); + } + else + { + assertFalse(BufferUtil.isEmpty(b)); + assertThat(b.remaining(), is(MAX_CAPACITY * 2 - buffer.capacity())); + } assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); assertTrue(buffer.isFull()); From 44dcac8d0f4f66a1c241b53fd6e4eecabb2e2b80 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 1 Mar 2024 16:40:32 +0100 Subject: [PATCH 22/43] removed writeTo(OutputStream) --- .../java/org/eclipse/jetty/io/Content.java | 23 +++++++++++++++++++ .../jetty/io/RetainableByteBuffer.java | 14 ----------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index daffe5b0b7bd..e238f1220742 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -470,6 +470,29 @@ default boolean rewind() */ public interface Sink { + /** + *

Wraps the given {@link OutputStream} as a {@link Sink}. + * @param out The stream to wrap + * @return A sink wrapping the stream + */ + static Sink from(OutputStream out) + { + return (last, byteBuffer, callback) -> + { + try + { + BufferUtil.writeTo(byteBuffer, out); + if (last) + out.close(); + callback.succeeded(); + } + catch (Throwable t) + { + callback.failed(t); + } + }; + } + /** *

Wraps the given content sink with a buffering sink.

* diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index d7e86a0bbeea..bfaac1decb66 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -13,8 +13,6 @@ package org.eclipse.jetty.io; -import java.io.IOException; -import java.io.OutputStream; import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.List; @@ -208,11 +206,6 @@ default void putTo(ByteBuffer toInfillMode) toInfillMode.put(getByteBuffer()); } - default void writeTo(OutputStream out) throws IOException - { - BufferUtil.writeTo(getByteBuffer(), out); - } - default void writeTo(Content.Sink sink, boolean last, Callback callback) { sink.write(last, getByteBuffer(), callback); @@ -522,13 +515,6 @@ public void putTo(ByteBuffer toInfillMode) buffer.putTo(toInfillMode); } - @Override - public void writeTo(OutputStream out) throws IOException - { - for (RetainableByteBuffer buffer : _buffers) - buffer.writeTo(out); - } - @Override public void writeTo(Content.Sink sink, boolean last, Callback callback) { From c48fae491459bb2d8312f134a6a89700db11fde7 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 1 Mar 2024 17:07:16 +0100 Subject: [PATCH 23/43] updates from review --- .../org/eclipse/jetty/io/RetainableByteBuffer.java | 10 +++++----- .../org/eclipse/jetty/io/content/AsyncContent.java | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index bfaac1decb66..86bc002d837b 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -227,7 +227,7 @@ class Aggregator implements RetainableByteBuffer * {@link RetainableByteBuffer}s with zero-copy if the {@link #append(RetainableByteBuffer)} API is used * @param pool The pool from which to allocate buffers * @param direct true if direct buffers should be used - * @param maxCapacity The maximum requested length of the accumulated buffers or -1 for no limit. + * @param maxCapacity The maximum requested length of the accumulated buffers or -1 for 2GB limit. * Note that the pool may provide a buffer that exceeds this capacity. */ public Aggregator(ByteBufferPool pool, boolean direct, int maxCapacity) @@ -241,7 +241,7 @@ public Aggregator(ByteBufferPool pool, boolean direct, int maxCapacity) * @param pool The pool from which to allocate buffers * @param direct true if direct buffers should be used * @param growBy the size to grow the buffer by or <= 0 for a heuristic - * @param maxCapacity The maximum requested length of the accumulated buffers or -1 for no limit. + * @param maxCapacity The maximum requested length of the accumulated buffers or -1 for 2GB limit. * Note that the pool may provide a buffer that exceeds this capacity. */ public Aggregator(ByteBufferPool pool, boolean direct, int growBy, int maxCapacity) @@ -368,13 +368,13 @@ class Accumulator implements RetainableByteBuffer * {@link RetainableByteBuffer}s with zero-copy if the {@link #append(RetainableByteBuffer)} API is used * @param pool The pool from which to allocate buffers * @param direct true if direct buffers should be used - * @param maxLength The maximum length of the accumulated buffers or -1 for no limit + * @param maxLength The maximum length of the accumulated buffers or -1 for 2GB limit */ public Accumulator(ByteBufferPool pool, boolean direct, long maxLength) { _pool = pool == null ? new ByteBufferPool.NonPooling() : pool; _direct = direct; - _maxLength = maxLength < 0 ? Long.MAX_VALUE : maxLength; + _maxLength = maxLength < 0 ? Integer.MAX_VALUE : maxLength; } @Override @@ -531,7 +531,7 @@ protected Action process() { if (i < _buffers.size()) { - _buffers.get(i).writeTo(sink, ++i == _buffers.size() || !last, this); + _buffers.get(i).writeTo(sink, last && ++i == _buffers.size(), this); return Action.SCHEDULED; } return Action.SUCCEEDED; diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java index 9055e1f76d13..d4db343868a2 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java @@ -340,10 +340,10 @@ public void failed(Throwable x) @Override public String toString() { - return "%s@%x[rc=%d,l=%b,b=%s]".formatted( + return "%s@%x[rc=%s,l=%b,b=%s]".formatted( getClass().getSimpleName(), hashCode(), - referenceCounter.get(), + referenceCounter == null ? "-" : referenceCounter.get(), isLast(), BufferUtil.toDetailString(getByteBuffer()) ); From 6c9e40f49eb20f212b7b62fe031c662e0f268b3d Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 5 Mar 2024 10:26:22 +0100 Subject: [PATCH 24/43] Don't modify deprecated class --- .../jetty/io/ByteBufferAccumulator.java | 291 +++--------------- .../jetty/io/RetainableByteBuffer.java | 38 +-- .../io/internal/ContentSourceByteBuffer.java | 10 +- .../jetty/io/ByteBufferAccumulatorTest.java | 110 ++----- 4 files changed, 85 insertions(+), 364 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java index 054deb4c999c..8bfdfe5a15ba 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferAccumulator.java @@ -18,27 +18,24 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.util.BufferUtil; -import org.eclipse.jetty.util.Callback; /** * Accumulates data into a list of ByteBuffers which can then be combined into a single buffer or written to an OutputStream. * The buffer list automatically grows as data is written to it, the buffers are taken from the * supplied {@link ByteBufferPool} or freshly allocated if one is not supplied. - *

- * The method {@link #accessInternalBuffer(int, int)} can be used access a buffer that can be written to directly as the last buffer list, + * + * The method {@link #ensureBuffer(int, int)} is used to write directly to the last buffer stored in the buffer list, * if there is less than a certain amount of space available in that buffer then a new one will be allocated and returned instead. * @deprecated use {@link RetainableByteBuffer.Accumulator} */ -@Deprecated +@Deprecated(forRemoval = true) public class ByteBufferAccumulator implements AutoCloseable { private final List _buffers = new ArrayList<>(); private final ByteBufferPool _bufferPool; private final boolean _direct; - private final long _maxLength; public ByteBufferAccumulator() { @@ -46,15 +43,9 @@ public ByteBufferAccumulator() } public ByteBufferAccumulator(ByteBufferPool bufferPool, boolean direct) - { - this(bufferPool, direct, -1); - } - - public ByteBufferAccumulator(ByteBufferPool bufferPool, boolean direct, long maxLength) { _bufferPool = (bufferPool == null) ? new ByteBufferPool.NonPooling() : bufferPool; _direct = direct; - _maxLength = maxLength; } /** @@ -62,9 +53,9 @@ public ByteBufferAccumulator(ByteBufferPool bufferPool, boolean direct, long max * This will add up the remaining of each buffer in the accumulator. * @return the total length of the content in the accumulator. */ - public long getLength() + public int getLength() { - long length = 0; + int length = 0; for (RetainableByteBuffer buffer : _buffers) length = Math.addExact(length, buffer.remaining()); return length; @@ -74,12 +65,10 @@ public long getLength() * Get the last buffer of the accumulator, this can be written to directly to avoid copying into the accumulator. * @param minAllocationSize new buffers will be allocated to have at least this size. * @return a buffer with at least {@code minSize} space to write into. - * @deprecated Use {@link #copyBuffer(ByteBuffer)}, {@link #copyBytes(byte[], int, int)} or {@link #addBuffer(RetainableByteBuffer)} */ - @Deprecated public RetainableByteBuffer ensureBuffer(int minAllocationSize) { - return accessInternalBuffer(1, minAllocationSize); + return ensureBuffer(1, minAllocationSize); } /** @@ -87,22 +76,8 @@ public RetainableByteBuffer ensureBuffer(int minAllocationSize) * @param minSize the smallest amount of remaining space before a new buffer is allocated. * @param minAllocationSize new buffers will be allocated to have at least this size. * @return a buffer with at least {@code minSize} space to write into. - * @deprecated Use {@link #copyBuffer(ByteBuffer)}, {@link #copyBytes(byte[], int, int)} or {@link #addBuffer(RetainableByteBuffer)} */ - @Deprecated public RetainableByteBuffer ensureBuffer(int minSize, int minAllocationSize) - { - return accessInternalBuffer(minSize, minAllocationSize); - } - - /** - * Get the last buffer of the accumulator, this can be written to directly to avoid copying into the accumulator. - * Used internally and for testing. - * @param minSize the smallest amount of remaining space before a new buffer is allocated. - * @param minAllocationSize new buffers will be allocated to have at least this size. - * @return a buffer with at least {@code minSize} space to write into. - */ - RetainableByteBuffer accessInternalBuffer(int minSize, int minAllocationSize) { RetainableByteBuffer buffer = _buffers.isEmpty() ? null : _buffers.get(_buffers.size() - 1); if (buffer == null || BufferUtil.space(buffer.getByteBuffer()) < minSize) @@ -113,159 +88,23 @@ RetainableByteBuffer accessInternalBuffer(int minSize, int minAllocationSize) return buffer; } - private boolean maxLengthExceeded(long extraLength) - { - if (_maxLength < 0) - return false; - long length = Math.addExact(getLength(), extraLength); - return length > _maxLength; - } - - /** - * Copy bytes from an array into this accumulator - * @param buf The byte array to copy from - * @param offset The offset into the array to start copying from - * @param length The number of bytes to copy - * @throws IllegalArgumentException if the max length is exceeded. - */ public void copyBytes(byte[] buf, int offset, int length) - throws IllegalArgumentException { copyBuffer(BufferUtil.toBuffer(buf, offset, length)); } - /** - * Copy bytes from a {@link ByteBuffer} into this accumulator - * @param buffer The {@code ByteBuffer} to copy from, whose position is updated. - * @throws IllegalArgumentException if the max length is exceeded. - */ - public void copyBuffer(ByteBuffer buffer) - throws IllegalArgumentException + public void copyBuffer(ByteBuffer source) { - if (maxLengthExceeded(buffer.remaining())) - throw new IllegalArgumentException("maxLength exceeded"); - while (buffer.hasRemaining()) + while (source.hasRemaining()) { - RetainableByteBuffer target = accessInternalBuffer(1, buffer.remaining()); - ByteBuffer byteBuffer = target.getByteBuffer(); + RetainableByteBuffer buffer = ensureBuffer(source.remaining()); + ByteBuffer byteBuffer = buffer.getByteBuffer(); int pos = BufferUtil.flipToFill(byteBuffer); - BufferUtil.put(buffer, byteBuffer); + BufferUtil.put(source, byteBuffer); BufferUtil.flipToFlush(byteBuffer, pos); } } - /** - * Add (without copying if possible) a {@link RetainableByteBuffer} - * @param buffer The {@code RetainableByteBuffer} to add to the accumulator, which will either be - * {@link Retainable#retain() retained} or copied if it {@link Retainable#canRetain() cannot be retained}. - * The buffers position will not be updated. - */ - public void addBuffer(RetainableByteBuffer buffer) - { - if (buffer != null && buffer.hasRemaining()) - { - if (maxLengthExceeded(buffer.remaining())) - throw new IllegalArgumentException("maxLength exceeded"); - - if (buffer.canRetain()) - { - buffer.retain(); - _buffers.add(buffer); - } - else - { - copyBuffer(buffer.getByteBuffer().slice()); - } - } - } - - /** - * Add without copying a {@link ByteBuffer}. - * @param buffer The {@code ByteBuffer} to add. - * @param releaseCallback A callback that is {@link Callback#succeeded() succeeded} when the buffer is no longer held, - * or {@link Callback#failed(Throwable) failed} if the {@link #fail(Throwable)} method is called. - */ - public void addBuffer(ByteBuffer buffer, Callback releaseCallback) - { - if (BufferUtil.isEmpty(buffer)) - { - releaseCallback.succeeded(); - } - else - { - _buffers.add(new FailableRetainableByteBuffer(buffer, releaseCallback)); - } - } - - /** - * Aggregates the given ByteBuffer into the last buffer of this accumulation, growing the buffer in size if - * necessary. This copies bytes up to the specified maximum size into the - * last buffer in the accumulation, at which time this method returns {@code true} - * and {@link #takeRetainableByteBuffer()} must be called for this method to accept aggregating again. - * @param source the buffer to copy into this aggregator; its position is updated according to - * the number of aggregated bytes - * @return true if the aggregator's buffer is full and should be taken, false otherwise - */ - public boolean aggregate(ByteBuffer source) - { - if (BufferUtil.isEmpty(source)) - return false; - - // How much of the buffer can be aggregated? - int toCopy = source.remaining(); - boolean full = false; - if (_maxLength >= 0) - { - long space = _maxLength - getLength(); - if (space == 0) - return true; - if (toCopy >= space) - { - full = true; - toCopy = (int)space; - } - } - - // Do we need to allocate a new buffer? - RetainableByteBuffer buffer = _buffers.isEmpty() ? null : _buffers.get(_buffers.size() - 1); - if (buffer == null || buffer.isRetained() || BufferUtil.space(buffer.getByteBuffer()) < toCopy) - { - int prefix = buffer == null ? 0 : buffer.remaining(); - int minSize = prefix + toCopy; - int allocSize = (int)Math.min(_maxLength, ceilToNextPowerOfTwo(minSize)); - RetainableByteBuffer next = _bufferPool.acquire(allocSize, _direct); - - if (prefix > 0) - BufferUtil.append(next.getByteBuffer(), buffer.getByteBuffer()); - - if (buffer == null) - _buffers.add(next); - else - { - _buffers.set(_buffers.size() - 1, next); - buffer.release(); - } - buffer = next; - } - - // Aggregate the bytes into the prepared space - ByteBuffer target = buffer.getByteBuffer(); - int p = BufferUtil.flipToFill(target); - int tp = target.position(); - target.put(tp, source, 0, toCopy); - source.position(source.position() + toCopy); - target.position(tp + toCopy); - BufferUtil.flipToFlush(target, p); - - return full; - } - - private static int ceilToNextPowerOfTwo(int val) - { - int result = 1 << (Integer.SIZE - Integer.numberOfLeadingZeros(val - 1)); - return result > 0 ? result : Integer.MAX_VALUE; - } - /** * Take the combined buffer containing all content written to the accumulator. * The caller is responsible for releasing this {@link RetainableByteBuffer}. @@ -274,33 +113,26 @@ private static int ceilToNextPowerOfTwo(int val) */ public RetainableByteBuffer takeRetainableByteBuffer() { - return switch (_buffers.size()) + RetainableByteBuffer combinedBuffer; + if (_buffers.size() == 1) + { + combinedBuffer = _buffers.get(0); + _buffers.clear(); + return combinedBuffer; + } + + int length = getLength(); + combinedBuffer = _bufferPool.acquire(length, _direct); + ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); + BufferUtil.clearToFill(byteBuffer); + for (RetainableByteBuffer buffer : _buffers) { - case 0 -> RetainableByteBuffer.EMPTY; - case 1 -> - { - RetainableByteBuffer buffer = _buffers.get(0); - _buffers.clear(); - yield buffer; - } - default -> - { - long length = getLength(); - if (length > Integer.MAX_VALUE) - throw new IllegalStateException("too large for ByteBuffer"); - RetainableByteBuffer combinedBuffer = _bufferPool.acquire((int)length, _direct); - ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); - BufferUtil.clearToFill(byteBuffer); - for (RetainableByteBuffer buffer : _buffers) - { - byteBuffer.put(buffer.getByteBuffer()); - buffer.release(); - } - BufferUtil.flipToFlush(byteBuffer, 0); - _buffers.clear(); - yield combinedBuffer; - } - }; + byteBuffer.put(buffer.getByteBuffer()); + buffer.release(); + } + BufferUtil.flipToFlush(byteBuffer, 0); + _buffers.clear(); + return combinedBuffer; } public ByteBuffer takeByteBuffer() @@ -330,30 +162,24 @@ public RetainableByteBuffer toRetainableByteBuffer() */ public byte[] toByteArray() { - long length = getLength(); + int length = getLength(); if (length == 0) return new byte[0]; - if (length > Integer.MAX_VALUE) - throw new IllegalStateException("too large for array"); - byte[] bytes = new byte[(int)length]; + + byte[] bytes = new byte[length]; ByteBuffer buffer = BufferUtil.toBuffer(bytes); BufferUtil.clear(buffer); writeTo(buffer); return bytes; } - public byte[] takeByteArray() - { - byte[] out = toByteArray(); - close(); - return out; - } - public void writeTo(ByteBuffer byteBuffer) { int pos = BufferUtil.flipToFill(byteBuffer); for (RetainableByteBuffer buffer : _buffers) + { byteBuffer.put(buffer.getByteBuffer().slice()); + } BufferUtil.flipToFlush(byteBuffer, pos); } @@ -371,51 +197,4 @@ public void close() _buffers.forEach(RetainableByteBuffer::release); _buffers.clear(); } - - public void fail(Throwable cause) - { - _buffers.forEach(r -> - { - if (r instanceof FailableRetainableByteBuffer frbb) - frbb.fail(cause); - else - r.release(); - }); - _buffers.clear(); - } - - private static class FailableRetainableByteBuffer implements RetainableByteBuffer - { - private final ByteBuffer _buffer; - private final AtomicReference _callback; - - private FailableRetainableByteBuffer(ByteBuffer buffer, Callback callback) - { - _buffer = buffer; - _callback = new AtomicReference<>(callback); - } - - @Override - public ByteBuffer getByteBuffer() - { - return _buffer; - } - - @Override - public boolean release() - { - Callback callback = _callback.getAndSet(null); - if (callback == null) - return false; - callback.succeeded(); - return true; - } - - public void fail(Throwable cause) - { - Callback callback = _callback.getAndSet(null); - if (callback != null) - callback.failed(cause); - } - } } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 86bc002d837b..8a0c4f74f6eb 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -384,27 +384,29 @@ public ByteBuffer getByteBuffer() { case 0 -> RetainableByteBuffer.EMPTY.getByteBuffer(); case 1 -> _buffers.get(0).getByteBuffer(); - default -> - { - int length = remaining(); - RetainableByteBuffer combinedBuffer = _pool.acquire(length, _direct); - ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); - BufferUtil.flipToFill(byteBuffer); - for (RetainableByteBuffer buffer : _buffers) - { - buffer.putTo(byteBuffer); - buffer.clear(); - buffer.release(); - } - BufferUtil.flipToFlush(byteBuffer, 0); - _buffers.clear(); - _buffers.add(combinedBuffer); - _canAggregate = true; - yield combinedBuffer.getByteBuffer(); - } + default -> copyBuffer(); }; } + public ByteBuffer copyBuffer() + { + int length = remaining(); + RetainableByteBuffer combinedBuffer = _pool.acquire(length, _direct); + ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); + BufferUtil.flipToFill(byteBuffer); + for (RetainableByteBuffer buffer : _buffers) + { + buffer.putTo(byteBuffer); + buffer.clear(); + buffer.release(); + } + BufferUtil.flipToFlush(byteBuffer, 0); + _buffers.clear(); + _buffers.add(combinedBuffer); + _canAggregate = true; + return combinedBuffer.getByteBuffer(); + } + /** * {@inheritDoc} * @throws ArithmeticException if the length of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE} diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java index b9e9a754fe48..fa5b3d420a47 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java @@ -15,13 +15,13 @@ import java.nio.ByteBuffer; -import org.eclipse.jetty.io.ByteBufferAccumulator; import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.util.Promise; public class ContentSourceByteBuffer implements Runnable { - private final ByteBufferAccumulator accumulator = new ByteBufferAccumulator(); + private final RetainableByteBuffer.Accumulator accumulator = new RetainableByteBuffer.Accumulator(null, false, -1); private final Content.Source source; private final Promise promise; @@ -52,12 +52,14 @@ public void run() return; } - accumulator.addBuffer(chunk); + accumulator.append(chunk); chunk.release(); if (chunk.isLast()) { - promise.succeeded(accumulator.takeByteBuffer()); + ByteBuffer buffer = accumulator.copyBuffer(); + accumulator.release(); + promise.succeeded(buffer); return; } } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java index 89c6f651c1e0..ec2332863178 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteBufferAccumulatorTest.java @@ -26,10 +26,9 @@ import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.is; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; +@Deprecated(forRemoval = true) public class ByteBufferAccumulatorTest { private CountingBufferPool bufferPool; @@ -51,19 +50,19 @@ public void testToBuffer() ByteBuffer slice = content.slice(); // We completely fill up the internal buffer with the first write. - RetainableByteBuffer internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); + RetainableByteBuffer internalBuffer = accumulator.ensureBuffer(1, allocationSize); ByteBuffer byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), greaterThanOrEqualTo(allocationSize)); writeInFlushMode(slice, byteBuffer); assertThat(BufferUtil.space(byteBuffer), is(0)); // If we ask for min size of 0 we get the same buffer which is full. - internalBuffer = accumulator.accessInternalBuffer(0, allocationSize); + internalBuffer = accumulator.ensureBuffer(0, allocationSize); byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), is(0)); // If we need at least 1 minSpace we must allocate a new buffer. - internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); + internalBuffer = accumulator.ensureBuffer(1, allocationSize); byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), greaterThan(0)); assertThat(BufferUtil.space(byteBuffer), greaterThanOrEqualTo(allocationSize)); @@ -79,31 +78,31 @@ public void testToBuffer() // If we request anything under the amount remaining we get back the same buffer. for (int i = 0; i <= 13; i++) { - internalBuffer = accumulator.accessInternalBuffer(i, allocationSize); + internalBuffer = accumulator.ensureBuffer(i, allocationSize); byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), is(13)); } // If we request over 13 then we get a new buffer. - internalBuffer = accumulator.accessInternalBuffer(14, allocationSize); + internalBuffer = accumulator.ensureBuffer(14, allocationSize); byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), greaterThanOrEqualTo(1024)); // Copy the rest of the content. while (slice.hasRemaining()) { - internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); + internalBuffer = accumulator.ensureBuffer(1, allocationSize); byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), greaterThanOrEqualTo(1)); writeInFlushMode(slice, byteBuffer); } // Check we have the same content as the original buffer. - assertThat(accumulator.getLength(), is((long)size)); + assertThat(accumulator.getLength(), is(size)); assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); RetainableByteBuffer combinedBuffer = accumulator.toRetainableByteBuffer(); assertThat(bufferPool.getAcquireCount(), is(1L)); - assertThat(accumulator.getLength(), is((long)size)); + assertThat(accumulator.getLength(), is(size)); assertThat(combinedBuffer.getByteBuffer(), is(content)); // Close the accumulator and make sure all is returned to bufferPool. @@ -122,18 +121,18 @@ public void testTakeBuffer() // Copy the content. while (slice.hasRemaining()) { - RetainableByteBuffer internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); + RetainableByteBuffer internalBuffer = accumulator.ensureBuffer(1, allocationSize); ByteBuffer byteBuffer = internalBuffer.getByteBuffer(); assertThat(BufferUtil.space(byteBuffer), greaterThanOrEqualTo(1)); writeInFlushMode(slice, byteBuffer); } // Check we have the same content as the original buffer. - assertThat(accumulator.getLength(), is((long)size)); + assertThat(accumulator.getLength(), is(size)); assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); RetainableByteBuffer combinedBuffer = accumulator.takeRetainableByteBuffer(); assertThat(bufferPool.getAcquireCount(), is(1L)); - assertThat(accumulator.getLength(), is(0L)); + assertThat(accumulator.getLength(), is(0)); accumulator.close(); assertThat(bufferPool.getAcquireCount(), is(1L)); assertThat(combinedBuffer.getByteBuffer(), is(content)); @@ -154,17 +153,17 @@ public void testToByteArray() // Copy the content. while (slice.hasRemaining()) { - RetainableByteBuffer internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); + RetainableByteBuffer internalBuffer = accumulator.ensureBuffer(1, allocationSize); ByteBuffer byteBuffer = internalBuffer.getByteBuffer(); writeInFlushMode(slice, byteBuffer); } // Check we have the same content as the original buffer. - assertThat(accumulator.getLength(), is((long)size)); + assertThat(accumulator.getLength(), is(size)); assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); byte[] combinedBuffer = accumulator.toByteArray(); assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); - assertThat(accumulator.getLength(), is((long)size)); + assertThat(accumulator.getLength(), is(size)); assertThat(BufferUtil.toBuffer(combinedBuffer), is(content)); // Close the accumulator and make sure all is returned to bufferPool. @@ -177,7 +176,7 @@ public void testEmptyToBuffer() { RetainableByteBuffer combinedBuffer = accumulator.toRetainableByteBuffer(); assertThat(combinedBuffer.remaining(), is(0)); - assertThat(bufferPool.getAcquireCount(), is(0L)); + assertThat(bufferPool.getAcquireCount(), is(1L)); accumulator.close(); assertThat(bufferPool.getAcquireCount(), is(0L)); } @@ -188,7 +187,7 @@ public void testEmptyTakeBuffer() RetainableByteBuffer combinedBuffer = accumulator.takeRetainableByteBuffer(); assertThat(combinedBuffer.remaining(), is(0)); accumulator.close(); - assertThat(bufferPool.getAcquireCount(), is(0L)); + assertThat(bufferPool.getAcquireCount(), is(1L)); combinedBuffer.release(); assertThat(bufferPool.getAcquireCount(), is(0L)); } @@ -204,16 +203,16 @@ public void testWriteTo() // Copy the content. while (slice.hasRemaining()) { - RetainableByteBuffer internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); + RetainableByteBuffer internalBuffer = accumulator.ensureBuffer(1, allocationSize); ByteBuffer byteBuffer = internalBuffer.getByteBuffer(); writeInFlushMode(slice, byteBuffer); } // Check we have the same content as the original buffer. assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); - RetainableByteBuffer combinedBuffer = bufferPool.acquire((int)accumulator.getLength(), false); + RetainableByteBuffer combinedBuffer = bufferPool.acquire(accumulator.getLength(), false); accumulator.writeTo(combinedBuffer.getByteBuffer()); - assertThat(accumulator.getLength(), is((long)size)); + assertThat(accumulator.getLength(), is(size)); assertThat(combinedBuffer.getByteBuffer(), is(content)); combinedBuffer.release(); @@ -233,14 +232,14 @@ public void testWriteToBufferTooSmall() // Copy the content. while (slice.hasRemaining()) { - RetainableByteBuffer internalBuffer = accumulator.accessInternalBuffer(1, allocationSize); + RetainableByteBuffer internalBuffer = accumulator.ensureBuffer(1, allocationSize); ByteBuffer byteBuffer = internalBuffer.getByteBuffer(); writeInFlushMode(slice, byteBuffer); } // Writing to a buffer too small gives buffer overflow. assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); - ByteBuffer combinedBuffer = BufferUtil.toBuffer(new byte[(int)accumulator.getLength() - 1]); + ByteBuffer combinedBuffer = BufferUtil.toBuffer(new byte[accumulator.getLength() - 1]); BufferUtil.clear(combinedBuffer); assertThrows(BufferOverflowException.class, () -> accumulator.writeTo(combinedBuffer)); @@ -268,9 +267,9 @@ public void testCopy() // Check we have the same content as the original buffer. assertThat(bufferPool.getAcquireCount(), greaterThan(1L)); - RetainableByteBuffer combinedBuffer = bufferPool.acquire((int)accumulator.getLength(), false); + RetainableByteBuffer combinedBuffer = bufferPool.acquire(accumulator.getLength(), false); accumulator.writeTo(combinedBuffer.getByteBuffer()); - assertThat(accumulator.getLength(), is((long)size)); + assertThat(accumulator.getLength(), is(size)); assertThat(combinedBuffer.getByteBuffer(), is(content)); combinedBuffer.release(); @@ -279,29 +278,6 @@ public void testCopy() assertThat(bufferPool.getAcquireCount(), is(0L)); } - @Test - public void testAdd() - { - Retainable.ReferenceCounter counter = new Retainable.ReferenceCounter(); - RetainableByteBuffer buffer = RetainableByteBuffer.wrap(BufferUtil.toBuffer("Ground Hog Day\n"), counter); - accumulator.addBuffer(buffer); - assertTrue(buffer.isRetained()); - accumulator.addBuffer(buffer); - accumulator.addBuffer(buffer); - accumulator.addBuffer(buffer); - - ByteBuffer taken = accumulator.takeByteBuffer(); - assertFalse(buffer.isRetained()); - assertTrue(buffer.release()); - - assertThat(BufferUtil.toString(taken), is(""" - Ground Hog Day - Ground Hog Day - Ground Hog Day - Ground Hog Day - """)); - } - private ByteBuffer randomBytes(int size) { byte[] data = new byte[size]; @@ -348,42 +324,4 @@ public long getAcquireCount() return _acquires.get(); } } - - @Test - public void testAggregateFullInSingleShot() - { - try (ByteBufferAccumulator accumulator = new ByteBufferAccumulator(bufferPool, true, 16)) - { - ByteBuffer byteBuffer1 = ByteBuffer.wrap(new byte[16]); - assertThat(accumulator.aggregate(byteBuffer1), is(true)); - assertThat(byteBuffer1.remaining(), is(0)); - - ByteBuffer byteBuffer2 = ByteBuffer.wrap(new byte[16]); - assertThat(accumulator.aggregate(byteBuffer2), is(true)); - assertThat(byteBuffer2.remaining(), is(16)); - - RetainableByteBuffer retainableByteBuffer = accumulator.takeRetainableByteBuffer(); - assertThat(retainableByteBuffer.getByteBuffer().remaining(), is(16)); - assertThat(retainableByteBuffer.release(), is(true)); - } - } - - @Test - public void testAggregateFullInMultipleShots() - { - try (ByteBufferAccumulator accumulator = new ByteBufferAccumulator(bufferPool, true, 16)) - { - ByteBuffer byteBuffer1 = ByteBuffer.wrap(new byte[15]); - assertThat(accumulator.aggregate(byteBuffer1), is(false)); - assertThat(byteBuffer1.remaining(), is(0)); - - ByteBuffer byteBuffer2 = ByteBuffer.wrap(new byte[16]); - assertThat(accumulator.aggregate(byteBuffer2), is(true)); - assertThat(byteBuffer2.remaining(), is(15)); - - RetainableByteBuffer retainableByteBuffer = accumulator.takeRetainableByteBuffer(); - assertThat(retainableByteBuffer.getByteBuffer().remaining(), is(16)); - assertThat(retainableByteBuffer.release(), is(true)); - } - } } From 3970a3a4c0a758395d4dad5918b7ff0a89fee852 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 5 Mar 2024 14:35:34 +0100 Subject: [PATCH 25/43] More updates after review --- .../org/eclipse/jetty/io/ByteBufferPool.java | 5 +- .../java/org/eclipse/jetty/io/Content.java | 14 +++ .../jetty/io/RetainableByteBuffer.java | 102 +++++++++++++++--- .../io/internal/ContentSourceByteBuffer.java | 8 +- .../ContentSourceRetainableByteBuffer.java | 64 +++++++++++ .../eclipse/jetty/io/ContentSourceTest.java | 40 +++++++ .../jetty/io/RetainableByteBufferTest.java | 42 ++++++++ 7 files changed, 258 insertions(+), 17 deletions(-) create mode 100644 jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java index f9d6965c2bc6..a46ea1f23dc0 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java @@ -127,12 +127,15 @@ private Buffer(ByteBuffer byteBuffer) /** *

Accumulates a sequence of {@link RetainableByteBuffer} that - * are typically created during the generation of protocol bytes.

+ * are typically created during the generation of protocol bytes. + * The accumulated buffers are then used individually rather than + * as a single buffer (like {@link RetainableByteBuffer.Accumulator}.

*

{@code RetainableByteBuffer}s can be either * {@link #append(RetainableByteBuffer) appended} to the sequence, * or {@link #insert(int, RetainableByteBuffer) inserted} at a * specific position in the sequence, and then * {@link #release() released} when they are consumed.

+ * @see RetainableByteBuffer.Accumulator */ class Accumulator { diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index e238f1220742..035b9ee43873 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -33,6 +33,7 @@ import org.eclipse.jetty.io.internal.ContentCopier; import org.eclipse.jetty.io.internal.ContentSourceByteBuffer; import org.eclipse.jetty.io.internal.ContentSourceConsumer; +import org.eclipse.jetty.io.internal.ContentSourceRetainableByteBuffer; import org.eclipse.jetty.io.internal.ContentSourceString; import org.eclipse.jetty.util.Blocker; import org.eclipse.jetty.util.BufferUtil; @@ -161,6 +162,19 @@ static void asByteBuffer(Source source, Promise promise) new ContentSourceByteBuffer(source, promise).run(); } + /** + *

Reads, non-blocking, the whole content source into a {@link ByteBuffer}.

+ * + * @param source the source to read + * @param promise the promise to notify when the whole content has been read into a RetainableByteBuffer. + * The consumer of the promise must {@link RetainableByteBuffer#retain() retain} the buffer + * if it is to be held beyond the call to {@link Promise#succeeded(Object)}. + */ + static void asRetainableByteBuffer(Source source, Promise promise) + { + new ContentSourceRetainableByteBuffer(source, promise).run(); + } + /** *

Reads, blocking if necessary, the whole content source into a {@link ByteBuffer}.

* diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 8a0c4f74f6eb..f4e65a0b57e6 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -20,7 +20,7 @@ import org.eclipse.jetty.io.internal.NonRetainableByteBuffer; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; -import org.eclipse.jetty.util.IteratingCallback; +import org.eclipse.jetty.util.IteratingNestedCallback; /** *

A pooled {@link ByteBuffer} which maintains a reference count that is @@ -123,6 +123,10 @@ static RetainableByteBuffer wrap(ByteBuffer byteBuffer, Runnable releaser) { return new AbstractRetainableByteBuffer(byteBuffer) { + { + acquire(); + } + @Override public boolean release() { @@ -140,6 +144,19 @@ public boolean release() */ ByteBuffer getByteBuffer(); + /** + * @return A copy of this RetainableByteBuffer that is entirely independent + */ + default RetainableByteBuffer copy() + { + return new AbstractRetainableByteBuffer(BufferUtil.copy(getByteBuffer())) + { + { + acquire(); + } + }; + } + /** * @return whether the {@code ByteBuffer} is direct */ @@ -309,6 +326,30 @@ public ByteBuffer getByteBuffer() return _buffer.getByteBuffer(); } + @Override + public RetainableByteBuffer copy() + { + RetainableByteBuffer buffer = _buffer; + buffer.retain(); + return new AbstractRetainableByteBuffer(buffer.getByteBuffer().slice()) + { + { + acquire(); + } + + @Override + public boolean release() + { + if (super.release()) + { + buffer.release(); + return true; + } + return false; + } + }; + } + @Override public int capacity() { @@ -357,6 +398,7 @@ private void ensureSpace(int spaceNeeded) */ class Accumulator implements RetainableByteBuffer { + private final ReferenceCounter _retainable = new ReferenceCounter(1); private final ByteBufferPool _pool; private final boolean _direct; private final long _maxLength; @@ -384,27 +426,37 @@ public ByteBuffer getByteBuffer() { case 0 -> RetainableByteBuffer.EMPTY.getByteBuffer(); case 1 -> _buffers.get(0).getByteBuffer(); - default -> copyBuffer(); + default -> + { + int length = remaining(); + RetainableByteBuffer combinedBuffer = _pool.acquire(length, _direct); + ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); + BufferUtil.flipToFill(byteBuffer); + for (RetainableByteBuffer buffer : _buffers) + { + buffer.putTo(byteBuffer); + buffer.clear(); + buffer.release(); + } + BufferUtil.flipToFlush(byteBuffer, 0); + _buffers.clear(); + _buffers.add(combinedBuffer); + _canAggregate = true; + yield combinedBuffer.getByteBuffer(); + } }; } - public ByteBuffer copyBuffer() + public RetainableByteBuffer copy() { int length = remaining(); RetainableByteBuffer combinedBuffer = _pool.acquire(length, _direct); ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); BufferUtil.flipToFill(byteBuffer); for (RetainableByteBuffer buffer : _buffers) - { buffer.putTo(byteBuffer); - buffer.clear(); - buffer.release(); - } BufferUtil.flipToFlush(byteBuffer, 0); - _buffers.clear(); - _buffers.add(combinedBuffer); - _canAggregate = true; - return combinedBuffer.getByteBuffer(); + return combinedBuffer; } /** @@ -440,11 +492,33 @@ public long capacityLong() return _maxLength; } + @Override + public boolean canRetain() + { + return _retainable.canRetain(); + } + + @Override + public boolean isRetained() + { + return _retainable.isRetained(); + } + + @Override + public void retain() + { + _retainable.retain(); + } + @Override public boolean release() { - clear(); - return true; + if (_retainable.release()) + { + clear(); + return true; + } + return false; } @Override @@ -524,7 +598,7 @@ public void writeTo(Content.Sink sink, boolean last, Callback callback) { case 0 -> callback.succeeded(); case 1 -> _buffers.get(0).writeTo(sink, last, callback); - default -> new IteratingCallback() + default -> new IteratingNestedCallback(callback) { private int i = 0; diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java index fa5b3d420a47..79936a6c3e0f 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java @@ -57,9 +57,13 @@ public void run() if (chunk.isLast()) { - ByteBuffer buffer = accumulator.copyBuffer(); + RetainableByteBuffer copy = accumulator.copy(); accumulator.release(); - promise.succeeded(buffer); + + // We know the accumulator is not using a pool, so whilst we release after succeeded, it is safe + // for the promise to retain the ByteBuffer after the call. + promise.succeeded(copy.getByteBuffer()); + copy.release(); return; } } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java new file mode 100644 index 000000000000..0f12ad7ccaf2 --- /dev/null +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java @@ -0,0 +1,64 @@ +// +// ======================================================================== +// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.io.internal; + +import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.io.RetainableByteBuffer; +import org.eclipse.jetty.util.Promise; + +public class ContentSourceRetainableByteBuffer implements Runnable +{ + private final RetainableByteBuffer.Accumulator accumulator = new RetainableByteBuffer.Accumulator(null, false, -1); + private final Content.Source source; + private final Promise promise; + + public ContentSourceRetainableByteBuffer(Content.Source source, Promise promise) + { + this.source = source; + this.promise = promise; + } + + @Override + public void run() + { + while (true) + { + Content.Chunk chunk = source.read(); + + if (chunk == null) + { + source.demand(this); + return; + } + + if (Content.Chunk.isFailure(chunk)) + { + promise.failed(chunk.getFailure()); + if (!chunk.isLast()) + source.fail(chunk.getFailure()); + return; + } + + accumulator.append(chunk); + chunk.release(); + + if (chunk.isLast()) + { + promise.succeeded(accumulator); + accumulator.release(); + return; + } + } + } +} diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java index 45799c4f1fcc..6522f68682c3 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java @@ -715,4 +715,44 @@ public void testAsyncContentWithWarningsAsInputStream() throws Exception len = in.read(buffer); assertThat(len, is(-1)); } + + @Test + public void testAsRetainableByteBuffer() throws Exception + { + TestContentSource source = new TestContentSource(); + + FuturePromise promise = new FuturePromise<>() + { + @Override + public void succeeded(RetainableByteBuffer result) + { + result.retain(); + super.succeeded(result); + } + }; + Content.Source.asRetainableByteBuffer(source, promise); + + Retainable.ReferenceCounter counter = new Retainable.ReferenceCounter(3); + + Runnable todo = source.takeDemand(); + assertNotNull(todo); + source.add(Content.Chunk.asChunk(BufferUtil.toBuffer("hello"), false, counter)); + todo.run(); + assertFalse(promise.isDone()); + + todo = source.takeDemand(); + assertNotNull(todo); + source.add(Content.Chunk.asChunk(BufferUtil.toBuffer(" cruel"), false, counter)); + source.add(Content.Chunk.asChunk(BufferUtil.toBuffer(" world"), true, counter)); + todo.run(); + + todo = source.takeDemand(); + assertNull(todo); + assertTrue(promise.isDone()); + + RetainableByteBuffer buffer = promise.get(); + assertNotNull(buffer); + + assertThat(BufferUtil.toString(buffer.getByteBuffer()), equalTo("hello cruel world")); + } } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java index 6cbe77240b7f..9ecd6250b6be 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java @@ -13,11 +13,16 @@ package org.eclipse.jetty.io; +import java.io.ByteArrayOutputStream; import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.stream.Stream; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.FutureCallback; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.params.ParameterizedTest; @@ -138,4 +143,41 @@ public void testAppendBigByteBuffer(RetainableByteBuffer buffer) assertTrue(buffer.isFull()); buffer.release(); } + + @ParameterizedTest + @MethodSource("buffers") + public void testNonRetainableWriteTo(RetainableByteBuffer buffer) throws Exception + { + buffer.append(RetainableByteBuffer.wrap(BufferUtil.toBuffer("Hello"))); + buffer.append(RetainableByteBuffer.wrap(BufferUtil.toBuffer(" "))); + buffer.append(RetainableByteBuffer.wrap(BufferUtil.toBuffer("World!"))); + + ByteArrayOutputStream out = new ByteArrayOutputStream(); + FutureCallback callback = new FutureCallback(); + buffer.writeTo(Content.Sink.from(out), true, callback); + callback.get(5, TimeUnit.SECONDS); + assertThat(out.toString(StandardCharsets.ISO_8859_1), is("Hello World!")); + buffer.release(); + } + + @ParameterizedTest + @MethodSource("buffers") + public void testRetainableWriteTo(RetainableByteBuffer buffer) throws Exception + { + CountDownLatch released = new CountDownLatch(3); + RetainableByteBuffer[] buffers = new RetainableByteBuffer[3]; + buffer.append(buffers[0] = RetainableByteBuffer.wrap(BufferUtil.toBuffer("Hello"), released::countDown)); + buffer.append(buffers[1] = RetainableByteBuffer.wrap(BufferUtil.toBuffer(" "), released::countDown)); + buffer.append(buffers[2] = RetainableByteBuffer.wrap(BufferUtil.toBuffer("World!"), released::countDown)); + Arrays.asList(buffers).forEach(RetainableByteBuffer::release); + + ByteArrayOutputStream out = new ByteArrayOutputStream(); + FutureCallback callback = new FutureCallback(); + buffer.writeTo(Content.Sink.from(out), true, callback); + callback.get(5, TimeUnit.SECONDS); + assertThat(out.toString(StandardCharsets.ISO_8859_1), is("Hello World!")); + + buffer.release(); + assertTrue(released.await(5, TimeUnit.SECONDS)); + } } From 22ec9b08fc2faa75aea7eb65df37d0e3254342a8 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 8 Mar 2024 10:01:52 +0100 Subject: [PATCH 26/43] updated after merge --- .../jetty/http2/tests/RawHTTP2ProxyTest.java | 13 ++++--------- .../core/messages/ByteArrayMessageSink.java | 17 +++++++++-------- .../core/messages/ByteBufferMessageSink.java | 15 +++++++-------- .../extensions/PermessageDeflateDemandTest.java | 13 ++++++++----- 4 files changed, 28 insertions(+), 30 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java index c0b87ed22cb2..c58570c1459b 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java @@ -44,7 +44,6 @@ import org.eclipse.jetty.http2.frames.ResetFrame; import org.eclipse.jetty.http2.server.RawHTTP2ServerConnectionFactory; import org.eclipse.jetty.io.ArrayByteBufferPool; -import org.eclipse.jetty.io.ByteBufferAccumulator; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.Server; @@ -63,8 +62,6 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; public class RawHTTP2ProxyTest @@ -245,7 +242,7 @@ public void onDataAvailable(Stream stream) CountDownLatch latch1 = new CountDownLatch(1); Stream stream1 = clientSession.newStream(new HeadersFrame(request1, null, false), new Stream.Listener() { - private final ByteBufferAccumulator accumulator = new ByteBufferAccumulator(client.getByteBufferPool(), true, data1.length * 2); + private final RetainableByteBuffer.Aggregator aggregator = new RetainableByteBuffer.Aggregator(client.getByteBufferPool(), true, data1.length * 2); @Override public void onHeaders(Stream stream, HeadersFrame frame) @@ -262,17 +259,15 @@ public void onDataAvailable(Stream stream) DataFrame frame = data.frame(); if (LOGGER.isDebugEnabled()) LOGGER.debug("CLIENT1 received {}", frame); - assertFalse(accumulator.aggregate(frame.getByteBuffer())); + assertTrue(aggregator.append(frame.getByteBuffer())); data.release(); if (!data.frame().isEndStream()) { stream.demand(); return; } - RetainableByteBuffer buffer = accumulator.takeRetainableByteBuffer(); - assertNotNull(buffer); - assertEquals(buffer1.slice(), buffer.getByteBuffer()); - buffer.release(); + assertEquals(buffer1.slice(), aggregator.getByteBuffer()); + aggregator.release(); latch1.countDown(); } }).get(5, TimeUnit.SECONDS); diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java index 1d24ed0edc76..13182b8097ec 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java @@ -17,7 +17,7 @@ import java.lang.invoke.MethodType; import java.nio.ByteBuffer; -import org.eclipse.jetty.io.ByteBufferAccumulator; +import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.websocket.core.CoreSession; @@ -32,7 +32,7 @@ */ public class ByteArrayMessageSink extends AbstractMessageSink { - private ByteBufferAccumulator accumulator; + private RetainableByteBuffer.Accumulator accumulator; /** * Creates a new {@link ByteArrayMessageSink}. @@ -56,7 +56,7 @@ public void accept(Frame frame, Callback callback) { try { - long size = (accumulator == null ? 0 : accumulator.getLength()) + frame.getPayloadLength(); + long size = (accumulator == null ? 0 : accumulator.remaining()) + frame.getPayloadLength(); long maxSize = getCoreSession().getMaxBinaryMessageSize(); if (maxSize > 0 && size > maxSize) { @@ -67,7 +67,7 @@ public void accept(Frame frame, Callback callback) // If the frame is fin and no accumulator has been // created or used, then we don't need to aggregate. ByteBuffer payload = frame.getPayload(); - if (frame.isFin() && (accumulator == null || accumulator.getLength() == 0)) + if (frame.isFin() && (accumulator == null || accumulator.remaining() == 0)) { byte[] buf = BufferUtil.toArray(payload); getMethodHandle().invoke(buf, 0, buf.length); @@ -84,14 +84,15 @@ public void accept(Frame frame, Callback callback) } if (accumulator == null) - accumulator = new ByteBufferAccumulator(); - accumulator.addBuffer(payload, callback); + accumulator = new RetainableByteBuffer.Accumulator(getCoreSession().getByteBufferPool(), false, maxSize); + accumulator.append(RetainableByteBuffer.wrap(payload, callback::succeeded)); if (frame.isFin()) { // Do not complete twice the callback if the invocation fails. callback = Callback.NOOP; - byte[] buf = accumulator.takeByteArray(); + byte[] buf = BufferUtil.toArray(accumulator.getByteBuffer()); + accumulator.release(); getMethodHandle().invoke(buf, 0, buf.length); autoDemand(); } @@ -111,6 +112,6 @@ public void accept(Frame frame, Callback callback) public void fail(Throwable failure) { if (accumulator != null) - accumulator.fail(failure); + accumulator.clear(); } } diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java index db7712662467..126d939b78dd 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java @@ -17,7 +17,6 @@ import java.lang.invoke.MethodType; import java.nio.ByteBuffer; -import org.eclipse.jetty.io.ByteBufferAccumulator; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.websocket.core.CoreSession; @@ -32,7 +31,7 @@ */ public class ByteBufferMessageSink extends AbstractMessageSink { - private ByteBufferAccumulator accumulator; + private RetainableByteBuffer accumulator; /** * Creates a new {@link ByteBufferMessageSink}. @@ -63,7 +62,7 @@ public void accept(Frame frame, Callback callback) { try { - int size = Math.addExact(accumulator == null ? 0 : Math.toIntExact(accumulator.getLength()), frame.getPayloadLength()); + int size = Math.addExact(accumulator == null ? 0 : Math.toIntExact(accumulator.remaining()), frame.getPayloadLength()); long maxSize = getCoreSession().getMaxBinaryMessageSize(); if (maxSize > 0 && size > maxSize) { @@ -73,7 +72,7 @@ public void accept(Frame frame, Callback callback) // If the frame is fin and no accumulator has been // created or used, then we don't need to aggregate. - if (frame.isFin() && (accumulator == null || accumulator.getLength() == 0)) + if (frame.isFin() && (accumulator == null || accumulator.remaining() == 0)) { invoke(getMethodHandle(), frame.getPayload(), callback); autoDemand(); @@ -88,12 +87,12 @@ public void accept(Frame frame, Callback callback) } if (accumulator == null) - accumulator = new ByteBufferAccumulator(); - accumulator.addBuffer(frame.getPayload(), callback); + accumulator = new RetainableByteBuffer.Accumulator(getCoreSession().getByteBufferPool(), false, maxSize); + accumulator.append(RetainableByteBuffer.wrap(frame.getPayload(), callback::succeeded)); if (frame.isFin()) { - RetainableByteBuffer buffer = accumulator.toRetainableByteBuffer(); + RetainableByteBuffer buffer = accumulator; callback = Callback.from(buffer::release); invoke(getMethodHandle(), buffer.getByteBuffer(), callback); autoDemand(); @@ -115,7 +114,7 @@ public void accept(Frame frame, Callback callback) public void fail(Throwable failure) { if (accumulator != null) - accumulator.fail(failure); + accumulator.clear(); } protected void invoke(MethodHandle methodHandle, ByteBuffer byteBuffer, Callback callback) throws Throwable diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java b/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java index 41cafb9552d2..b099592005b4 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java @@ -19,7 +19,7 @@ import java.util.concurrent.BlockingQueue; import java.util.concurrent.TimeUnit; -import org.eclipse.jetty.io.ByteBufferAccumulator; +import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.util.BlockingArrayQueue; @@ -113,7 +113,7 @@ public static class ServerHandler implements FrameHandler public BlockingQueue textMessages = new BlockingArrayQueue<>(); public BlockingQueue binaryMessages = new BlockingArrayQueue<>(); private StringBuilder _stringBuilder = new StringBuilder(); - private ByteBufferAccumulator _byteBuilder = new ByteBufferAccumulator(); + private final RetainableByteBuffer.Accumulator _byteBuilder = new RetainableByteBuffer.Accumulator(_coreSession.getByteBufferPool(), false, -1); @Override public void onOpen(CoreSession coreSession, Callback callback) @@ -154,11 +154,12 @@ public void onFrame(Frame frame, Callback callback) } break; case OpCode.BINARY: - _byteBuilder.addBuffer(frame.getPayload(), callback); + _byteBuilder.append(RetainableByteBuffer.wrap(frame.getPayload(), callback::succeeded)); if (frame.isFin()) { - binaryMessages.add(BufferUtil.toBuffer(_byteBuilder.takeByteArray())); - _byteBuilder = new ByteBufferAccumulator(); + // TODO this looks wrong + binaryMessages.add(ByteBuffer.wrap(BufferUtil.toArray(_byteBuilder.getByteBuffer()))); + _byteBuilder.clear(); } break; default: @@ -177,12 +178,14 @@ public void onFrame(Frame frame, Callback callback) public void onError(Throwable cause, Callback callback) { cause.printStackTrace(); + _byteBuilder.clear(); callback.succeeded(); } @Override public void onClosed(CloseStatus closeStatus, Callback callback) { + _byteBuilder.clear(); callback.succeeded(); } } From 10dd573e899314b985e3139aa6bc117e4954138e Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 8 Mar 2024 11:00:12 +0100 Subject: [PATCH 27/43] updated after merge --- .../websocket/core/messages/ByteArrayMessageSink.java | 7 ++++++- .../core/extensions/PermessageDeflateDemandTest.java | 9 ++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java index 13182b8097ec..5ee9d30e8086 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java @@ -17,8 +17,10 @@ import java.lang.invoke.MethodType; import java.nio.ByteBuffer; +import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.ByteArrayOutputStream2; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.websocket.core.CoreSession; import org.eclipse.jetty.websocket.core.Frame; @@ -91,7 +93,10 @@ public void accept(Frame frame, Callback callback) { // Do not complete twice the callback if the invocation fails. callback = Callback.NOOP; - byte[] buf = BufferUtil.toArray(accumulator.getByteBuffer()); + + ByteArrayOutputStream2 bout = new ByteArrayOutputStream2(accumulator.remaining()); + accumulator.writeTo(Content.Sink.from(bout), true, Callback.NOOP); + byte[] buf = bout.getBuf(); accumulator.release(); getMethodHandle().invoke(buf, 0, buf.length); autoDemand(); diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java b/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java index b099592005b4..4f5a9edcc82a 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java @@ -113,12 +113,13 @@ public static class ServerHandler implements FrameHandler public BlockingQueue textMessages = new BlockingArrayQueue<>(); public BlockingQueue binaryMessages = new BlockingArrayQueue<>(); private StringBuilder _stringBuilder = new StringBuilder(); - private final RetainableByteBuffer.Accumulator _byteBuilder = new RetainableByteBuffer.Accumulator(_coreSession.getByteBufferPool(), false, -1); + private RetainableByteBuffer.Accumulator _byteBuilder; @Override public void onOpen(CoreSession coreSession, Callback callback) { _coreSession = coreSession; + _byteBuilder = new RetainableByteBuffer.Accumulator(_coreSession.getByteBufferPool(), false, -1); callback.succeeded(); coreSession.demand(); } @@ -178,14 +179,16 @@ public void onFrame(Frame frame, Callback callback) public void onError(Throwable cause, Callback callback) { cause.printStackTrace(); - _byteBuilder.clear(); + if (_byteBuilder != null) + _byteBuilder.clear(); callback.succeeded(); } @Override public void onClosed(CloseStatus closeStatus, Callback callback) { - _byteBuilder.clear(); + if (_byteBuilder != null) + _byteBuilder.clear(); callback.succeeded(); } } From 270777706f72df921a10d4e3df4f8fff033e997d Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 8 Mar 2024 11:55:44 +0100 Subject: [PATCH 28/43] replace ChunkAccumulator usages with ContentSourceRetainableByteBuffer and add tests Signed-off-by: Ludovic Orban --- .../eclipse/jetty/io/ChunkAccumulator.java | 2 + .../java/org/eclipse/jetty/io/Content.java | 41 +++++++---- .../ContentSourceRetainableByteBuffer.java | 6 +- .../eclipse/jetty/io/ContentSourceTest.java | 68 ++++++++++++++++++- 4 files changed, 98 insertions(+), 19 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java index 0358f8583969..c693bd68e20a 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java @@ -27,7 +27,9 @@ /** * An accumulator of {@link Content.Chunk}s used to facilitate minimal copy * aggregation of multiple chunks. + * @deprecated use {@link Content.Source#asRetainableByteBuffer(Content.Source, ByteBufferPool, boolean, int)} instead. */ +@Deprecated public class ChunkAccumulator { private static final ByteBufferPool NON_POOLING = new ByteBufferPool.NonPooling(); diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index 035b9ee43873..fd049e79a0bf 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -162,19 +162,6 @@ static void asByteBuffer(Source source, Promise promise) new ContentSourceByteBuffer(source, promise).run(); } - /** - *

Reads, non-blocking, the whole content source into a {@link ByteBuffer}.

- * - * @param source the source to read - * @param promise the promise to notify when the whole content has been read into a RetainableByteBuffer. - * The consumer of the promise must {@link RetainableByteBuffer#retain() retain} the buffer - * if it is to be held beyond the call to {@link Promise#succeeded(Object)}. - */ - static void asRetainableByteBuffer(Source source, Promise promise) - { - new ContentSourceRetainableByteBuffer(source, promise).run(); - } - /** *

Reads, blocking if necessary, the whole content source into a {@link ByteBuffer}.

* @@ -206,7 +193,7 @@ static ByteBuffer asByteBuffer(Source source) throws IOException */ static CompletableFuture asByteArrayAsync(Source source, int maxSize) { - return new ChunkAccumulator().readAll(source, maxSize); + return asRetainableByteBuffer(source, null, false, maxSize).thenApply(rbb -> rbb.getByteBuffer().array()); } /** @@ -244,7 +231,31 @@ static CompletableFuture asByteBufferAsync(Source source, int maxSiz */ static CompletableFuture asRetainableByteBuffer(Source source, ByteBufferPool pool, boolean direct, int maxSize) { - return new ChunkAccumulator().readAll(source, pool, direct, maxSize); + Promise.Completable promise = new Promise.Completable<>() + { + @Override + public void succeeded(RetainableByteBuffer result) + { + result.retain(); + super.succeeded(result); + } + }; + asRetainableByteBuffer(source, pool, direct, maxSize, promise); + return promise; + } + + /** + *

Reads, non-blocking, the whole content source into a {@link RetainableByteBuffer}.

+ * + * @param source the source to read + * @param pool The {@link ByteBufferPool} to acquire the buffer from, or null for a non {@link Retainable} buffer + * @param direct True if the buffer should be direct. + * @param maxSize The maximum size to read, or -1 for no limit + * @param promise the promise to notify when the whole content has been read into a RetainableByteBuffer. + */ + static void asRetainableByteBuffer(Source source, ByteBufferPool pool, boolean direct, int maxSize, Promise promise) + { + new ContentSourceRetainableByteBuffer(source, pool, direct, maxSize, promise).run(); } /** diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java index 0f12ad7ccaf2..8336b9dd164c 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java @@ -13,19 +13,21 @@ package org.eclipse.jetty.io.internal; +import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.util.Promise; public class ContentSourceRetainableByteBuffer implements Runnable { - private final RetainableByteBuffer.Accumulator accumulator = new RetainableByteBuffer.Accumulator(null, false, -1); + private final RetainableByteBuffer.Accumulator accumulator; private final Content.Source source; private final Promise promise; - public ContentSourceRetainableByteBuffer(Content.Source source, Promise promise) + public ContentSourceRetainableByteBuffer(Content.Source source, ByteBufferPool pool, boolean direct, int maxSize, Promise promise) { this.source = source; + this.accumulator = new RetainableByteBuffer.Accumulator(pool, direct, maxSize); this.promise = promise; } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java index 6522f68682c3..7b21e0d7108c 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java @@ -25,6 +25,7 @@ import java.util.Deque; import java.util.List; import java.util.concurrent.CancellationException; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentLinkedDeque; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; @@ -717,7 +718,7 @@ public void testAsyncContentWithWarningsAsInputStream() throws Exception } @Test - public void testAsRetainableByteBuffer() throws Exception + public void testAsRetainableByteBufferWithPromise() throws Exception { TestContentSource source = new TestContentSource(); @@ -730,7 +731,7 @@ public void succeeded(RetainableByteBuffer result) super.succeeded(result); } }; - Content.Source.asRetainableByteBuffer(source, promise); + Content.Source.asRetainableByteBuffer(source, null, false, -1, promise); Retainable.ReferenceCounter counter = new Retainable.ReferenceCounter(3); @@ -755,4 +756,67 @@ public void succeeded(RetainableByteBuffer result) assertThat(BufferUtil.toString(buffer.getByteBuffer()), equalTo("hello cruel world")); } + + @Test + public void testAsRetainableByteBufferWithCompletableFuture() throws Exception + { + TestContentSource source = new TestContentSource(); + + CompletableFuture completableFuture = Content.Source.asRetainableByteBuffer(source, null, false, -1); + + Retainable.ReferenceCounter counter = new Retainable.ReferenceCounter(3); + + Runnable todo = source.takeDemand(); + assertNotNull(todo); + source.add(Content.Chunk.asChunk(BufferUtil.toBuffer("hello"), false, counter)); + todo.run(); + assertFalse(completableFuture.isDone()); + + todo = source.takeDemand(); + assertNotNull(todo); + source.add(Content.Chunk.asChunk(BufferUtil.toBuffer(" cruel"), false, counter)); + source.add(Content.Chunk.asChunk(BufferUtil.toBuffer(" world"), true, counter)); + todo.run(); + + todo = source.takeDemand(); + assertNull(todo); + assertTrue(completableFuture.isDone()); + + RetainableByteBuffer buffer = completableFuture.get(); + assertNotNull(buffer); + + assertThat(BufferUtil.toString(buffer.getByteBuffer()), equalTo("hello cruel world")); + } + + @Test + public void testAsByteArrayAsync() throws Exception + { + TestContentSource source = new TestContentSource(); + + CompletableFuture completableFuture = Content.Source.asByteArrayAsync(source, -1); + + Retainable.ReferenceCounter counter = new Retainable.ReferenceCounter(3); + + Runnable todo = source.takeDemand(); + assertNotNull(todo); + source.add(Content.Chunk.asChunk(BufferUtil.toBuffer("hello"), false, counter)); + todo.run(); + assertFalse(completableFuture.isDone()); + + todo = source.takeDemand(); + assertNotNull(todo); + source.add(Content.Chunk.asChunk(BufferUtil.toBuffer(" cruel"), false, counter)); + source.add(Content.Chunk.asChunk(BufferUtil.toBuffer(" world"), true, counter)); + todo.run(); + + todo = source.takeDemand(); + assertNull(todo); + assertTrue(completableFuture.isDone()); + + byte[] buffer = completableFuture.get(); + assertNotNull(buffer); + + assertThat(new String(buffer, UTF_8), equalTo("hello cruel world")); + + } } From e06c10df8fa701302b10c915f591f0c750466918 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 8 Mar 2024 12:10:34 +0100 Subject: [PATCH 29/43] fix maxSize check, add test, add javadoc Signed-off-by: Ludovic Orban --- .../jetty/io/RetainableByteBuffer.java | 12 ++++++ .../ContentSourceRetainableByteBuffer.java | 11 +++++- .../eclipse/jetty/io/ContentSourceTest.java | 38 +++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index f4e65a0b57e6..1d1dadf06b45 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -207,12 +207,24 @@ default void clear() BufferUtil.clear(getByteBuffer()); } + /** + * Copies the contents of the given byte buffer at the end of this buffer. + * @param bytes the byte buffer to copy from. + * @return true if all bytes of the given buffer were copied, false otherwise. + * @see BufferUtil#append(ByteBuffer, ByteBuffer) + */ default boolean append(ByteBuffer bytes) { BufferUtil.append(getByteBuffer(), bytes); return !bytes.hasRemaining(); } + /** + * Copies the contents of the given retainable byte buffer at the end of this buffer. + * @param bytes the retainable byte buffer to copy from. + * @return true if all bytes of the given buffer were copied, false otherwise. + * @see BufferUtil#append(ByteBuffer, ByteBuffer) + */ default boolean append(RetainableByteBuffer bytes) { return bytes.remaining() == 0 || append(bytes.getByteBuffer()); diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java index 8336b9dd164c..65015cd2d64b 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceRetainableByteBuffer.java @@ -52,9 +52,18 @@ public void run() return; } - accumulator.append(chunk); + boolean appended = accumulator.append(chunk); chunk.release(); + if (!appended) + { + IllegalStateException ise = new IllegalStateException("Max size (" + accumulator.capacity() + ") exceeded"); + promise.failed(ise); + accumulator.release(); + source.fail(ise); + return; + } + if (chunk.isLast()) { promise.succeeded(accumulator); diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java index 7b21e0d7108c..76822febd09f 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java @@ -58,6 +58,7 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -655,6 +656,8 @@ public void demand(Runnable demandCallback) @Override public void fail(Throwable failure) { + _chunks.clear(); + _chunks.add(Content.Chunk.from(failure, true)); } } @@ -757,6 +760,41 @@ public void succeeded(RetainableByteBuffer result) assertThat(BufferUtil.toString(buffer.getByteBuffer()), equalTo("hello cruel world")); } + @Test + public void testAsRetainableByteBufferWithPromiseExceedsMaxSize() throws Exception + { + TestContentSource source = new TestContentSource(); + + FuturePromise promise = new FuturePromise<>() + { + @Override + public void succeeded(RetainableByteBuffer result) + { + result.retain(); + super.succeeded(result); + } + }; + Content.Source.asRetainableByteBuffer(source, null, false, 3, promise); + + Runnable todo = source.takeDemand(); + assertNotNull(todo); + source.add(Content.Chunk.asChunk(BufferUtil.toBuffer("hello"), false, new Retainable.ReferenceCounter(1))); + todo.run(); + assertTrue(promise.isDone()); + + try + { + promise.get(); + fail("expected ExecutionException"); + } + catch (ExecutionException e) + { + assertInstanceOf(IllegalStateException.class, e.getCause()); + } + + assertInstanceOf(IllegalStateException.class, source.read().getFailure()); + } + @Test public void testAsRetainableByteBufferWithCompletableFuture() throws Exception { From 1461ec62b3ea1f376a840bdda73187b86715b86b Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 8 Mar 2024 13:50:29 +0100 Subject: [PATCH 30/43] fix Content.Sink.from(OutputStream) and add test Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/io/Content.java | 35 ++++++++++---- .../jetty/io/BufferedContentSinkTest.java | 47 +++++++++++++++++++ 2 files changed, 72 insertions(+), 10 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index fd049e79a0bf..c68f465a8527 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -13,6 +13,7 @@ package org.eclipse.jetty.io; +import java.io.EOFException; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -502,18 +503,32 @@ public interface Sink */ static Sink from(OutputStream out) { - return (last, byteBuffer, callback) -> + return new Sink() { - try - { - BufferUtil.writeTo(byteBuffer, out); - if (last) - out.close(); - callback.succeeded(); - } - catch (Throwable t) + boolean closed; + + @Override + public void write(boolean last, ByteBuffer byteBuffer, Callback callback) { - callback.failed(t); + if (closed) + { + callback.failed(new EOFException()); + return; + } + try + { + BufferUtil.writeTo(byteBuffer, out); + if (last) + { + closed = true; + out.close(); + } + callback.succeeded(); + } + catch (Throwable t) + { + callback.failed(t); + } } }; } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java index b77fe6715323..0f4108f6343d 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/BufferedContentSinkTest.java @@ -13,9 +13,13 @@ package org.eclipse.jetty.io; +import java.io.ByteArrayOutputStream; +import java.io.EOFException; import java.io.IOException; import java.nio.ByteBuffer; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import java.util.Objects; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -34,9 +38,11 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; +import static java.nio.charset.StandardCharsets.US_ASCII; import static java.nio.charset.StandardCharsets.UTF_8; import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; @@ -601,4 +607,45 @@ public void succeeded() assertThat(count.get(), is(-1)); } } + + @Test + public void testFromOutputStream() + { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + Content.Sink sink = Content.Sink.from(baos); + + AccountingCallback accountingCallback = new AccountingCallback(); + + sink.write(false, ByteBuffer.wrap("hello ".getBytes(US_ASCII)), accountingCallback); + assertThat(accountingCallback.reports, equalTo(List.of("succeeded"))); + accountingCallback.reports.clear(); + + sink.write(true, ByteBuffer.wrap("world".getBytes(US_ASCII)), accountingCallback); + assertThat(accountingCallback.reports, equalTo(List.of("succeeded"))); + accountingCallback.reports.clear(); + + sink.write(true, ByteBuffer.wrap(" again".getBytes(US_ASCII)), accountingCallback); + assertThat(accountingCallback.reports.size(), is(1)); + assertThat(accountingCallback.reports.get(0), instanceOf(EOFException.class)); + accountingCallback.reports.clear(); + + assertThat(baos.toString(US_ASCII), is("hello world")); + } + + private static class AccountingCallback implements Callback + { + private final List reports = new ArrayList<>(); + + @Override + public void succeeded() + { + reports.add("succeeded"); + } + + @Override + public void failed(Throwable x) + { + reports.add(x); + } + } } From 07c8cbf12da324de9d6519b2dbdcb2e923669e97 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 8 Mar 2024 16:34:06 +0100 Subject: [PATCH 31/43] merged copy and getByteBuffer implementation --- .../jetty/io/RetainableByteBuffer.java | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 1d1dadf06b45..d227ef125cb5 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -440,34 +440,35 @@ public ByteBuffer getByteBuffer() case 1 -> _buffers.get(0).getByteBuffer(); default -> { - int length = remaining(); - RetainableByteBuffer combinedBuffer = _pool.acquire(length, _direct); - ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); - BufferUtil.flipToFill(byteBuffer); - for (RetainableByteBuffer buffer : _buffers) - { - buffer.putTo(byteBuffer); - buffer.clear(); - buffer.release(); - } - BufferUtil.flipToFlush(byteBuffer, 0); - _buffers.clear(); - _buffers.add(combinedBuffer); + RetainableByteBuffer combined = copy(true); + _buffers.add(combined); _canAggregate = true; - yield combinedBuffer.getByteBuffer(); + yield combined.getByteBuffer(); } }; } + @Override public RetainableByteBuffer copy() + { + return copy(false); + } + + private RetainableByteBuffer copy(boolean take) { int length = remaining(); RetainableByteBuffer combinedBuffer = _pool.acquire(length, _direct); ByteBuffer byteBuffer = combinedBuffer.getByteBuffer(); BufferUtil.flipToFill(byteBuffer); for (RetainableByteBuffer buffer : _buffers) + { buffer.putTo(byteBuffer); + if (take) + buffer.release(); + } BufferUtil.flipToFlush(byteBuffer, 0); + if (take) + _buffers.clear(); return combinedBuffer; } From 7dae7b7396f2fdb5ea71a628ff240ad5f93e4853 Mon Sep 17 00:00:00 2001 From: gregw Date: Sat, 9 Mar 2024 09:30:38 +0100 Subject: [PATCH 32/43] handle 0 capacity --- .../java/org/eclipse/jetty/io/RetainableByteBuffer.java | 8 ++++++-- .../src/main/java/org/eclipse/jetty/util/Jetty.java | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index d227ef125cb5..eca933ef7a39 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -428,7 +428,7 @@ public Accumulator(ByteBufferPool pool, boolean direct, long maxLength) { _pool = pool == null ? new ByteBufferPool.NonPooling() : pool; _direct = direct; - _maxLength = maxLength < 0 ? Integer.MAX_VALUE : maxLength; + _maxLength = maxLength < 0 ? Long.MAX_VALUE : maxLength; } @Override @@ -497,6 +497,8 @@ public long remainingLong() @Override public int capacity() { + if (capacityLong() > Integer.MAX_VALUE) + return -1; return Math.toIntExact(capacityLong()); } @@ -568,6 +570,9 @@ public boolean append(ByteBuffer bytes) public boolean append(RetainableByteBuffer bytes) { int remaining = bytes.remaining(); + if (remaining == 0) + return true; + int length = ensureMaxLength(remaining); // If we cannot retain, then try aggregation into the last buffer @@ -582,7 +587,6 @@ public boolean append(RetainableByteBuffer bytes) // if the length was restricted by maxLength, or we can't retain, then copy into new buffer if (length < remaining || !bytes.canRetain()) { - RetainableByteBuffer buffer = _pool.acquire(length, _direct); buffer.append(bytes); bytes.clear(); diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Jetty.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Jetty.java index d65aa3d37529..b01d1c67a76b 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Jetty.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Jetty.java @@ -81,7 +81,8 @@ private static String formatTimestamp(String timestamp) { try { - long epochMillis = Long.parseLong(timestamp); + long epochMillis = (StringUtil.isBlank(timestamp) || timestamp.startsWith("$")) + ? System.currentTimeMillis() : Long.parseLong(timestamp); return Instant.ofEpochMilli(epochMillis).toString(); } catch (NumberFormatException e) From 9a91601b54c1e178b7298b5ebb33e608f36673a8 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 18 Mar 2024 14:17:24 +0100 Subject: [PATCH 33/43] do not copy in append() Signed-off-by: Ludovic Orban --- .../jetty/io/RetainableByteBuffer.java | 83 ++++++++----------- 1 file changed, 33 insertions(+), 50 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index eca933ef7a39..39546c6ea0fa 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -415,7 +415,6 @@ class Accumulator implements RetainableByteBuffer private final boolean _direct; private final long _maxLength; private final List _buffers = new ArrayList<>(); - private boolean _canAggregate; /** * Construct an accumulating {@link RetainableByteBuffer} that may internally accumulate multiple other @@ -442,7 +441,6 @@ public ByteBuffer getByteBuffer() { RetainableByteBuffer combined = copy(true); _buffers.add(combined); - _canAggregate = true; yield combined.getByteBuffer(); } }; @@ -541,7 +539,6 @@ public void clear() { for (RetainableByteBuffer buffer : _buffers) buffer.release(); - _canAggregate = false; _buffers.clear(); } @@ -549,56 +546,55 @@ public void clear() public boolean append(ByteBuffer bytes) { int remaining = bytes.remaining(); - int length = ensureMaxLength(remaining); - // if the length was restricted by maxLength, slice the bytes smaller - ByteBuffer out = bytes; - if (length < remaining) + if (remaining == 0) + return true; + + long currentlyRemaining = _maxLength - remainingLong(); + if (currentlyRemaining >= remaining) + { + RetainableByteBuffer rbb = RetainableByteBuffer.wrap(bytes.slice()); + bytes.position(bytes.limit()); + _buffers.add(rbb); + return true; + } + else { ByteBuffer slice = bytes.slice(); - slice.limit(slice.position() + length); - bytes.position(bytes.position() + length); - out = slice; + slice.limit((int)(slice.position() + currentlyRemaining)); + RetainableByteBuffer rbb = RetainableByteBuffer.wrap(slice); + bytes.position((int)(bytes.position() + currentlyRemaining)); + _buffers.add(rbb); + return false; } - RetainableByteBuffer buffer = _pool.acquire(length, _direct); - buffer.append(out); - _buffers.add(buffer); - _canAggregate = true; - return !bytes.hasRemaining(); } @Override - public boolean append(RetainableByteBuffer bytes) + public boolean append(RetainableByteBuffer retainableBytes) { + ByteBuffer bytes = retainableBytes.getByteBuffer(); int remaining = bytes.remaining(); if (remaining == 0) return true; - int length = ensureMaxLength(remaining); - - // If we cannot retain, then try aggregation into the last buffer - if (!bytes.canRetain() && _canAggregate) + long currentlyRemaining = _maxLength - remainingLong(); + if (currentlyRemaining >= remaining) { - if (_buffers.get(_buffers.size() - 1).append(bytes)) - return true; - length -= remaining - bytes.remaining(); - remaining = bytes.remaining(); + retainableBytes.retain(); + RetainableByteBuffer rbb = RetainableByteBuffer.wrap(bytes.slice(), retainableBytes); + bytes.position(bytes.limit()); + _buffers.add(rbb); + return true; } - - // if the length was restricted by maxLength, or we can't retain, then copy into new buffer - if (length < remaining || !bytes.canRetain()) + else { - RetainableByteBuffer buffer = _pool.acquire(length, _direct); - buffer.append(bytes); - bytes.clear(); - _buffers.add(buffer); - _canAggregate = true; + retainableBytes.retain(); + ByteBuffer slice = bytes.slice(); + slice.limit((int)(slice.position() + currentlyRemaining)); + RetainableByteBuffer rbb = RetainableByteBuffer.wrap(slice, retainableBytes); + bytes.position((int)(bytes.position() + currentlyRemaining)); + _buffers.add(rbb); return false; } - - bytes.retain(); - _buffers.add(bytes); - _canAggregate = false; - return true; } @Override @@ -632,19 +628,6 @@ protected Action process() }.iterate(); } } - - private int ensureMaxLength(int increment) - { - long length = 0; - for (RetainableByteBuffer buffer : _buffers) - length += buffer.remaining(); - - long newLength = Math.addExact(length, increment); - if (newLength <= _maxLength) - return increment; - - return Math.toIntExact(_maxLength - length); - } } /** From 75236c1d4780d7e7b911d85689e84e1c41548c2c Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 18 Mar 2024 15:22:56 +0100 Subject: [PATCH 34/43] add tests for append(RBB) variant Signed-off-by: Ludovic Orban --- .../jetty/io/RetainableByteBufferTest.java | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java index 9ecd6250b6be..6c20e6f918fc 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java @@ -91,6 +91,19 @@ public void testAppendOneByte(RetainableByteBuffer buffer) buffer.release(); } + @ParameterizedTest + @MethodSource("buffers") + public void testAppendOneByteRetainable(RetainableByteBuffer buffer) + { + RetainableByteBuffer toAppend = _pool.acquire(1, true); + BufferUtil.append(toAppend.getByteBuffer(), (byte)'X'); + assertThat(buffer.append(toAppend), is(true)); + assertFalse(toAppend.hasRemaining()); + toAppend.release(); + assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X")); + buffer.release(); + } + @ParameterizedTest @MethodSource("buffers") public void testAppendMoreBytesThanCapacity(RetainableByteBuffer buffer) @@ -115,6 +128,34 @@ public void testAppendMoreBytesThanCapacity(RetainableByteBuffer buffer) buffer.release(); } + @ParameterizedTest + @MethodSource("buffers") + public void testAppendMoreBytesThanCapacityRetainable(RetainableByteBuffer buffer) + { + RetainableByteBuffer toAppend = _pool.acquire(MAX_CAPACITY * 2, true); + int pos = BufferUtil.flipToFill(toAppend.getByteBuffer()); + byte[] bytes = new byte[MAX_CAPACITY * 2]; + Arrays.fill(bytes, (byte)'X'); + toAppend.getByteBuffer().put(bytes); + BufferUtil.flipToFlush(toAppend.getByteBuffer(), pos); + + if (buffer.append(toAppend)) + { + assertTrue(BufferUtil.isEmpty(toAppend.getByteBuffer())); + assertThat(buffer.capacity(), greaterThanOrEqualTo(MAX_CAPACITY * 2)); + } + else + { + assertFalse(BufferUtil.isEmpty(toAppend.getByteBuffer())); + assertThat(toAppend.remaining(), is(MAX_CAPACITY * 2 - buffer.capacity())); + } + toAppend.release(); + + assertThat(BufferUtil.toString(buffer.getByteBuffer()), is("X".repeat(buffer.capacity()))); + assertTrue(buffer.isFull()); + buffer.release(); + } + @ParameterizedTest @MethodSource("buffers") public void testAppendSmallByteBuffer(RetainableByteBuffer buffer) From 00c08ebf7517e9d1098cb89f47a5eeca47bb29cd Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 18 Mar 2024 15:36:45 +0100 Subject: [PATCH 35/43] add tests for copy + fix bugs Signed-off-by: Ludovic Orban --- .../jetty/io/RetainableByteBuffer.java | 2 +- .../jetty/io/RetainableByteBufferTest.java | 31 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 39546c6ea0fa..32a556454d0a 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -460,7 +460,7 @@ private RetainableByteBuffer copy(boolean take) BufferUtil.flipToFill(byteBuffer); for (RetainableByteBuffer buffer : _buffers) { - buffer.putTo(byteBuffer); + byteBuffer.put(buffer.getByteBuffer().slice()); if (take) buffer.release(); } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java index 6c20e6f918fc..61625c8e36e9 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java @@ -32,6 +32,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -221,4 +222,34 @@ public void testRetainableWriteTo(RetainableByteBuffer buffer) throws Exception buffer.release(); assertTrue(released.await(5, TimeUnit.SECONDS)); } + + @ParameterizedTest + @MethodSource("buffers") + public void testCopy(RetainableByteBuffer original) + { + original.append(ByteBuffer.wrap("hello".getBytes(StandardCharsets.UTF_8))); + RetainableByteBuffer copy = original.copy(); + + assertEquals(0, copy.space()); + assertEquals("hello", StandardCharsets.UTF_8.decode(original.getByteBuffer()).toString()); + assertEquals("hello", StandardCharsets.UTF_8.decode(copy.getByteBuffer()).toString()); + + copy.release(); + original.release(); + } + + @ParameterizedTest + @MethodSource("buffers") + public void testCopyThenModifyOriginal(RetainableByteBuffer original) + { + original.append(ByteBuffer.wrap("hello".getBytes(StandardCharsets.UTF_8))); + RetainableByteBuffer copy = original.copy(); + original.append(ByteBuffer.wrap(" world".getBytes(StandardCharsets.UTF_8))); + + assertEquals("hello world", StandardCharsets.UTF_8.decode(original.getByteBuffer()).toString()); + assertEquals("hello", StandardCharsets.UTF_8.decode(copy.getByteBuffer()).toString()); + + copy.release(); + original.release(); + } } From a56693f4d4fbb6a0b968110bde9fe290fcf954bb Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 18 Mar 2024 15:55:10 +0100 Subject: [PATCH 36/43] improve javadoc Signed-off-by: Ludovic Orban --- .../jetty/io/RetainableByteBuffer.java | 38 +++++++++++++++---- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 32a556454d0a..cb4509914fbf 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -145,7 +145,10 @@ public boolean release() ByteBuffer getByteBuffer(); /** - * @return A copy of this RetainableByteBuffer that is entirely independent + * Creates a copy of this RetainableByteBuffer that is entirely independent, but + * backed by the same memory space, i.e.: modifying the ByteBuffer of the original + * also modifies the ByteBuffer of the copy and vice-versa. + * @return A copy of this RetainableByteBuffer */ default RetainableByteBuffer copy() { @@ -189,17 +192,24 @@ default int capacity() return getByteBuffer().capacity(); } + /** + * @return the number of bytes left for appending in the {@code ByteBuffer} + */ default int space() { return capacity() - remaining(); } + /** + * @return whether the {@code ByteBuffer} has remaining bytes left for appending + */ default boolean isFull() { - return remaining() == capacity(); + return space() == 0; } /** + * Clears the contained byte buffer to be empty in flush mode. * @see BufferUtil#clear(ByteBuffer) */ default void clear() @@ -230,11 +240,23 @@ default boolean append(RetainableByteBuffer bytes) return bytes.remaining() == 0 || append(bytes.getByteBuffer()); } + /** + * Copies the contents of this retainable byte buffer at the end of the given byte buffer. + * @param toInfillMode the destination buffer. + * @see ByteBuffer#put(ByteBuffer) + */ default void putTo(ByteBuffer toInfillMode) { toInfillMode.put(getByteBuffer()); } + /** + * Asynchronously copies the contents of this retainable byte buffer into given sink. + * @param sink the destination sink. + * @param last true if this is the last write. + * @param callback the callback to call upon the write completion. + * @see org.eclipse.jetty.io.Content.Sink#write(boolean, ByteBuffer, Callback) + */ default void writeTo(Content.Sink sink, boolean last, Callback callback) { sink.write(last, getByteBuffer(), callback); @@ -472,12 +494,13 @@ private RetainableByteBuffer copy(boolean take) /** * {@inheritDoc} - * @throws ArithmeticException if the length of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE} + * @throws {@link Integer#MAX_VALUE} if the length of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE} */ @Override public int remaining() { - return Math.toIntExact(remainingLong()); + long remainingLong = remainingLong(); + return remainingLong > Integer.MAX_VALUE ? Integer.MAX_VALUE : Math.toIntExact(remainingLong); } public long remainingLong() @@ -490,14 +513,13 @@ public long remainingLong() /** * {@inheritDoc} - * @throws ArithmeticException if the maxLength of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE}. + * @throws {@link Integer#MAX_VALUE} if the maxLength of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE}. */ @Override public int capacity() { - if (capacityLong() > Integer.MAX_VALUE) - return -1; - return Math.toIntExact(capacityLong()); + long capacityLong = capacityLong(); + return capacityLong > Integer.MAX_VALUE ? Integer.MAX_VALUE : Math.toIntExact(capacityLong); } public long capacityLong() From 9a41edc52d9aefe4127a3cec4271a6ed8f97cf93 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 18 Mar 2024 16:10:27 +0100 Subject: [PATCH 37/43] fix javadoc Signed-off-by: Ludovic Orban --- .../main/java/org/eclipse/jetty/io/RetainableByteBuffer.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index cb4509914fbf..f7896285bf34 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -494,7 +494,7 @@ private RetainableByteBuffer copy(boolean take) /** * {@inheritDoc} - * @throws {@link Integer#MAX_VALUE} if the length of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE} + * @return {@link Integer#MAX_VALUE} if the length of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE} */ @Override public int remaining() @@ -513,7 +513,7 @@ public long remainingLong() /** * {@inheritDoc} - * @throws {@link Integer#MAX_VALUE} if the maxLength of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE}. + * @return {@link Integer#MAX_VALUE} if the maxLength of this {@code Accumulator} is greater than {@link Integer#MAX_VALUE}. */ @Override public int capacity() From e7eec0e581916c898be8e2ace35434f8c680bcf5 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 18 Mar 2024 16:26:13 +0100 Subject: [PATCH 38/43] improve tests Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/io/RetainableByteBufferTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java index 61625c8e36e9..cdbef592e615 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/RetainableByteBufferTest.java @@ -231,6 +231,8 @@ public void testCopy(RetainableByteBuffer original) RetainableByteBuffer copy = original.copy(); assertEquals(0, copy.space()); + assertEquals(5, copy.remaining()); + assertEquals(5, original.remaining()); assertEquals("hello", StandardCharsets.UTF_8.decode(original.getByteBuffer()).toString()); assertEquals("hello", StandardCharsets.UTF_8.decode(copy.getByteBuffer()).toString()); @@ -246,6 +248,9 @@ public void testCopyThenModifyOriginal(RetainableByteBuffer original) RetainableByteBuffer copy = original.copy(); original.append(ByteBuffer.wrap(" world".getBytes(StandardCharsets.UTF_8))); + assertEquals(0, copy.space()); + assertEquals(5, copy.remaining()); + assertEquals(11, original.remaining()); assertEquals("hello world", StandardCharsets.UTF_8.decode(original.getByteBuffer()).toString()); assertEquals("hello", StandardCharsets.UTF_8.decode(copy.getByteBuffer()).toString()); From 0adadf86fba9da3f01c5fd82501fad3177e7188d Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 19 Mar 2024 09:48:36 +0100 Subject: [PATCH 39/43] fix releasing of appended RBB Signed-off-by: Ludovic Orban --- .../core/messages/ByteBufferMessageSink.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java index 126d939b78dd..30fd1bc26eb0 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteBufferMessageSink.java @@ -31,7 +31,7 @@ */ public class ByteBufferMessageSink extends AbstractMessageSink { - private RetainableByteBuffer accumulator; + private RetainableByteBuffer.Accumulator accumulator; /** * Creates a new {@link ByteBufferMessageSink}. @@ -88,13 +88,14 @@ public void accept(Frame frame, Callback callback) if (accumulator == null) accumulator = new RetainableByteBuffer.Accumulator(getCoreSession().getByteBufferPool(), false, maxSize); - accumulator.append(RetainableByteBuffer.wrap(frame.getPayload(), callback::succeeded)); + RetainableByteBuffer wrappedPayload = RetainableByteBuffer.wrap(frame.getPayload(), callback::succeeded); + accumulator.append(wrappedPayload); + wrappedPayload.release(); if (frame.isFin()) { - RetainableByteBuffer buffer = accumulator; - callback = Callback.from(buffer::release); - invoke(getMethodHandle(), buffer.getByteBuffer(), callback); + callback = Callback.from(accumulator::release); + invoke(getMethodHandle(), accumulator.getByteBuffer(), callback); autoDemand(); } else From ccca9aaca39b7541bcffff6ddd8da3e0c026401b Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 19 Mar 2024 12:15:54 +0100 Subject: [PATCH 40/43] fix leaks and leak detection in tests Signed-off-by: Ludovic Orban --- .../core/messages/ByteArrayMessageSink.java | 4 +++- .../PermessageDeflateDemandTest.java | 22 ++++++++++++++---- .../websocket/tests/ClientDisconnectTest.java | 23 +++++++++++-------- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java index 5ee9d30e8086..2b57b9b36911 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/messages/ByteArrayMessageSink.java @@ -87,7 +87,9 @@ public void accept(Frame frame, Callback callback) if (accumulator == null) accumulator = new RetainableByteBuffer.Accumulator(getCoreSession().getByteBufferPool(), false, maxSize); - accumulator.append(RetainableByteBuffer.wrap(payload, callback::succeeded)); + RetainableByteBuffer wrappedPayload = RetainableByteBuffer.wrap(payload, callback::succeeded); + accumulator.append(wrappedPayload); + wrappedPayload.release(); if (frame.isFin()) { diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java b/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java index 4f5a9edcc82a..602666250d1f 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/extensions/PermessageDeflateDemandTest.java @@ -19,12 +19,14 @@ import java.util.concurrent.BlockingQueue; import java.util.concurrent.TimeUnit; +import org.eclipse.jetty.io.ArrayByteBufferPool; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.util.BlockingArrayQueue; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.websocket.core.CloseStatus; import org.eclipse.jetty.websocket.core.CoreSession; import org.eclipse.jetty.websocket.core.Frame; @@ -40,12 +42,14 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; public class PermessageDeflateDemandTest { private Server _server; + private ArrayByteBufferPool.Tracking _bufferPool; private WebSocketCoreClient _client; private ServerConnector _connector; private WebSocketUpgradeHandler _upgradeHandler; @@ -53,7 +57,8 @@ public class PermessageDeflateDemandTest @BeforeEach public void before() throws Exception { - _server = new Server(); + _bufferPool = new ArrayByteBufferPool.Tracking(); + _server = new Server(null, null, _bufferPool); _connector = new ServerConnector(_server); _server.addConnector(_connector); @@ -68,8 +73,15 @@ public void before() throws Exception @AfterEach public void after() throws Exception { - _client.stop(); - _server.stop(); + try + { + assertThat("Detected leaks: " + _bufferPool.dumpLeaks(), _bufferPool.getLeaks().size(), is(0)); + } + finally + { + LifeCycle.stop(_client); + LifeCycle.stop(_server); + } } @Test @@ -155,7 +167,9 @@ public void onFrame(Frame frame, Callback callback) } break; case OpCode.BINARY: - _byteBuilder.append(RetainableByteBuffer.wrap(frame.getPayload(), callback::succeeded)); + RetainableByteBuffer wrappedPayload = RetainableByteBuffer.wrap(frame.getPayload(), callback::succeeded); + _byteBuilder.append(wrappedPayload); + wrappedPayload.release(); if (frame.isFin()) { // TODO this looks wrong diff --git a/jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jetty-tests/src/test/java/org/eclipse/jetty/ee9/websocket/tests/ClientDisconnectTest.java b/jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jetty-tests/src/test/java/org/eclipse/jetty/ee9/websocket/tests/ClientDisconnectTest.java index 97909fb5c8a4..9ad1b890e400 100644 --- a/jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jetty-tests/src/test/java/org/eclipse/jetty/ee9/websocket/tests/ClientDisconnectTest.java +++ b/jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jetty-tests/src/test/java/org/eclipse/jetty/ee9/websocket/tests/ClientDisconnectTest.java @@ -29,12 +29,13 @@ import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.component.LifeCycle; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -44,6 +45,7 @@ public class ClientDisconnectTest private final Duration _serverIdleTimeout = Duration.ofSeconds(5); private final int _messageSize = 5 * 1024 * 1024; private Server _server; + private ArrayByteBufferPool.Tracking _bufferPool; private ServerConnector _connector; private WebSocketClient _client; @@ -62,7 +64,8 @@ public void onOpen(Session session) public void before() throws Exception { _client = new WebSocketClient(); - _server = new Server(); + _bufferPool = new ArrayByteBufferPool.Tracking(); + _server = new Server(null, null, _bufferPool); _connector = new ServerConnector(_server); _server.addConnector(_connector); @@ -82,8 +85,15 @@ public void before() throws Exception @AfterEach public void after() throws Exception { - _client.stop(); - _server.stop(); + try + { + assertThat("Detected leaks: " + _bufferPool.dumpLeaks(), _bufferPool.getLeaks().size(), is(0)); + } + finally + { + LifeCycle.stop(_client); + LifeCycle.stop(_server); + } } @Test @@ -105,10 +115,5 @@ public void testBuffersAfterIncompleteMessage() throws Exception // Wait for the server to close its session. assertTrue(serverSocket.closeLatch.await(_serverIdleTimeout.toSeconds() + 1, TimeUnit.SECONDS)); - - // We should have no buffers still used in the pool. - ArrayByteBufferPool bufferPool = _server.getBean(ArrayByteBufferPool.class); - assertThat(bufferPool.getDirectByteBufferCount() - bufferPool.getAvailableDirectByteBufferCount(), equalTo(0L)); - assertThat(bufferPool.getHeapByteBufferCount() - bufferPool.getAvailableHeapByteBufferCount(), equalTo(0L)); } } From 48a6c654c5d1fa675ddc6a0c8bc32b73d981e444 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 20 Mar 2024 13:16:12 +0100 Subject: [PATCH 41/43] Update from review Use RBB.append rather than BU.append --- .../main/java/org/eclipse/jetty/http/GZIPContentDecoder.java | 2 +- .../src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java | 2 +- .../src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java | 2 +- .../org/eclipse/jetty/server/DetectorConnectionFactory.java | 2 +- .../java/org/eclipse/jetty/server/ProxyConnectionFactory.java | 2 +- .../java/org/eclipse/jetty/websocket/core/internal/Parser.java | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/GZIPContentDecoder.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/GZIPContentDecoder.java index 45c41eda4977..001cb9abcc10 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/GZIPContentDecoder.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/GZIPContentDecoder.java @@ -109,7 +109,7 @@ public RetainableByteBuffer decode(ByteBuffer compressed) RetainableByteBuffer result = acquire(length); for (RetainableByteBuffer buffer : _inflateds) { - BufferUtil.append(result.getByteBuffer(), buffer.getByteBuffer()); + result.append(buffer); buffer.release(); } _inflateds.clear(); diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java index 5152b3f0f638..ac383bec0bb8 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java @@ -547,7 +547,7 @@ public boolean release() private void put(ByteBuffer source) { - BufferUtil.append(delegate.getByteBuffer(), source); + delegate.append(source); } } } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java index c693bd68e20a..7adad1f6bc1e 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java @@ -111,7 +111,7 @@ public RetainableByteBuffer take(ByteBufferPool pool, boolean direct) for (Chunk chunk : _chunks) { offset += chunk.remaining(); - BufferUtil.append(buffer.getByteBuffer(), chunk.getByteBuffer()); + buffer.append(chunk); chunk.release(); } assert offset == _length; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/DetectorConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/DetectorConnectionFactory.java index c68a69e6324c..d9a5e816f160 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/DetectorConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/DetectorConnectionFactory.java @@ -155,7 +155,7 @@ public void onUpgradeTo(ByteBuffer byteBuffer) if (LOG.isDebugEnabled()) LOG.debug("Detector {} adopting {}", getProtocol(), BufferUtil.toDetailString(byteBuffer)); // Throws if the ByteBuffer to adopt is too big, but it is handled. - BufferUtil.append(_buffer.getByteBuffer(), byteBuffer); + _buffer.append(byteBuffer); } @Override diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java index f8b747090417..c9b78eff648d 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java @@ -476,7 +476,7 @@ public void onUpgradeTo(ByteBuffer buffer) { if (LOG.isDebugEnabled()) LOG.debug("Proxy v2 copying unconsumed buffer {}", BufferUtil.toDetailString(buffer)); - BufferUtil.append(_buffer.getByteBuffer(), buffer); + _buffer.append(buffer); } @Override diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/Parser.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/Parser.java index 7626f40a5960..49e8ad219215 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/Parser.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/Parser.java @@ -344,7 +344,7 @@ private Frame.Parsed parsePayload(ByteBuffer buffer) // No space in the buffer, so we have to copy the partial payload. // The size of this allocation is limited by the maxFrameSize. aggregate = bufferPool.acquire(payloadLength, false); - BufferUtil.append(aggregate.getByteBuffer(), buffer); + aggregate.append(buffer); return null; } From fb5c5a3ed5f5b594e10e6c5984b72341fad2d317 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 20 Mar 2024 13:50:11 +0100 Subject: [PATCH 42/43] Update from review Use RBB.append rather than BU.append --- .../java/org/eclipse/jetty/io/ssl/SslConnection.java | 2 +- .../eclipse/jetty/server/ProxyConnectionFactory.java | 2 +- .../eclipse/jetty/server/internal/HttpConnection.java | 11 ++++++++--- .../eclipse/jetty/websocket/core/internal/Parser.java | 2 +- .../jetty/ee9/nested/BufferedResponseHandler.java | 2 +- 5 files changed, 12 insertions(+), 7 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java index 39f74d7dde11..7c9c8acb59a4 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java @@ -342,7 +342,7 @@ private void acquireEncryptedOutput() public void onUpgradeTo(ByteBuffer buffer) { acquireEncryptedInput(); - BufferUtil.append(_encryptedInput.getByteBuffer(), buffer); + _encryptedInput.append(buffer); } @Override diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java index c9b78eff648d..d639139de71f 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java @@ -253,7 +253,7 @@ public void onUpgradeTo(ByteBuffer buffer) { if (LOG.isDebugEnabled()) LOG.debug("Proxy v1 copying unconsumed buffer {}", BufferUtil.toDetailString(buffer)); - BufferUtil.append(_buffer.getByteBuffer(), buffer); + _buffer.append(buffer); } /** diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 0a3dafedf50a..043696c20d51 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -333,7 +333,7 @@ public ByteBuffer onUpgradeFrom() @Override public void onUpgradeTo(ByteBuffer buffer) { - BufferUtil.append(getRequestBuffer(), buffer); + getRetainableRequestBuffer().append(buffer); } @Override @@ -356,11 +356,16 @@ void releaseRequestBuffer() } } - private ByteBuffer getRequestBuffer() + private RetainableByteBuffer getRetainableRequestBuffer() { if (_retainableByteBuffer == null) _retainableByteBuffer = _bufferPool.acquire(getInputBufferSize(), isUseInputDirectByteBuffers()); - return _retainableByteBuffer.getByteBuffer(); + return _retainableByteBuffer; + } + + private ByteBuffer getRequestBuffer() + { + return getRetainableRequestBuffer().getByteBuffer(); } public boolean isRequestBufferEmpty() diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/Parser.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/Parser.java index 49e8ad219215..dd0e4dc709f9 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/Parser.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/Parser.java @@ -376,7 +376,7 @@ private Frame.Parsed parsePayload(ByteBuffer buffer) if (available < expecting) { // not enough data to complete this frame, just copy it - BufferUtil.append(aggregate.getByteBuffer(), buffer); + aggregate.append(buffer); return null; } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/BufferedResponseHandler.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/BufferedResponseHandler.java index 61fa4cd6738f..9f4b649045bb 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/BufferedResponseHandler.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/BufferedResponseHandler.java @@ -205,7 +205,7 @@ class ArrayBufferedInterceptor implements BufferedInterceptor private final HttpChannel _channel; private final Queue _buffers = new ArrayDeque<>(); private Boolean _aggregating; - private ByteBuffer _aggregate; + private ByteBuffer _aggregate; // TODO use RetainableByteBuffer.Aggregator public ArrayBufferedInterceptor(HttpChannel httpChannel, Interceptor interceptor) { From 5936d4dc7b4649b89b81bc4b8b1dae99ff039f3a Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 27 Mar 2024 10:38:33 +0100 Subject: [PATCH 43/43] improved javadoc and removed asReadOnly --- .../java/org/eclipse/jetty/io/Content.java | 46 ------------------- .../jetty/io/RetainableByteBuffer.java | 41 ++++++++++++++++- .../org/eclipse/jetty/io/ContentTest.java | 25 ---------- .../jetty/server/handler/EventsHandler.java | 2 +- .../org/eclipse/jetty/util/BufferUtil.java | 19 +++++--- 5 files changed, 53 insertions(+), 80 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index c68f465a8527..7e4dc5507d1f 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -889,52 +889,6 @@ default Throwable getFailure() */ boolean isLast(); - /** - *

Copies the bytes from this Chunk to the given byte array.

- * - * @param bytes the byte array to copy the bytes into - * @param offset the offset within the byte array - * @param length the maximum number of bytes to copy - * @return the number of bytes actually copied - */ - default int get(byte[] bytes, int offset, int length) - { - ByteBuffer b = getByteBuffer(); - if (b == null || !b.hasRemaining()) - return 0; - length = Math.min(length, b.remaining()); - b.get(bytes, offset, length); - return length; - } - - /** - *

Skips, advancing the ByteBuffer position, the given number of bytes.

- * - * @param length the maximum number of bytes to skip - * @return the number of bytes actually skipped - */ - default int skip(int length) - { - if (length == 0) - return 0; - ByteBuffer byteBuffer = getByteBuffer(); - length = Math.min(byteBuffer.remaining(), length); - byteBuffer.position(byteBuffer.position() + length); - return length; - } - - /** - * @return an immutable version of this Chunk - */ - default Chunk asReadOnly() - { - if (getByteBuffer().isReadOnly()) - return this; - if (canRetain()) - return asChunk(getByteBuffer().asReadOnlyBuffer(), isLast(), this); - return from(getByteBuffer().asReadOnlyBuffer(), isLast()); - } - /** *

Implementations of this interface may process {@link Chunk}s being copied by the * {@link Content#copy(Source, Sink, Processor, Callback)} method, so that diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index f7896285bf34..6046e529163c 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -14,6 +14,7 @@ package org.eclipse.jetty.io; import java.nio.ByteBuffer; +import java.nio.ReadOnlyBufferException; import java.util.ArrayList; import java.util.List; @@ -221,9 +222,10 @@ default void clear() * Copies the contents of the given byte buffer at the end of this buffer. * @param bytes the byte buffer to copy from. * @return true if all bytes of the given buffer were copied, false otherwise. + * @throws ReadOnlyBufferException if the buffer is read only * @see BufferUtil#append(ByteBuffer, ByteBuffer) */ - default boolean append(ByteBuffer bytes) + default boolean append(ByteBuffer bytes) throws ReadOnlyBufferException { BufferUtil.append(getByteBuffer(), bytes); return !bytes.hasRemaining(); @@ -233,13 +235,48 @@ default boolean append(ByteBuffer bytes) * Copies the contents of the given retainable byte buffer at the end of this buffer. * @param bytes the retainable byte buffer to copy from. * @return true if all bytes of the given buffer were copied, false otherwise. + * @throws ReadOnlyBufferException if the buffer is read only * @see BufferUtil#append(ByteBuffer, ByteBuffer) */ - default boolean append(RetainableByteBuffer bytes) + default boolean append(RetainableByteBuffer bytes) throws ReadOnlyBufferException { return bytes.remaining() == 0 || append(bytes.getByteBuffer()); } + /** + *

Copies the bytes from this Chunk to the given byte array.

+ * + * @param bytes the byte array to copy the bytes into + * @param offset the offset within the byte array + * @param length the maximum number of bytes to copy + * @return the number of bytes actually copied + */ + default int get(byte[] bytes, int offset, int length) + { + ByteBuffer b = getByteBuffer(); + if (b == null || !b.hasRemaining()) + return 0; + length = Math.min(length, b.remaining()); + b.get(bytes, offset, length); + return length; + } + + /** + *

Skips, advancing the ByteBuffer position, the given number of bytes.

+ * + * @param length the maximum number of bytes to skip + * @return the number of bytes actually skipped + */ + default int skip(int length) + { + if (length == 0) + return 0; + ByteBuffer byteBuffer = getByteBuffer(); + length = Math.min(byteBuffer.remaining(), length); + byteBuffer.position(byteBuffer.position() + length); + return length; + } + /** * Copies the contents of this retainable byte buffer at the end of the given byte buffer. * @param toInfillMode the destination buffer. diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentTest.java index 211914a4b802..add174fb4dd5 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentTest.java @@ -14,43 +14,18 @@ package org.eclipse.jetty.io; import java.nio.ByteBuffer; -import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; -import org.eclipse.jetty.util.BufferUtil; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.sameInstance; public class ContentTest { - @Test - public void testAsReadOnly() - { - assertThat(Content.Chunk.EOF.asReadOnly(), sameInstance(Content.Chunk.EOF)); - assertThat(Content.Chunk.EMPTY.asReadOnly(), sameInstance(Content.Chunk.EMPTY)); - - assertThat(Content.Chunk.from(BufferUtil.EMPTY_BUFFER, true).asReadOnly(), sameInstance(Content.Chunk.EOF)); - assertThat(Content.Chunk.from(BufferUtil.EMPTY_BUFFER, false).asReadOnly(), sameInstance(Content.Chunk.EMPTY)); - - Content.Chunk failureChunk = Content.Chunk.from(new NumberFormatException()); - assertThat(failureChunk.asReadOnly(), sameInstance(failureChunk)); - - Content.Chunk chunk = Content.Chunk.from(ByteBuffer.wrap(new byte[1]).asReadOnlyBuffer(), false); - assertThat(chunk.asReadOnly(), sameInstance(chunk)); - - Content.Chunk rwChunk = Content.Chunk.from(ByteBuffer.wrap("abc".getBytes(StandardCharsets.US_ASCII)), false); - Content.Chunk roChunk = rwChunk.asReadOnly(); - assertThat(rwChunk, not(sameInstance(roChunk))); - assertThat(BufferUtil.toString(rwChunk.getByteBuffer(), StandardCharsets.US_ASCII), equalTo(BufferUtil.toString(roChunk.getByteBuffer(), StandardCharsets.US_ASCII))); - } - @Test public void testFromEmptyByteBufferWithoutReleaser() { diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/EventsHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/EventsHandler.java index b338f5cb3bb6..4f31b702e3bc 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/EventsHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/EventsHandler.java @@ -106,7 +106,7 @@ private void notifyOnRequestRead(Request wrapped, Content.Chunk chunk) { try { - onRequestRead(wrapped, chunk == null ? null : chunk.asReadOnly()); + onRequestRead(wrapped, chunk); } catch (Throwable x) { diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java index 13b95d5bcbc4..71561cfc084f 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java @@ -21,6 +21,7 @@ import java.nio.BufferOverflowException; import java.nio.ByteBuffer; import java.nio.ByteOrder; +import java.nio.ReadOnlyBufferException; import java.nio.channels.FileChannel; import java.nio.channels.FileChannel.MapMode; import java.nio.channels.ReadableByteChannel; @@ -431,8 +432,9 @@ public static boolean compact(ByteBuffer buffer) * @param from Buffer to take bytes from in flush mode, whose position is modified with the bytes taken. * @param to Buffer to put bytes to in fill mode. * @return number of bytes moved + * @throws ReadOnlyBufferException if the buffer is read only */ - public static int put(ByteBuffer from, ByteBuffer to) + public static int put(ByteBuffer from, ByteBuffer to) throws ReadOnlyBufferException { int length = from.remaining(); if (length == 0) @@ -469,8 +471,9 @@ public static int put(ByteBuffer from, ByteBuffer to) * @param off offset into byte * @param len length to append * @throws BufferOverflowException if unable to append buffer due to space limits + * @throws ReadOnlyBufferException if the buffer is read only */ - public static void append(ByteBuffer to, byte[] b, int off, int len) throws BufferOverflowException + public static void append(ByteBuffer to, byte[] b, int off, int len) throws BufferOverflowException, ReadOnlyBufferException { int pos = flipToFill(to); try @@ -489,8 +492,9 @@ public static void append(ByteBuffer to, byte[] b, int off, int len) throws Buff * @param to Buffer is flush mode * @param b bytes to append * @throws BufferOverflowException if unable to append buffer due to space limits + * @throws ReadOnlyBufferException if the buffer is read only */ - public static void append(ByteBuffer to, byte[] b) throws BufferOverflowException + public static void append(ByteBuffer to, byte[] b) throws BufferOverflowException, ReadOnlyBufferException { append(to, b, 0, b.length); } @@ -501,8 +505,9 @@ public static void append(ByteBuffer to, byte[] b) throws BufferOverflowExceptio * @param to Buffer is flush mode * @param s String to append as UTF8 * @throws BufferOverflowException if unable to append buffer due to space limits + * @throws ReadOnlyBufferException if the buffer is read only */ - public static void append(ByteBuffer to, String s) throws BufferOverflowException + public static void append(ByteBuffer to, String s) throws BufferOverflowException, ReadOnlyBufferException { byte[] b = s.getBytes(StandardCharsets.UTF_8); append(to, b, 0, b.length); @@ -514,8 +519,9 @@ public static void append(ByteBuffer to, String s) throws BufferOverflowExceptio * @param to Buffer is flush mode * @param b byte to append * @throws BufferOverflowException if unable to append buffer due to space limits + * @throws ReadOnlyBufferException if the buffer is read only */ - public static void append(ByteBuffer to, byte b) + public static void append(ByteBuffer to, byte b) throws BufferOverflowException, ReadOnlyBufferException { int pos = flipToFill(to); try @@ -534,8 +540,9 @@ public static void append(ByteBuffer to, byte b) * @param to Buffer in flush mode, whose position will be incremented by the number of bytes appended * @param b buffer to append to in flush mode, whose limit will be incremented by the number of bytes appended. * @return The number of bytes appended. + * @throws ReadOnlyBufferException if the buffer is read only */ - public static int append(ByteBuffer to, ByteBuffer b) + public static int append(ByteBuffer to, ByteBuffer b) throws ReadOnlyBufferException { int pos = flipToFill(to); try