Repository: netty/netty
Issue: recycler has problems recycling objects in a multi-threaded environment

### Expected behavior

An object is collected in a multi-threaded environment and only exists in one queue.

### Actual behavior

An object is collected in multiple queues in a multithreaded environment.

### Steps to reproduce

```
public void testMultipleRecycleAtDifferentThread() throws InterruptedException {
        Recycler<HandledObject> recycler = newRecycler(1024);
        final HandledObject object = recycler.get();
        final AtomicReference<IllegalStateException> exceptionStore = new AtomicReference<IllegalStateException>();

        final CountDownLatch countDownLatch = new CountDownLatch(2);
        final Thread thread1 = new Thread(new Runnable() {
            @Override
            public void run() {
                try{
                    object.recycle();
                }finally {
                    countDownLatch.countDown();
                }
                
            }
        });
        thread1.start();
        //thread1.join();

        final Thread thread2 = new Thread(new Runnable() {
            @Override
            public void run() {
                try {
                    object.recycle();
                } catch (IllegalStateException e) {
                    exceptionStore.set(e);
                } finally {
                    countDownLatch.countDown();
                }
            }
        });
        thread2.start();
        //thread2.join();
        
        countDownLatch.await();
        recycler.get();
        IllegalStateException exception = exceptionStore.get();
        if (exception != null) {
            throw exception;
        }
    }
```

Breakpoint debugging recycler.get() method, you will find that there are two queues in the stack, holding the same object.

### Minimal yet complete reproducer code (or URL to code)

### Netty version

4.1.59.Final-SNAPSHOT

### JVM version (e.g. `java -version`)

1.8

### OS version (e.g. `uname -a`)

masos

Issue comments:
Hmm... I can't see this. Can you make a screenshot ?
like this.

<img width="669" alt="stack" src="https://user-images.githubusercontent.com/12539730/107210938-d1300080-6a3f-11eb-8760-2e754935ea67.png">




@chrisvest can you have a look ?
@normanmaurer Yeah, I'll try to spend some time on this.

Pull request review comments:
You can just extends `AtomicInteger` that would give you back `Unsafe` perf for free :)

Yeah I know that's bad design so you can ignore me ;)
I considered it, but then I loose the field name which is quite awkward. Also, I think the JIT is able to trust the finals inside A\*FUs these days, so they're as fast as VarHandles or Atomic\*s these days.

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 9e81aa527d0..d64889488ee 100644
--- a/common/src/main/java/io/netty/util/Recycler.java
+++ b/common/src/main/java/io/netty/util/Recycler.java
@@ -27,6 +27,7 @@
 import java.util.Map;
 import java.util.WeakHashMap;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 
 import static io.netty.util.internal.MathUtil.safeFindNextPositivePowerOfTwo;
 import static java.lang.Math.max;
