#8993: make the EOF / EMPTY decision in Content.Chunk.from()

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
jetty · Dec 6, 2022 · c4a928c · c4a928c
1 parent 41345d3
commit c4a928c
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 61 deletions.
diff --git a/...rc/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java b/...rc/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/...tp2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java b/...tp2-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/...rc/main/java/org/eclipse/jetty/http3/client/transport/internal/HttpReceiverOverHTTP3.java b/...rc/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/...tp3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/...tp3-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
@@ -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<ByteBuffer> 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<ByteBuffer> 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
@@ -24,15 +24,15 @@
 
 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()
         {
             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;
     }