Repository: netty/netty
Issue: Recycler maxCapcity can be ignored
Labels: defect

If `maxCapcity` is not a power of two and greater than 256, it's completely ignored by `Recycler`. This allows caching an infinite number of `Recycler` objects until memory is exhausted.

`Recycler.Stack.push()` only checks against `maxCapacity` when the `elements` array is full. However, that array is reallocated in powers of two. It starts out with 256 items and then goes to 512, 1024, etc. When the array is full, it checks if the current size _equals_ `maxCapacity` instead of greater than it.

So if `maxCapacity` is 500, the array size will grow to 512, it will fill up, and will never stop growing because `elements.length` will never be equal to `maxCapacity` at times when the array is full.

Here is a little piece of code to show this in action. When `maxCapacity` is 300, it will cache 1000 objects. When setting `maxCapcity` to 512, it will properly cache only 512 objects.

``` java
import io.netty.util.Recycler;
import io.netty.util.Recycler.Handle;


public class TestRecycler
{
    static int counter;


    public static class HandledObject
    {
        Handle handle;


        HandledObject( Handle handle )
        {
            this.handle = handle;
            counter++;
        }


        @Override
        protected void finalize() throws Throwable
        {
            counter--;
        }
    }


    public static void main( String[] args )
    {
        // int maxCapcity = 512;
        int maxCapcity = 300;

        Recycler<HandledObject> recycler = new Recycler<HandledObject>( maxCapcity )
        {
            @Override
            protected HandledObject newObject( Handle handle )
            {
                return new HandledObject( handle );
            }
        };

        HandledObject[] objects = new HandledObject[1000];
        for( int i = 0; i < objects.length; i++ ) {
            objects[i] = recycler.get();
        }

        for( int i = 0; i < objects.length; i++ ) {
            recycler.recycle( objects[i], objects[i].handle );
            objects[i] = null;
        }

        System.gc();
        System.runFinalization();

        System.out.println( "maxCapcity = " + maxCapcity + " and actual live objects = " + counter );
    }

}
```


Issue comments:
Pull request #2849 has a proposed fix.

Was fixed 


Pull request review comments:
you should only check for `==`. `size` can never be greater than `maxCapacity`.

`testMaxCapacity` and add a link to your bug report please.

maybe wrap everything in a loop that tests this with a few random `maxCapacity` values between [1,1000).

Well, that was actually part of the issue. The original commit (8d2ddae) checked with `==`, but also had `Math.max()` for proper capacity. That was later removed for some reason. If the original code had checked with `>=`, this bug wouldn't have been as bad as it is. The capacity would still be capped at the next power of two. It wouldn't have been unbounded.

With the new unit test, this kind of breakage during random change should be caught early. I personally prefer to still leave room for error. But if you want me to change to `==`, I will be glad to do it.

I think this method should be package private and maybe `threadLocalCapacity()` would be a more accurate name?

> I personally prefer to still leave room for error. But if you want me to change to ==, I will be glad to do it.

I see what you mean, however I would argue that if the code is correct, `>` cannot happen and thus  `==` makes much more sense when someone else reads the code a year from now :)

Sure.

Done.

Added 50 random numbers between 256 and 1256.

Done.

you are missing an `a` after `Cap` in the name :-). Also we usually use javadoc comments like this

```
/**
  * Test for issue https://github.com/netty/netty/issues/2848
  */
```


Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/Recycler.java b/common/src/main/java/io/netty/util/Recycler.java
index a56499cd7b8..0600f66d70f 100644
--- a/common/src/main/java/io/netty/util/Recycler.java
+++ b/common/src/main/java/io/netty/util/Recycler.java
@@ -96,6 +96,10 @@ public final boolean recycle(T o, Handle<T> handle) {
         return true;
     }
 
+    final int threadLocalCapacity() {
+        return threadLocal.get().elements.length;
+    }
+
     protected abstract T newObject(Handle<T> handle);
 
     public interface Handle<T> {
@@ -339,12 +343,12 @@ void push(DefaultHandle<?> item) {
             item.recycleId = item.lastRecycledId = OWN_THREAD_ID;
 
             int size = this.size;
+            if (size == maxCapacity) {
+                // Hit the maximum capacity - drop the possibly youngest object.
+                return;
+            }
             if (size == elements.length) {
-                if (size == maxCapacity) {
-                    // Hit the maximum capacity - drop the possibly youngest object.
-                    return;
-                }
-                elements = Arrays.copyOf(elements, size << 1);
+                elements = Arrays.copyOf(elements, Math.min(size << 1, maxCapacity));
             }
 
             elements[size] = item;


Test patch:
diff --git a/common/src/test/java/io/netty/util/RecyclerTest.java b/common/src/test/java/io/netty/util/RecyclerTest.java
index 5212e9b4df5..91114344b6a 100644
--- a/common/src/test/java/io/netty/util/RecyclerTest.java
+++ b/common/src/test/java/io/netty/util/RecyclerTest.java
@@ -15,6 +15,8 @@
 */
 package io.netty.util;
 
+import java.util.Random;
+
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -59,4 +61,47 @@ public void recycle() {
             RECYCLER.recycle(this, handle);
         }
     }
+
+    /**
+     * Test to make sure bug #2848 never happens again
+     * https://github.com/netty/netty/issues/2848
+     */
+    @Test
+    public void testMaxCapacity() {
+        testMaxCapacity(300);
+        Random rand = new Random();
+        for (int i = 0; i < 50; i++) {
+            testMaxCapacity(rand.nextInt(1000) + 256); // 256 - 1256
+        }
+    }
+
+    void testMaxCapacity(int maxCapacity) {
+        Recycler<HandledObject> recycler = new Recycler<HandledObject>(maxCapacity) {
+            @Override
+            protected HandledObject newObject(
+                    Recycler.Handle<HandledObject> handle) {
+                return new HandledObject(handle);
+            }
+        };
+
+        HandledObject[] objects = new HandledObject[maxCapacity * 3];
+        for (int i = 0; i < objects.length; i++) {
+            objects[i] = recycler.get();
+        }
+
+        for (int i = 0; i < objects.length; i++) {
+            recycler.recycle(objects[i], objects[i].handle);
+            objects[i] = null;
+        }
+
+        Assert.assertEquals(maxCapacity, recycler.threadLocalCapacity());
+    }
+
+    static final class HandledObject {
+        Recycler.Handle<HandledObject> handle;
+
+        HandledObject(Recycler.Handle<HandledObject> handle) {
+            this.handle = handle;
+        }
+    }
 }