From 1946351fcef535e86547bea1f9394936e9a62dc3 Mon Sep 17 00:00:00 2001 From: Clebert Suconic Date: Mon, 25 Apr 2022 18:43:11 -0400 Subject: [PATCH] ARTEMIS-3802 Avoiding races on deleteWithID and Iterator.remove --- .../utils/collections/LinkedListImpl.java | 82 ++++++++++-------- .../tests/unit/util/LinkedListTest.java | 83 +++++++++++++++++++ 2 files changed, 129 insertions(+), 36 deletions(-) diff --git a/artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/LinkedListImpl.java b/artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/LinkedListImpl.java index 6648ba0e006..daf65e5516b 100644 --- a/artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/LinkedListImpl.java +++ b/artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/LinkedListImpl.java @@ -103,7 +103,7 @@ public void addHead(E 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 node = nodeStore.getNode(listID, id); @@ -422,67 +422,77 @@ public void repeat() { @Override public boolean hasNext() { - Node e = getNode(); + synchronized (LinkedListImpl.this) { + Node 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 = getNode(); + synchronized (LinkedListImpl.this) { + Node 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(); } } - } - if (e == null || e == last) { - if (canAdvance()) { - advance(); + last = e; - e = getNode(); - } else { - throw new NoSuchElementException(); - } - } - - last = e; - - repeat = false; + repeat = false; - return e.val(); + 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) { - throw new NoSuchElementException(); - } + if (current == null) { + return; + } + + Node prev = current.prev; - LinkedListImpl.this.removeAfter(current.prev); + if (prev != null) { + LinkedListImpl.this.removeAfter(prev); - last = null; + last = null; + } + } } @Override diff --git a/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/util/LinkedListTest.java b/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/util/LinkedListTest.java index be4619cc561..edd1590aa2d 100644 --- a/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/util/LinkedListTest.java +++ b/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/util/LinkedListTest.java @@ -22,6 +22,12 @@ 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 @@ private void internalAddWithID(boolean deferSupplier) { } } + + @Test + public void testRaceRemovingID() throws Exception { + ExecutorService executor = Executors.newFixedThreadPool(2); + + int elements = 1000; + try { + LinkedListImpl 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 objs = new LinkedListImpl<>();