Fixes #4971 - Simplify Connection.upgradeFrom()/upgradeTo().

Strengthened ByteBufferPool behavior when releasing non-pooled ByteBuffers: the buffer is now discarded. Signed-off-by: Simone Bordet <[email protected]>
jetty · Jul 6, 2020 · 12b5371 · 12b5371
1 parent 3592521
commit 12b5371
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 28 deletions.
diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java
@@ -23,8 +23,11 @@
 import java.util.Objects;
 import java.util.function.IntFunction;
 
+import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.annotation.ManagedAttribute;
 import org.eclipse.jetty.util.annotation.ManagedObject;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
 
 /**
  * <p>A ByteBuffer pool where ByteBuffers are held in queues that are held in array elements.</p>
@@ -35,6 +38,8 @@
 @ManagedObject
 public class ArrayByteBufferPool extends AbstractByteBufferPool
 {
+    private static final Logger LOG = Log.getLogger(MappedByteBufferPool.class);
+
     private final int _minCapacity;
     private final ByteBufferPool.Bucket[] _direct;
     private final ByteBufferPool.Bucket[] _indirect;
@@ -119,8 +124,18 @@ public void release(ByteBuffer buffer)
     {
         if (buffer == null)
             return;
+
+        int capacity = buffer.capacity();
+        // Validate that this buffer is from this pool.
+        if ((capacity % getCapacityFactor()) != 0)
+        {
+            if (LOG.isDebugEnabled())
+                LOG.debug("ByteBuffer {} does not belong to this pool, discarding it", BufferUtil.toDetailString(buffer));
+            return;
+        }
+
         boolean direct = buffer.isDirect();
-        ByteBufferPool.Bucket bucket = bucketFor(buffer.capacity(), direct, this::newBucket);
+        ByteBufferPool.Bucket bucket = bucketFor(capacity, direct, this::newBucket);
         if (bucket != null)
         {
             bucket.release(buffer);

diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/MappedByteBufferPool.java b/jetty-io/src/main/java/org/eclipse/jetty/io/MappedByteBufferPool.java
@@ -28,6 +28,8 @@
 import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.annotation.ManagedAttribute;
 import org.eclipse.jetty.util.annotation.ManagedObject;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
 
 /**
  * <p>A ByteBuffer pool where ByteBuffers are held in queues that are held in a Map.</p>
@@ -38,6 +40,8 @@
 @ManagedObject
 public class MappedByteBufferPool extends AbstractByteBufferPool
 {
+    private static final Logger LOG = Log.getLogger(MappedByteBufferPool.class);
+
     private final ConcurrentMap<Integer, Bucket> _directBuffers = new ConcurrentHashMap<>();
     private final ConcurrentMap<Integer, Bucket> _heapBuffers = new ConcurrentHashMap<>();
     private final Function<Integer, Bucket> _newBucket;
@@ -127,7 +131,12 @@ public void release(ByteBuffer buffer)
 
         int capacity = buffer.capacity();
         // Validate that this buffer is from this pool.
-        assert ((capacity % getCapacityFactor()) == 0);
+        if ((capacity % getCapacityFactor()) != 0)
+        {
+            if (LOG.isDebugEnabled())
+                LOG.debug("ByteBuffer {} does not belong to this pool, discarding it", BufferUtil.toDetailString(buffer));
+            return;
+        }
 
         int b = bucketFor(capacity);
         boolean direct = buffer.isDirect();

diff --git a/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java b/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java
@@ -23,6 +23,7 @@
 import java.util.Objects;
 
 import org.eclipse.jetty.io.ByteBufferPool.Bucket;
+import org.eclipse.jetty.util.StringUtil;
 import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -150,6 +151,17 @@ public void testAcquireReleaseAcquire()
         }
     }
 
+    @Test
+    public void testReleaseNonPooledBuffer()
+    {
+        ArrayByteBufferPool bufferPool = new ArrayByteBufferPool();
+
+        // Release a few small non-pool buffers
+        bufferPool.release(ByteBuffer.wrap(StringUtil.getUtf8Bytes("Hello")));
+
+        assertEquals(0, bufferPool.getHeapByteBufferCount());
+    }
+
     @Test
     public void testMaxQueue()
     {

diff --git a/jetty-io/src/test/java/org/eclipse/jetty/io/MappedByteBufferPoolTest.java b/jetty-io/src/test/java/org/eclipse/jetty/io/MappedByteBufferPoolTest.java
@@ -35,7 +35,6 @@
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.junit.jupiter.api.Assertions.fail;
 
 public class MappedByteBufferPoolTest
 {
@@ -95,34 +94,15 @@ public void testAcquireReleaseClear()
         assertTrue(buckets.isEmpty());
     }
 
-    /**
-     * In a scenario where MappedByteBufferPool is being used improperly,
-     * such as releasing a buffer that wasn't created/acquired by the
-     * MappedByteBufferPool, an assertion is tested for.
-     */
     @Test
-    public void testReleaseAssertion()
+    public void testReleaseNonPooledBuffer()
     {
-        int factor = 1024;
-        MappedByteBufferPool bufferPool = new MappedByteBufferPool(factor);
+        MappedByteBufferPool bufferPool = new MappedByteBufferPool();
 
-        try
-        {
-            // Release a few small non-pool buffers
-            bufferPool.release(ByteBuffer.wrap(StringUtil.getUtf8Bytes("Hello")));
-
-            /* NOTES:
-             *
-             * 1) This test will pass on command line maven build, as its surefire setup uses "-ea" already.
-             * 2) In Eclipse, goto the "Run Configuration" for this test case.
-             *    Select the "Arguments" tab, and make sure "-ea" is present in the text box titled "VM arguments"
-             */
-            fail("Expected java.lang.AssertionError, do you have '-ea' JVM command line option enabled?");
-        }
-        catch (java.lang.AssertionError e)
-        {
-            // Expected path.
-        }
+        // Release a few small non-pool buffers
+        bufferPool.release(ByteBuffer.wrap(StringUtil.getUtf8Bytes("Hello")));
+
+        assertEquals(0, bufferPool.getHeapByteBufferCount());
     }
 
     @Test