From 243b2b9f193c8459c56ad9196401c46f9584b7e4 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Wed, 23 Nov 2016 13:26:56 +0100
Subject: [PATCH] PooledByteBufAllocatorTest may has memory visiblity issues as
 it uses non concurrent queue

Motivation:

PooledByteBufAllocatorTest uses an ArrayQueue but access it from multiple threads (not concurrently but still from different threads). This may leak to memory visibility issues.

Modifications:

- Use a concurrent queue
- Some cleanup

Result:

Non racy test code.
---
 .../buffer/PooledByteBufAllocatorTest.java    | 29 +++++++++----------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/buffer/src/test/java/io/netty/buffer/PooledByteBufAllocatorTest.java b/buffer/src/test/java/io/netty/buffer/PooledByteBufAllocatorTest.java
index 1035ea65bf..9f17abad5d 100644
--- a/buffer/src/test/java/io/netty/buffer/PooledByteBufAllocatorTest.java
+++ b/buffer/src/test/java/io/netty/buffer/PooledByteBufAllocatorTest.java
@@ -21,12 +21,13 @@ import io.netty.util.concurrent.FastThreadLocalThread;
 import io.netty.util.internal.SystemPropertyUtil;
 import org.junit.Test;
 
-import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.LockSupport;
 
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
@@ -339,11 +340,9 @@ public class PooledByteBufAllocatorTest {
             }
         }
 
-        private final CountDownLatch latch = new CountDownLatch(1);
-        private final Queue<ByteBuf> buffers = new ArrayDeque<ByteBuf>(10);
+        private final Queue<ByteBuf> buffers = new ConcurrentLinkedQueue<ByteBuf>();
         private final ByteBufAllocator allocator;
-        private volatile boolean finished;
-        private volatile Throwable error;
+        private final AtomicReference<Object> finish = new AtomicReference<Object>();
 
         public AllocationThread(ByteBufAllocator allocator) {
             this.allocator = allocator;
@@ -353,7 +352,7 @@ public class PooledByteBufAllocatorTest {
         public void run() {
             try {
                 int idx = 0;
-                while (!finished) {
+                while (finish.get() == null) {
                     for (int i = 0; i < 10; i++) {
                         buffers.add(allocator.directBuffer(
                                 ALLOCATION_SIZES[Math.abs(idx++ % ALLOCATION_SIZES.length)],
@@ -362,12 +361,10 @@ public class PooledByteBufAllocatorTest {
                     releaseBuffers();
                 }
             } catch (Throwable cause) {
-                error = cause;
-                finished = true;
+                finish.set(cause);
             } finally {
                 releaseBuffers();
             }
-            latch.countDown();
         }
 
         private void releaseBuffers() {
@@ -381,22 +378,24 @@ public class PooledByteBufAllocatorTest {
         }
 
         public boolean isFinished() {
-            return finished;
+            return finish.get() != null;
         }
 
         public void finish() throws Throwable {
             try {
-                finished = true;
-                latch.await();
-                checkForError();
+                // Mark as finish if not already done but ensure we not override the previous set error.
+                finish.compareAndSet(null, Boolean.TRUE);
+                join();
             } finally {
                 releaseBuffers();
             }
+            checkForError();
         }
 
         public void checkForError() throws Throwable {
-            if (error != null) {
-                throw error;
+            Object obj = finish.get();
+            if (obj instanceof Throwable) {
+                throw (Throwable) obj;
             }
         }
     }