QPID-4390: Improve the thread safety of java broker management operations

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

3 files changed, 106 insertions, 24 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/updater/ChangeAttributesTask.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/updater/ChangeAttributesTask.java
new file mode 100644
index 0000000000..4ba4057fee
--- /dev/null
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/updater/ChangeAttributesTask.java
@@ -0,0 +1,62 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.qpid.server.configuration.updater;
+
+import java.util.Map;
+import java.util.concurrent.Callable;
+
+import org.apache.qpid.server.model.ConfiguredObject;
+
+public class ChangeAttributesTask implements Callable<Void>
+{
+    private final Map<String, Object> _attributes;
+    private final ConfiguredObject _object;
+
+    public ChangeAttributesTask(ConfiguredObject target, Map<String, Object> attributes)
+    {
+        super();
+        _object = target;
+        _attributes = attributes;
+    }
+
+    @Override
+    public Void call() throws Exception
+    {
+        _object.setAttributes(_attributes);
+        return null;
+    }
+
+    public Map<String, Object> getAttributes()
+    {
+        return _attributes;
+    }
+
+    public ConfiguredObject getObject()
+    {
+        return _object;
+    }
+
+    @Override
+    public String toString()
+    {
+        return "ChangeAttributesTask [object=" + _object + ", attributes=" + _attributes + "]";
+    }
+}
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AbstractAdapter.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AbstractAdapter.java
index 6b6cce3ffa..ce8e06bf4f 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AbstractAdapter.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AbstractAdapter.java
@@ -27,12 +27,12 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.UUID;
-import java.util.concurrent.Callable;
 
 import org.apache.qpid.server.model.ConfigurationChangeListener;
 import org.apache.qpid.server.model.ConfiguredObject;
 import org.apache.qpid.server.model.IllegalStateTransitionException;
 import org.apache.qpid.server.model.State;
+import org.apache.qpid.server.configuration.updater.ChangeAttributesTask;
 import org.apache.qpid.server.configuration.updater.ChangeStateTask;
 import org.apache.qpid.server.configuration.updater.CreateChildTask;
 import org.apache.qpid.server.configuration.updater.SetAttributeTask;
@@ -56,7 +56,14 @@ abstract class AbstractAdapter implements ConfiguredObject
         _id = id;
         if (attributes != null)
         {
-            _attributes.putAll(attributes);
+            Collection<String> names = getAttributeNames();
+            for (String name : names)
+            {
+                if (attributes.containsKey(name))
+                {
+                    _attributes.put(name, attributes.get(name));
+                }
+            }
         }
         if (defaults != null)
         {
@@ -88,13 +95,17 @@ abstract class AbstractAdapter implements ConfiguredObject
             if (setState(currentState, desiredState))
             {
                 notifyStateChanged(currentState, desiredState);
+                return desiredState;
+            }
+            else
+            {
+                return getActualState();
             }
         }
         else
         {
-            _taskExecutor.submitAndWait(new ChangeStateTask(this, currentState, desiredState));
+            return (State)_taskExecutor.submitAndWait(new ChangeStateTask(this, currentState, desiredState));
         }
-        return getActualState();
     }
 
     /**
@@ -218,13 +229,17 @@ abstract class AbstractAdapter implements ConfiguredObject
             if (changeAttribute(name, expected, desired))
             {
                 attributeSet(name, expected, desired);
+                return desired;
+            }
+            else
+            {
+                return getAttribute(name);
             }
         }
         else
         {
-            _taskExecutor.submitAndWait(new SetAttributeTask(this, name, expected, desired));
+            return _taskExecutor.submitAndWait(new SetAttributeTask(this, name, expected, desired));
         }
-        return getAttribute(name);
     }
 
     protected boolean changeAttribute(final String name, final Object expected, final Object desired)
@@ -322,30 +337,23 @@ abstract class AbstractAdapter implements ConfiguredObject
         }
         else
         {
-            getTaskExecutor().submitAndWait(new Callable<Void>()
-            {
-
-                @Override
-                public Void call() throws Exception
-                {
-                    AbstractAdapter.this.setAttributes(attributes);
-                    return null;
-                }
-            });
+            getTaskExecutor().submitAndWait(new ChangeAttributesTask(this, attributes));
         }
-
     }
 
     protected void changeAttributes(final Map<String, Object> attributes)
     {
-        for (Map.Entry<String, Object> attributeEntry : attributes.entrySet())
+        Collection<String> names = getAttributeNames();
+        for (String name : names)
         {
-            String name = attributeEntry.getKey();
-            Object desired = attributeEntry.getValue();
-            Object expected = getAttribute(name);
-            if (changeAttribute(name, expected, desired))
+            if (attributes.containsKey(name))
             {
-                attributeSet(name, expected, desired);
+                Object desired = attributes.get(name);
+                Object expected = getAttribute(name);
+                if (changeAttribute(name, expected, desired))
+                {
+                    attributeSet(name, expected, desired);
+                }
             }
         }
     }
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java
index 1323655f3c..a6c5a74765 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java
@@ -74,12 +74,24 @@ public abstract class AuthenticationProviderAdapter<T extends AuthenticationMana
 
     private AuthenticationProviderAdapter(UUID id, Broker broker, final T authManager, Map<String, Object> attributes, Collection<String> attributeNames)
     {
-        super(id, null, attributes, broker.getTaskExecutor());
+        super(id, null, null, broker.getTaskExecutor());
         _authManager = authManager;
         _broker = broker;
         _supportedAttributes = createSupportedAttributes(attributeNames);
         _factories = getAuthenticationManagerFactories();
         addParent(Broker.class, broker);
+
+        // set attributes now after all attribute names are known
+        if (attributes != null)
+        {
+            for (String name : _supportedAttributes)
+            {
+                if (attributes.containsKey(name))
+                {
+                    changeAttribute(name, null, attributes.get(name));
+                }
+            }
+        }
     }
 
     T getAuthManager()