@@ -209,8 +210,16 @@ final int threadLocalSize() {
 
     public interface Handle<T> extends ObjectPool.Handle<T>  { }
 
+    @SuppressWarnings("unchecked")
     private static final class DefaultHandle<T> implements Handle<T> {
-        int lastRecycledId;
+        private static final AtomicIntegerFieldUpdater<DefaultHandle<?>> LAST_RECYCLED_ID_UPDATER;
+        static {
+            AtomicIntegerFieldUpdater<?> updater = AtomicIntegerFieldUpdater.newUpdater(
+                    DefaultHandle.class, "lastRecycledId");
+            LAST_RECYCLED_ID_UPDATER = (AtomicIntegerFieldUpdater<DefaultHandle<?>>) updater;
+        }
+
+        volatile int lastRecycledId;
         int recycleId;
 
         boolean hasBeenRecycled;
@@ -235,6 +244,12 @@ public void recycle(Object object) {
 
             stack.push(this);
         }
+
+        public boolean compareAndSetLastRecycledId(int expectLastRecycledId, int updateLastRecycledId) {
+            // Use "weak…" because we do not need synchronize-with ordering, only atomicity.
+            // Also, spurious failures are fine, since no code should rely on recycling for correctness.
+            return LAST_RECYCLED_ID_UPDATER.weakCompareAndSet(this, expectLastRecycledId, updateLastRecycledId);
+        }
     }
 
     private static final FastThreadLocal<Map<Stack<?>, WeakOrderQueue>> DELAYED_RECYCLED =
@@ -371,11 +386,15 @@ void setNext(WeakOrderQueue next) {
 
         void reclaimAllSpaceAndUnlink() {
             head.reclaimAllSpaceAndUnlink();
-            this.next = null;
+            next = null;
         }
 
         void add(DefaultHandle<?> handle) {
-            handle.lastRecycledId = id;
+            if (!handle.compareAndSetLastRecycledId(0, id)) {
+                // Separate threads could be racing to add the handle to each their own WeakOrderQueue.
+                // We only add the handle to the queue if we win the race and observe that lastRecycledId is zero.
+                return;
+            }
 
             // While we also enforce the recycling ratio when we transfer objects from the WeakOrderQueue to the Stack
             // we better should enforce it as well early. Missing to do so may let the WeakOrderQueue grow very fast
@@ -649,10 +668,10 @@ void push(DefaultHandle<?> item) {
         }
 
         private void pushNow(DefaultHandle<?> item) {
-            if ((item.recycleId | item.lastRecycledId) != 0) {
+            if (item.recycleId != 0 || !item.compareAndSetLastRecycledId(0, OWN_THREAD_ID)) {
                 throw new IllegalStateException("recycled already");
             }
-            item.recycleId = item.lastRecycledId = OWN_THREAD_ID;
+            item.recycleId = OWN_THREAD_ID;
 
             int size = this.size;
             if (size >= maxCapacity || dropHandle(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 1d711f5e4a4..0c2d0705606 100644
--- a/common/src/test/java/io/netty/util/RecyclerTest.java
+++ b/common/src/test/java/io/netty/util/RecyclerTest.java
@@ -18,6 +18,7 @@
 import org.junit.Test;
 
 import java.util.Random;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
@@ -114,12 +115,119 @@ public void run() {
         });
         thread2.start();
         thread2.join();
+        HandledObject a = recycler.get();
+        HandledObject b = recycler.get();
+        assertNotSame(a, b);
         IllegalStateException exception = exceptionStore.get();
         if (exception != null) {
             throw exception;
         }
     }
 
+    @Test
+    public void testMultipleRecycleAtDifferentThreadRacing() throws InterruptedException {
+        Recycler<HandledObject> recycler = newRecycler(1024);
+        final HandledObject object = recycler.get();
+        final AtomicReference<IllegalStateException> exceptionStore = new AtomicReference<IllegalStateException>();
+
+        final CountDownLatch countDownLatch = new CountDownLatch(2);
+        final Thread thread1 = new Thread(new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    object.recycle();
+                } catch (IllegalStateException e) {
+                    Exception x = exceptionStore.getAndSet(e);
+                    if (x != null) {
+                        e.addSuppressed(x);
+                    }
+                } finally {
+                    countDownLatch.countDown();
+                }
+            }
+        });
+        thread1.start();
+
+        final Thread thread2 = new Thread(new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    object.recycle();
+                } catch (IllegalStateException e) {
+                    Exception x = exceptionStore.getAndSet(e);
+                    if (x != null) {
+                        e.addSuppressed(x);
+                    }
+                } finally {
+                    countDownLatch.countDown();
+                }
+            }
+        });
+        thread2.start();
+
+        try {
+            countDownLatch.await();
+            HandledObject a = recycler.get();
+            HandledObject b = recycler.get();
+            assertNotSame(a, b);
+            IllegalStateException exception = exceptionStore.get();
+            if (exception != null) {
+                assertEquals("recycled already", exception.getMessage());
+                assertEquals(0, exception.getSuppressed().length);
+            }
+        } finally {
+            thread1.join(1000);
+            thread2.join(1000);
+        }
+    }
+
+    @Test
+    public void testMultipleRecycleRacing() throws InterruptedException {
+        Recycler<HandledObject> recycler = newRecycler(1024);
+        final HandledObject object = recycler.get();
+        final AtomicReference<IllegalStateException> exceptionStore = new AtomicReference<IllegalStateException>();
+
+        final CountDownLatch countDownLatch = new CountDownLatch(1);
+        final Thread thread1 = new Thread(new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    object.recycle();
+                } catch (IllegalStateException e) {
+                    Exception x = exceptionStore.getAndSet(e);
+                    if (x != null) {
+                        e.addSuppressed(x);
+                    }
+                } finally {
+                    countDownLatch.countDown();
+                }
+            }
+        });
+        thread1.start();
+
+        try {
+            object.recycle();
+        } catch (IllegalStateException e) {
+            Exception x = exceptionStore.getAndSet(e);
+            if (x != null) {
+                e.addSuppressed(x);
+            }
+        }
+
+        try {
+            countDownLatch.await();
+            HandledObject a = recycler.get();
+            HandledObject b = recycler.get();
+            assertNotSame(a, b);
+            IllegalStateException exception = exceptionStore.get();
+            if (exception != null) {
+                throw exception;
+            }
+        } finally {
+            thread1.join(1000);
+        }
+    }
+
     @Test
     public void testRecycle() {
         Recycler<HandledObject> recycler = newRecycler(1024);