diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/InMemoryDuplicateIDCache.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/InMemoryDuplicateIDCache.java index 8f012280c09..6b410020682 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/InMemoryDuplicateIDCache.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/InMemoryDuplicateIDCache.java @@ -190,7 +190,7 @@ private synchronized void addToCacheInMemory(final ByteArray holder) { logger.trace("address = {} adding {}", address, describeID(holder.bytes)); } - cache.put(holder, cachedBoxedInts.apply(pos)); + final Integer slot = cachedBoxedInts.apply(pos); if (pos < ids.size()) { // Need fast array style access here -hence ArrayList typing @@ -202,7 +202,8 @@ private synchronized void addToCacheInMemory(final ByteArray holder) { logger.trace("address = {} removing excess duplicateDetection {}", address, describeID(id.bytes)); } - cache.remove(id); + // The same id may be held in a newer slot too: then it stays + cache.remove(id, slot); } if (logger.isTraceEnabled()) { @@ -217,6 +218,8 @@ private synchronized void addToCacheInMemory(final ByteArray holder) { ids.add(holder); } + cache.put(holder, slot); + if (pos++ == cacheSize - 1) { pos = 0; } diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PersistentDuplicateIDCache.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PersistentDuplicateIDCache.java index 4d590fc62ff..891397a705b 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PersistentDuplicateIDCache.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PersistentDuplicateIDCache.java @@ -282,7 +282,7 @@ private synchronized void addToCacheInMemory(final ByteArray holder, final long logger.trace("address = {} adding {}", address, describeID(holder.bytes, recordID)); } - cache.put(holder, cachedBoxedInts.apply(pos)); + final Integer slot = cachedBoxedInts.apply(pos); ObjLongPair id; @@ -296,7 +296,8 @@ private synchronized void addToCacheInMemory(final ByteArray holder, final long logger.trace("address = {} removing excess duplicateDetection {}", address, describeID(id.getA().bytes, id.getB())); } - cache.remove(id.getA()); + // The same id may be held in a newer slot too: then it stays + cache.remove(id.getA(), slot); assert id.getB() != NIL; try { @@ -325,6 +326,8 @@ private synchronized void addToCacheInMemory(final ByteArray holder, final long } + cache.put(holder, slot); + if (pos++ == cacheSize - 1) { pos = 0; } diff --git a/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/DuplicateDetectionUnitTest.java b/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/DuplicateDetectionUnitTest.java index b04ffb52ca5..3b7cc21afa6 100644 --- a/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/DuplicateDetectionUnitTest.java +++ b/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/DuplicateDetectionUnitTest.java @@ -16,6 +16,7 @@ */ package org.apache.activemq.artemis.tests.unit.core.postoffice.impl; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -23,12 +24,14 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; +import java.util.function.IntFunction; import org.apache.activemq.artemis.api.config.ActiveMQDefaultConfiguration; import org.apache.activemq.artemis.api.core.Pair; import org.apache.activemq.artemis.api.core.SimpleString; import org.apache.activemq.artemis.core.config.Configuration; import org.apache.activemq.artemis.core.persistence.impl.journal.JournalStorageManager; +import org.apache.activemq.artemis.core.persistence.impl.nullpm.NullStorageManager; import org.apache.activemq.artemis.core.postoffice.DuplicateIDCache; import org.apache.activemq.artemis.core.postoffice.PostOffice; import org.apache.activemq.artemis.core.postoffice.impl.DuplicateIDCaches; @@ -47,6 +50,8 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; public class DuplicateDetectionUnitTest extends ActiveMQTestBase { @@ -153,4 +158,38 @@ public void testReloadDuplication() throws Exception { } } + + @Test + public void testIdHeldTwiceInMemory() throws Exception { + testIdHeldTwice(size -> DuplicateIDCaches.inMemory(SimpleString.of("address"), size)); + } + + @Test + public void testIdHeldTwicePersistent() throws Exception { + testIdHeldTwice(size -> DuplicateIDCaches.persistent(SimpleString.of("address"), size, new NullStorageManager())); + } + + /* + * An id the cache holds in two slots stays a duplicate until its newer slot is overwritten + */ + private void testIdHeldTwice(IntFunction caches) throws Exception { + final byte[] same = "same".getBytes(StandardCharsets.UTF_8); + + DuplicateIDCache cache = caches.apply(3); + cache.addToCache(same); + cache.addToCache(same); + cache.addToCache("other".getBytes(StandardCharsets.UTF_8)); + // Overwrites the older slot holding "same" + cache.addToCache("new".getBytes(StandardCharsets.UTF_8)); + assertTrue(cache.contains(same)); + // Overwrites the newer one + cache.addToCache("newer".getBytes(StandardCharsets.UTF_8)); + assertFalse(cache.contains(same)); + + // The same id overwriting itself + cache = caches.apply(1); + cache.addToCache(same); + cache.addToCache(same); + assertTrue(cache.contains(same)); + } }