Cleanup non-retainable Retainables

`Retainable`s that return false from `canRetain` now are noops if `retain` is called, which allows for a simpler calling convention. AsyncContent has also been reworked to allocate less and be clearer in its use of `canRetain`.
jetty · Jan 13, 2023 · 4655b61 · 4655b61
1 parent 46355c6
commit 4655b61
Show file tree

Hide file tree

Showing 26 changed files with 165 additions and 209 deletions.
diff --git a/...core/jetty-client/src/main/java/org/eclipse/jetty/client/InputStreamResponseListener.java b/...core/jetty-client/src/main/java/org/eclipse/jetty/client/InputStreamResponseListener.java
@@ -112,8 +112,7 @@ public void onContent(Response response, Content.Chunk chunk, Runnable demander)
             {
                 if (LOG.isDebugEnabled())
                     LOG.debug("Queueing chunk {}", chunk);
-                if (chunk.canRetain())
-                    chunk.retain();
+                chunk.retain();
                 chunkCallbacks.add(new ChunkCallback(chunk, demander, response::abort));
                 l.signalAll();
                 return;

diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/internal/HttpReceiver.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/internal/HttpReceiver.java
@@ -574,7 +574,7 @@ protected Content.Chunk transform(Content.Chunk inputChunk)
                     return _chunk;
 
                 // Retain the input chunk because its ByteBuffer will be referenced by the Inflater.
-                if (retain && _chunk.canRetain())
+                if (retain)
                     _chunk.retain();
                 if (LOG.isDebugEnabled())
                     LOG.debug("decoding: {}", _chunk);

diff --git a/...y-core/jetty-client/src/main/java/org/eclipse/jetty/client/internal/ResponseNotifier.java b/...y-core/jetty-client/src/main/java/org/eclipse/jetty/client/internal/ResponseNotifier.java
@@ -412,8 +412,7 @@ private void onChunk(Content.Chunk chunk)
                     if (chunk.hasRemaining())
                         chunk = Content.Chunk.asChunk(chunk.getByteBuffer().slice(), chunk.isLast(), chunk);
                     // Retain the slice because it is stored for later reads.
-                    if (chunk.canRetain())
-                        chunk.retain();
+                    chunk.retain();
                     this.chunk = chunk;
                 }
                 else if (!currentChunk.isLast())

diff --git a/.../src/main/java/org/eclipse/jetty/fcgi/client/transport/internal/HttpReceiverOverFCGI.java b/.../src/main/java/org/eclipse/jetty/fcgi/client/transport/internal/HttpReceiverOverFCGI.java
@@ -99,8 +99,7 @@ void content(Content.Chunk chunk)
         if (this.chunk != null)
             throw new IllegalStateException();
         // Retain the chunk because it is stored for later reads.
-        if (chunk.canRetain())
-            chunk.retain();
+        chunk.retain();
         this.chunk = chunk;
         responseContentAvailable();
     }

diff --git a/...-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java b/...-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java
@@ -192,8 +192,7 @@ private void notifyContentAvailable()
     public void onContent(Content.Chunk chunk)
     {
         // Retain the chunk because it is stored for later reads.
-        if (chunk.canRetain())
-            chunk.retain();
+        chunk.retain();
         _chunk = chunk;
     }
 

diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java
@@ -284,8 +284,7 @@ private boolean isFailed()
         public void onPartContent(Content.Chunk chunk)
         {
             // Retain the chunk because it is stored for later use.
-            if (chunk.canRetain())
-                chunk.retain();
+            chunk.retain();
             partChunks.add(chunk);
         }
 

diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java
@@ -444,8 +444,7 @@ public void onPartContent(Content.Chunk chunk)
                 }
             }
             // Retain the chunk because it is stored for later use.
-            if (chunk.canRetain())
-                chunk.retain();
+            chunk.retain();
             partChunks.add(chunk);
         }
 

diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/Trailers.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/Trailers.java
@@ -43,22 +43,4 @@ public HttpFields getTrailers()
     {
         return trailers;
     }
-
-    @Override
-    public boolean canRetain()
-    {
-        return false;
-    }
-
-    @Override
-    public void retain()
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public boolean release()
-    {
-        return true;
-    }
 }
diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java
@@ -690,8 +690,7 @@ private static class TestPartsListener extends MultiPart.AbstractPartsListener
         public void onPartContent(Content.Chunk chunk)
         {
             // Retain the chunk because it is stored for later use.
-            if (chunk.canRetain())
-                chunk.retain();
+            chunk.retain();
             partContent.add(chunk);
         }
 

diff --git a/...core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java b/...core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/api/Stream.java
@@ -432,24 +432,6 @@ public EOF(int streamId)
             {
                 super(new DataFrame(streamId, BufferUtil.EMPTY_BUFFER, true));
             }
