Repository: netty/netty
Issue: Provide a way to disable recycling completely
Labels: feature

It seems like some users are observing an edge case where the `WeakOrderQueue` and `Stack` grow beyond its max capacity. Although we should fix it, there should be a way for a user to disable recycling completely so that he or she can perform with-recycler vs. without-recycler comparison.

/cc @ninja-


Issue comments:
@trustin should I take care?

> Am 27.08.2015 um 05:11 schrieb Trustin Lee notifications@github.com:
> 
> It seems like some users are observing an edge case where the WeakOrderQueue and Stack grow beyond its max capacity. Although we should fix it, there should be a way for a user to disable recycling completely so that he or she can perform with-recycler vs. without-recycler comparison.
> 
> /cc @ninja-
> 
> —
> Reply to this email directly or view it on GitHub.

Do you have spare cycles? Then please feel free to take it. :-)

@trustin spare cycles not really but let me fix it anyway =P

Given https://github.com/netty/netty/issues/4134#issuecomment-135484445 do we feel this is still worth it?  I think we should push to diagnose / solve issues rather than disable features.

@Scottmitch I think it still make sense to allow it for people that have limited memory constraints etc. 

@normanmaurer @Scottmitch looking at Recycler, isn't setting the size to 0 giving that effect? 

@ninja- - We currently don't allow the property to be set to 0 (see https://github.com/netty/netty/blob/master/common/src/main/java/io/netty/util/Recycler.java#L51). However doesn't seem like it would hurt if we did allow 0... @normanmaurer - Is this what you had in mind?

@Scottmitch it's fine when set to 1 [per thread]. I think we can keep it like that. I am using it on production since today.

Regarding the Recycler's logic...I think it's a wrong way to approach this problem :) the "max number" should be used for the case when there are _really that many objects_ used _at the same time_. And not just increasing on every recycle()! That makes no sense even for heavy http whatnot apps!
Based on that, the cache should be allowed to grow to the _max_.

I would think about an another condition to deny caching around here, https://github.com/netty/netty/blob/4.0/common/src/main/java/io/netty/util/Recycler.java#L383 @trustin 

For example: let's track the balance between .get() and .recycle() :)?

get: balance++
recycle: balance--
push: if (size >= maxCapacity || size >= balance) return; (something like that)

that seems sane to me because I wouldn't trust current recycler with _any_ app, not only my proxy

@ninja- - I don't fully understand your request. Are you asking for a way to be able to "purge" entries and reduce the size of the stack?  Maybe submit a PR so we can get a better idea?

@Scottmitch not purging but the logic should include shrinking also. I am saying that the _current logic_ doesn't suit _any_ type of the app. I proposed a new type of logic for Recycler that would cache the objects, but to a reasonable extent. 

I got rid of recycler by setting max to 1, tested on production, everything great, happy with it.
I am discussing this just to improve netty.

Fixed

@ninja- - Please open another issue for a feature request for your "purging" / "shrinking" Recycler entries


Pull request review comments:
Should we stick to this default value? /cc @normanmaurer @tea-dragon 

