ARTEMIS-3802 Avoiding races on deleteWithID and Iterator.remove

artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/LinkedListImpl.java

@@ -103,7 +103,7 @@ public class LinkedListImpl<E> implements LinkedList<E> {
    }
 
    @Override
-   public E removeWithID(String listID, long id) {
+   public synchronized E removeWithID(String listID, long id) {
       assert nodeStore != null; // it is assumed the code will call setNodeStore before callin removeWithID
 
       Node<E> node = nodeStore.getNode(listID, id);
@@ -422,67 +422,77 @@ public class LinkedListImpl<E> implements LinkedList<E> {
 
       @Override
       public boolean hasNext() {
-         Node<E> e = getNode();
+         synchronized (LinkedListImpl.this) {
+            Node<E> e = getNode();
 
-         if (e != null && (e != last || repeat)) {
-            return true;
+            if (e != null && (e != last || repeat)) {
+               return true;
+            }
+
+            return canAdvance();
          }
-
-         return canAdvance();
       }
 
       @Override
       public E next() {
-         Node<E> e = getNode();
+         synchronized (LinkedListImpl.this) {
+            Node<E> e = getNode();
 
-         if (repeat) {
-            repeat = false;
+            if (repeat) {
+               repeat = false;
 
-            if (e != null) {
-               return e.val();
-            } else {
+               if (e != null) {
+                  return e.val();
+               } else {
+                  if (canAdvance()) {
+                     advance();
+
+                     e = getNode();
+
+                     return e.val();
+                  } else {
+                     throw new NoSuchElementException();
+                  }
+               }
+            }
+
+            if (e == null || e == last) {
                if (canAdvance()) {
                   advance();
 
                   e = getNode();
-
-                  return e.val();
                } else {
                   throw new NoSuchElementException();
                }
             }
+
+            last = e;
+
+            repeat = false;
+
+            return e.val();
          }
-
-         if (e == null || e == last) {
-            if (canAdvance()) {
-               advance();
-
-               e = getNode();
-            } else {
-               throw new NoSuchElementException();
-            }
-         }
-
-         last = e;
-
-         repeat = false;
-
-         return e.val();
       }
 
       @Override
       public void remove() {
-         if (last == null) {
-            throw new NoSuchElementException();
+         synchronized (LinkedListImpl.this) {
+            if (last == null) {
+               throw new NoSuchElementException();
+            }
+
+            if (current == null) {
+               return;
+            }
+
+            Node<E> prev = current.prev;
+
+            if (prev != null) {
+               LinkedListImpl.this.removeAfter(prev);
+
+               last = null;
+            }
          }
-
-         if (current == null) {
-            throw new NoSuchElementException();
-         }
-
-         LinkedListImpl.this.removeAfter(current.prev);
-
-         last = null;
       }
 
       @Override

tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/util/LinkedListTest.java

@@ -22,6 +22,12 @@ import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.NoSuchElementException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import io.netty.util.collection.LongObjectHashMap;
 import org.apache.activemq.artemis.tests.util.ActiveMQTestBase;
@@ -313,6 +319,83 @@ public class LinkedListTest extends ActiveMQTestBase {
       }
    }
 
+
+   @Test
+   public void testRaceRemovingID() throws Exception {
+      ExecutorService executor = Executors.newFixedThreadPool(2);
+
+      int elements = 1000;
+      try {
+         LinkedListImpl<ObservableNode> objs = new LinkedListImpl<>();
+         ListNodeStore nodeStore = new ListNodeStore();
+         objs.setNodeStore(nodeStore);
+         final String serverID = RandomUtil.randomString();
+
+         for (int i = 0; i < elements; i++) {
+            objs.addHead(new ObservableNode(serverID, i));
+         }
+         Assert.assertEquals(elements, objs.size());
+
+         CyclicBarrier barrier = new CyclicBarrier(2);
+
+         CountDownLatch latch = new CountDownLatch(2);
+         AtomicInteger errors = new AtomicInteger(0);
+
+         executor.execute(() -> {
+            try {
+               barrier.await(10, TimeUnit.SECONDS);
+            } catch (Exception e) {
+               e.printStackTrace();
+            }
+
+            try {
+               for (int i = 0; i < elements; i++) {
+                  objs.removeWithID(serverID, i);
+               }
+            } catch (Exception e) {
+               e.printStackTrace();
+               errors.incrementAndGet();
+            } finally {
+               latch.countDown();
+            }
+         });
+
+         executor.execute(() -> {
+            LinkedListIterator iterator = objs.iterator();
+
+            try {
+               barrier.await(10, TimeUnit.SECONDS);
+            } catch (Exception e) {
+               e.printStackTrace();
+            }
+
+            try {
+               while (iterator.hasNext()) {
+                  Object value = iterator.next();
+                  iterator.remove();
+               }
+            } catch (Exception e) {
+               if (e instanceof NoSuchElementException) {
+                  // this is ok
+               } else {
+                  e.printStackTrace();
+                  errors.incrementAndGet();
+               }
+            } finally {
+               latch.countDown();
+            }
+         });
+
+         Assert.assertTrue(latch.await(10, TimeUnit.SECONDS));
+
+         Assert.assertEquals(0, objs.size());
+
+         Assert.assertEquals(0, errors.get());
+      } finally {
+         executor.shutdownNow();
+      }
+   }
+
    @Test
    public void testAddHeadAndRemove() {
       LinkedListImpl<ObservableNode> objs = new LinkedListImpl<>();