QPID-6196: Add synchronization on CO registration to avoid creation of objects with duplicate names and ids

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1635184 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 144 insertions, 15 deletions

diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
index 6d2d7e33b5..d4a11290a9 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
@@ -1379,25 +1379,26 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
 
     private <C extends ConfiguredObject> void registerChild(final C child)
     {
-
-        Class categoryClass = child.getCategoryClass();
-        UUID childId = child.getId();
-        String name = child.getName();
-        if(_childrenById.get(categoryClass).containsKey(childId))
+        synchronized(_children)
         {
-            throw new DuplicateIdException(child);
-        }
-        if(getModel().getParentTypes(categoryClass).size() == 1)
-        {
-            if (_childrenByName.get(categoryClass).containsKey(name))
+            Class categoryClass = child.getCategoryClass();
+            UUID childId = child.getId();
+            String name = child.getName();
+            if(_childrenById.get(categoryClass).containsKey(childId))
             {
-                throw new DuplicateNameException(child);
+                throw new DuplicateIdException(child);
             }
-            _childrenByName.get(categoryClass).put(name, child);
+            if(getModel().getParentTypes(categoryClass).size() == 1)
+            {
+                if (_childrenByName.get(categoryClass).containsKey(name))
+                {
+                    throw new DuplicateNameException(child);
+                }
+                _childrenByName.get(categoryClass).put(name, child);
+            }
+            _children.get(categoryClass).add(child);
+            _childrenById.get(categoryClass).put(childId,child);
         }
-        _children.get(categoryClass).add(child);
-        _childrenById.get(categoryClass).put(childId,child);
-
     }
 
     public final void stop()
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AbstractConfiguredObjectTest.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AbstractConfiguredObjectTest.java
index 9e1669fb28..07dce0f8a1 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AbstractConfiguredObjectTest.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AbstractConfiguredObjectTest.java
@@ -24,6 +24,12 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.UUID;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.qpid.server.model.testmodel.TestChildCategory;
@@ -610,4 +616,121 @@ public class AbstractConfiguredObjectTest extends QpidTestCase
         }
     }
 
+    public void testCreateConcurrentlyChildrenWithTheSameName() throws Exception
+    {
+        final TestConfiguredObject parent = new TestConfiguredObject("parent");
+        parent.create();
+
+        short numberOfThreads = 300;
+        ExecutorService executor = Executors.newFixedThreadPool(numberOfThreads);
+
+        final CountDownLatch startLatch = new CountDownLatch(1);
+        final CountDownLatch endLatch = new CountDownLatch(numberOfThreads);
+        final AtomicInteger duplicateNameExceptionCounter = new AtomicInteger();
+        final AtomicInteger successCounter = new AtomicInteger();
+        try
+        {
+            for (int i = 0; i < numberOfThreads; i++)
+            {
+                executor.submit(new Runnable()
+                {
+                    @Override
+                    public void run()
+                    {
+                        TestConfiguredObject child = new TestConfiguredObject("child", parent, parent.getTaskExecutor());
+                        try
+                        {
+                            startLatch.await();
+                            child.create();
+                            successCounter.incrementAndGet();
+                        }
+                        catch(AbstractConfiguredObject.DuplicateNameException e)
+                        {
+                            duplicateNameExceptionCounter.incrementAndGet();
+                        }
+                        catch (InterruptedException e)
+                        {
+                            // ignore
+                        }
+                        finally
+                        {
+                            endLatch.countDown();
+                        }
+                    }
+                });
+            }
+            startLatch.countDown();
+            assertTrue("Waiting interval expired", endLatch.await(10, TimeUnit.SECONDS));
+        }
+        finally
+        {
+            executor.shutdownNow();
+        }
+
+        assertEquals("Unexpected number of children", 1, parent.getChildren(TestConfiguredObject.class).size());
+        assertEquals("Unexpected number of successful creations", 1, successCounter.get());
+        assertEquals("Unexpected number of DuplicateNameException", numberOfThreads - 1, duplicateNameExceptionCounter.get());
+    }
+
+    public void testCreateConcurrentlyChildrenWithTheSameId() throws Exception
+    {
+        final TestConfiguredObject parent = new TestConfiguredObject("parent");
+        parent.create();
+
+        short numberOfThreads = 300;
+        ExecutorService executor = Executors.newFixedThreadPool(numberOfThreads);
+
+        final CountDownLatch startLatch = new CountDownLatch(1);
+        final CountDownLatch endLatch = new CountDownLatch(numberOfThreads);
+        final AtomicInteger duplicateIdExceptionCounter = new AtomicInteger();
+        final AtomicInteger successCounter = new AtomicInteger();
+        final UUID id = UUID.randomUUID();
+        try
+        {
+            for (int i = 0; i < numberOfThreads; i++)
+            {
+                final int iteration = i;
+                executor.submit(new Runnable()
+                {
+                    @Override
+                    public void run()
+                    {
+                        Map<String, Object> attributes = new HashMap<>();
+                        attributes.put(ConfiguredObject.NAME, "child-" + iteration);
+                        attributes.put(ConfiguredObject.ID, id);
+                        TestConfiguredObject child = new TestConfiguredObject(parent, attributes);
+
+                        try
+                        {
+                            startLatch.await();
+                            child.create();
+                            successCounter.incrementAndGet();
+                        }
+                        catch(AbstractConfiguredObject.DuplicateIdException e)
+                        {
+                            duplicateIdExceptionCounter.incrementAndGet();
+                        }
+                        catch (InterruptedException e)
+                        {
+                            // ignore
+                        }
+                        finally
+                        {
+                            endLatch.countDown();
+                        }
+                    }
+                });
+            }
+            startLatch.countDown();
+            assertTrue("Waiting interval expired", endLatch.await(10, TimeUnit.SECONDS));
+        }
+        finally
+        {
+            executor.shutdownNow();
+        }
+
+        assertEquals("Unexpected number of children", 1, parent.getChildren(TestConfiguredObject.class).size());
+        assertEquals("Unexpected number of successful creations", 1, successCounter.get());
+        assertEquals("Unexpected number of DuplicateIdException", numberOfThreads - 1, duplicateIdExceptionCounter.get());
+    }
 }
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestConfiguredObject.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestConfiguredObject.java
index 5c04db0cd6..8f61e37ad4 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestConfiguredObject.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestConfiguredObject.java
@@ -66,6 +66,11 @@ public class TestConfiguredObject extends AbstractConfiguredObject
         this(createParents(parent), Collections.<String, Object>singletonMap(ConfiguredObject.NAME, name), taskExecutor, TestConfiguredObjectModel.INSTANCE);
     }
 
+    public TestConfiguredObject(ConfiguredObject<?> parent, Map<String, Object> attributes)
+    {
+        this(createParents(parent), attributes, parent.getTaskExecutor(), TestConfiguredObjectModel.INSTANCE);
+    }
+
     public TestConfiguredObject(Map parents, Map<String, Object> attributes, TaskExecutor taskExecutor, Model model)
     {
         super(parents, attributes, taskExecutor, model);