@trustin for now yes. Let us use a new pr if we want to adjust it


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 edc7672c7c6..1e0bc39e260 100644
--- a/common/src/main/java/io/netty/util/Recycler.java
+++ b/common/src/main/java/io/netty/util/Recycler.java
@@ -36,8 +36,11 @@ public abstract class Recycler<T> {
 
     private static final InternalLogger logger = InternalLoggerFactory.getInstance(Recycler.class);
 
+    private static final Handle NOOP_HANDLE = new Handle() { };
     private static final AtomicInteger ID_GENERATOR = new AtomicInteger(Integer.MIN_VALUE);
     private static final int OWN_THREAD_ID = ID_GENERATOR.getAndIncrement();
+    // TODO: Some arbitrary large number - should adjust as we get more production experience.
+    private static final int DEFAULT_INITIAL_MAX_CAPACITY = 262144;
     private static final int DEFAULT_MAX_CAPACITY;
     private static final int INITIAL_CAPACITY;
 
@@ -45,15 +48,19 @@ public abstract class Recycler<T> {
         // In the future, we might have different maxCapacity for different object types.
         // e.g. io.netty.recycler.maxCapacity.writeTask
         //      io.netty.recycler.maxCapacity.outboundBuffer
-        int maxCapacity = SystemPropertyUtil.getInt("io.netty.recycler.maxCapacity.default", 0);
-        if (maxCapacity <= 0) {
-            // TODO: Some arbitrary large number - should adjust as we get more production experience.
-            maxCapacity = 262144;
+        int maxCapacity = SystemPropertyUtil.getInt("io.netty.recycler.maxCapacity.default",
+                                                    DEFAULT_INITIAL_MAX_CAPACITY);
+        if (maxCapacity < 0) {
+            maxCapacity = DEFAULT_INITIAL_MAX_CAPACITY;
         }
 
         DEFAULT_MAX_CAPACITY = maxCapacity;
         if (logger.isDebugEnabled()) {
-            logger.debug("-Dio.netty.recycler.maxCapacity.default: {}", DEFAULT_MAX_CAPACITY);
+            if (DEFAULT_MAX_CAPACITY == 0) {
+                logger.debug("-Dio.netty.recycler.maxCapacity.default: disabled");
+            } else {
+                logger.debug("-Dio.netty.recycler.maxCapacity.default: {}", DEFAULT_MAX_CAPACITY);
+            }
         }
 
         INITIAL_CAPACITY = Math.min(DEFAULT_MAX_CAPACITY, 256);
@@ -77,6 +84,9 @@ protected Recycler(int maxCapacity) {
 
     @SuppressWarnings("unchecked")
     public final T get() {
+        if (maxCapacity == 0) {
+            return newObject(NOOP_HANDLE);
+        }
         Stack<T> stack = threadLocal.get();
         DefaultHandle handle = stack.pop();
         if (handle == null) {
@@ -87,6 +97,10 @@ public final T get() {
     }
 
     public final boolean recycle(T o, Handle handle) {
+        if (handle == NOOP_HANDLE) {
+            return false;
+        }
+
         DefaultHandle h = (DefaultHandle) handle;
         if (h.stack.parent != this) {
             return false;


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 e90957034c6..cae4cc2e34e 100644
--- a/common/src/test/java/io/netty/util/RecyclerTest.java
+++ b/common/src/test/java/io/netty/util/RecyclerTest.java
@@ -40,6 +40,15 @@ public void testRecycle() {
         object2.recycle();
     }
 
+    @Test
+    public void testRecycleDisable() {
+        DisabledRecyclableObject object = DisabledRecyclableObject.newInstance();
+        object.recycle();
+        DisabledRecyclableObject object2 = DisabledRecyclableObject.newInstance();
+        assertNotSame(object, object2);
+        object2.recycle();
+    }
+
     static final class RecyclableObject {
 
         private static final Recycler<RecyclableObject> RECYCLER = new Recycler<RecyclableObject>() {
@@ -64,6 +73,30 @@ public void recycle() {
         }
     }
 
+    static final class DisabledRecyclableObject {
+
+        private static final Recycler<DisabledRecyclableObject> RECYCLER = new Recycler<DisabledRecyclableObject>(-1) {
+            @Override
+            protected DisabledRecyclableObject newObject(Handle handle) {
+                return new DisabledRecyclableObject(handle);
+            }
+        };
+
+        private final Recycler.Handle handle;
+
+        private DisabledRecyclableObject(Recycler.Handle handle) {
+            this.handle = handle;
+        }
+
+        public static DisabledRecyclableObject newInstance() {
+            return RECYCLER.get();
+        }
+
+        public void recycle() {
+            RECYCLER.recycle(this, handle);
+        }
+    }
+
     /**
      * Test to make sure bug #2848 never happens again
      * https://github.com/netty/netty/issues/2848