-
-            @Override
-            public boolean canRetain()
-            {
-                return false;
-            }
-
-            @Override
-            public void retain()
-            {
-                throw new UnsupportedOperationException();
-            }
-
-            @Override
-            public boolean release()
-            {
-                return true;
-            }
         }
     }
 }
diff --git a/...core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/api/Stream.java b/...core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/api/Stream.java
@@ -411,24 +411,6 @@ private EOFData()
             {
                 super(new DataFrame(BufferUtil.EMPTY_BUFFER, true));
             }
-
-            @Override
-            public boolean canRetain()
-            {
-                return false;
-            }
-
-            @Override
-            public void retain()
-            {
-                throw new UnsupportedOperationException();
-            }
-
-            @Override
-            public boolean release()
-            {
-                return true;
-            }
         }
     }
 }
diff --git a/...-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/internal/HTTP3Stream.java b/...-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/internal/HTTP3Stream.java
@@ -335,8 +335,7 @@ private void onDataAvailable()
     public void onData(Data data)
     {
         // Retain the data because it is stored for later reads.
-        if (data.canRetain())
-            data.retain();
+        data.retain();
         if (!dataRef.compareAndSet(null, data))
             throw new IllegalStateException();
 

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
@@ -436,11 +436,50 @@ public interface Chunk extends Retainable
         /**
          * <p>An empty, non-last, chunk.</p>
          */
-        Chunk EMPTY = ByteBufferChunk.EMPTY;
+        Chunk EMPTY = new Chunk()
+        {
+            @Override
+            public ByteBuffer getByteBuffer()
+            {
+                return BufferUtil.EMPTY_BUFFER;
+            }
+
+            @Override
+            public boolean isLast()
+            {
+                return false;
+            }
+
+            @Override
+            public String toString()
+            {
+                return "EMPTY";
+            }
+        };
+
         /**
          * <p>An empty, last, chunk.</p>
          */
-        Content.Chunk EOF = ByteBufferChunk.EOF;
+        Content.Chunk EOF = new Chunk()
+        {
+            @Override
+            public ByteBuffer getByteBuffer()
+            {
+                return BufferUtil.EMPTY_BUFFER;
+            }
+
+            @Override
+            public boolean isLast()
+            {
+                return true;
+            }
+
+            @Override
+            public String toString()
+            {
+                return "EOF";
+            }
+        };
 
         /**
          * <p>Creates a Chunk with the given ByteBuffer.</p>
@@ -657,24 +696,6 @@ public boolean isLast()
                 return true;
             }
 
-            @Override
-            public boolean canRetain()
-            {
-                return false;
-            }
-
-            @Override
-            public void retain()
-            {
-                throw new UnsupportedOperationException();
-            }
-
-            @Override
-            public boolean release()
-            {
-                return true;
-            }
-
             @Override
             public String toString()
             {

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
@@ -27,30 +27,36 @@
 public interface Retainable
 {
     /**
-     * <p>Returns whether this resource can be retained, that is whether {@link #retain()}
-     * can be called safely.</p>
-     * <p>Implementations may decide that special resources are not retainable (for example,
-     * {@code static} constants) so calling {@link #retain()} is not safe because it may throw.</p>
-     * <p>Calling {@link #release()} on those special resources is typically allowed, and
-     * it is a no-operation.</p>
+     * <p>Returns whether this resource is referenced counted by calls to {@link #retain()}
+     * and {@link #release()}.</p>
+     * <p>Implementations may decide that special resources are not not referenced counted (for example,
+     * {@code static} constants) so calling {@link #retain()} is a no-operation, and
+     * calling {@link #release()} on those special resources is a no-operation that always returns true.</p>
      *
-     * @return whether it is safe to call {@link #retain()}
+     * @return true if calls to {@link #retain()} are reference counted.
      */
-    boolean canRetain();
+    default boolean canRetain()
+    {
+        return false;
+    }
 
     /**
-     * <p>Retains this resource, incrementing the reference count.</p>
+     * <p>Retains this resource, potentially incrementing a reference count if there are resources that will be released.</p>
      */
-    void retain();
+    default void retain()
+    {
+    }
 
     /**
-     * <p>Releases this resource, decrementing the reference count.</p>
-     * <p>This method returns {@code true} when the reference count goes to zero,
-     * {@code false} otherwise.</p>
+     * <p>Releases this resource, potentially decrementing a reference count (if any).</p>
      *
-     * @return whether the invocation of this method decremented the reference count to zero
+     * @return {@code true} when the reference count goes to zero or if there was no reference count,
+     *         {@code false} otherwise.
      */
-    boolean release();
+    default boolean release()
+    {
+        return true;
+    }
 
     /**
      * A wrapper of {@link Retainable} instances.