From c4a928cf76015acae23c2e529c271e5819156816 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 6 Dec 2022 10:30:36 +0100 Subject: [PATCH] #8993: make the EOF / EMPTY decision in Content.Chunk.from() Signed-off-by: Ludovic Orban --- .../internal/HttpReceiverOverHTTP2.java | 20 ++++--------------- .../server/internal/HttpStreamOverHTTP2.java | 14 +++---------- .../internal/HttpReceiverOverHTTP3.java | 18 ++++------------- .../server/internal/HttpStreamOverHTTP3.java | 15 ++++---------- .../java/org/eclipse/jetty/io/Content.java | 17 ++++++++++++++++ .../jetty/io/internal/ByteBufferChunk.java | 11 ++-------- 6 files changed, 34 insertions(+), 61 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java index 6698e134587..0ca4f18051e 100644 --- a/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java @@ -14,7 +14,6 @@ package org.eclipse.jetty.http2.client.transport.internal; import java.io.IOException; -import java.nio.ByteBuffer; import java.util.List; import java.util.function.BiFunction; @@ -81,21 +80,10 @@ public Content.Chunk read(boolean fillInterestIfNeeded) return null; } DataFrame frame = data.frame(); - ByteBuffer buffer = frame.getData(); - if (!buffer.hasRemaining()) - { - data.release(); - if (frame.isEndStream()) - { - responseSuccess(getHttpExchange(), null); - return Content.Chunk.EOF; - } - else - { - return Content.Chunk.EMPTY; - } - } - return Content.Chunk.from(buffer, false, data); + boolean last = frame.remaining() == 0 && frame.isEndStream(); + if (last) + responseSuccess(getHttpExchange(), null); + return Content.Chunk.from(frame.getData(), last, data); } @Override diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java index 84cd2194797..4f657c6ef17 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java @@ -241,19 +241,11 @@ public Runnable onTrailer(HeadersFrame frame) private Content.Chunk createChunk(Stream.Data data) { DataFrame frame = data.frame(); - ByteBuffer byteBuffer = frame.getData(); - if (!byteBuffer.hasRemaining()) - { - // We must NOT retain because we are not passing the - // ByteBuffer to the Chunk. - if (frame.isEndStream()) - return Content.Chunk.EOF; - else - return Content.Chunk.EMPTY; - } + if (frame.isEndStream() && frame.remaining() == 0) + return Content.Chunk.EOF; // We need to retain because we are passing the ByteBuffer to the Chunk. data.retain(); - return Content.Chunk.from(byteBuffer, frame.isEndStream(), data); + return Content.Chunk.from(frame.getData(), frame.isEndStream(), data); } @Override diff --git a/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/internal/HttpReceiverOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/internal/HttpReceiverOverHTTP3.java index ec2deae87fd..3fecd5f98a6 100644 --- a/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/internal/HttpReceiverOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/internal/HttpReceiverOverHTTP3.java @@ -64,20 +64,10 @@ public Content.Chunk read(boolean fillInterestIfNeeded) return null; } ByteBuffer byteBuffer = data.getByteBuffer(); - if (!byteBuffer.hasRemaining()) - { - data.release(); - if (data.isLast()) - { - responseSuccess(getHttpExchange(), null); - return Content.Chunk.EOF; - } - else - { - return Content.Chunk.EMPTY; - } - } - return Content.Chunk.from(byteBuffer, false, data); + boolean last = !byteBuffer.hasRemaining() && data.isLast(); + if (last) + responseSuccess(getHttpExchange(), null); + return Content.Chunk.from(byteBuffer, last, data); } @Override diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index 5aeed2d8e5b..cb8474df029 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -233,19 +233,12 @@ public Runnable onTrailer(HeadersFrame frame) private Content.Chunk createChunk(Stream.Data data) { - ByteBuffer byteBuffer = data.getByteBuffer(); - if (!byteBuffer.hasRemaining()) - { - // As we are NOT passing the ByteBuffer to the Chunk, - // we must not retain. - if (data.isLast()) - return Content.Chunk.EOF; - else - return Content.Chunk.EMPTY; - } + if (data == Stream.Data.EOF) + return Content.Chunk.EOF; + // As we are passing the ByteBuffer to the Chunk we need to retain. data.retain(); - return Content.Chunk.from(byteBuffer, data.isLast(), data); + return Content.Chunk.from(data.getByteBuffer(), data.isLast(), data); } @Override 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 7371bbad23c..36c43c494f2 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 @@ -446,6 +446,8 @@ public interface Chunk extends Retainable */ static Chunk from(ByteBuffer byteBuffer, boolean last) { + if (!byteBuffer.hasRemaining()) + throw new IllegalArgumentException("Empty ByteBuffer instances are forbidden, use the EOF or EMPTY constant instead."); return new ByteBufferChunk.WithReferenceCount(byteBuffer, last); } @@ -459,6 +461,11 @@ static Chunk from(ByteBuffer byteBuffer, boolean last) */ static Chunk from(ByteBuffer byteBuffer, boolean last, Runnable releaser) { + if (!byteBuffer.hasRemaining()) + { + releaser.run(); + return last ? EOF : EMPTY; + } return new ByteBufferChunk.ReleasedByRunnable(byteBuffer, last, Objects.requireNonNull(releaser)); } @@ -475,6 +482,11 @@ static Chunk from(ByteBuffer byteBuffer, boolean last, Runnable releaser) */ static Chunk from(ByteBuffer byteBuffer, boolean last, Consumer releaser) { + if (!byteBuffer.hasRemaining()) + { + releaser.accept(byteBuffer); + return last ? EOF : EMPTY; + } return new ByteBufferChunk.ReleasedByConsumer(byteBuffer, last, Objects.requireNonNull(releaser)); } @@ -492,6 +504,11 @@ static Chunk from(ByteBuffer byteBuffer, boolean last, Consumer rele */ static Chunk from(ByteBuffer byteBuffer, boolean last, Retainable retainable) { + if (!byteBuffer.hasRemaining()) + { + retainable.release(); + return last ? EOF : EMPTY; + } return new ByteBufferChunk.WithRetainable(byteBuffer, last, Objects.requireNonNull(retainable)); } 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 d177723d7ad..03e216c5239 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 @@ -24,7 +24,7 @@ public abstract class ByteBufferChunk implements Content.Chunk { - public static final ByteBufferChunk EMPTY = new ByteBufferChunk(true, BufferUtil.EMPTY_BUFFER, false) + public static final ByteBufferChunk EMPTY = new ByteBufferChunk(BufferUtil.EMPTY_BUFFER, false) { @Override public String toString() @@ -32,7 +32,7 @@ public String toString() return "%s[EMPTY]".formatted(ByteBufferChunk.class.getSimpleName()); } }; - public static final ByteBufferChunk EOF = new ByteBufferChunk(true, BufferUtil.EMPTY_BUFFER, true) + public static final ByteBufferChunk EOF = new ByteBufferChunk(BufferUtil.EMPTY_BUFFER, true) { @Override public String toString() @@ -46,13 +46,6 @@ public String toString() public ByteBufferChunk(ByteBuffer byteBuffer, boolean last) { - this(false, byteBuffer, last); - } - - private ByteBufferChunk(boolean isConstant, ByteBuffer byteBuffer, boolean last) - { - if (!isConstant && !byteBuffer.hasRemaining() && last) - throw new IllegalArgumentException("Empty ByteBuffer instances with last=true are forbidden, use the EOF constant instead."); this.byteBuffer = Objects.requireNonNull(byteBuffer); this.last = last; }