QPID-6126: Normalize exception handling on open and prevent ERRORED children from being validated or opened

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

3 files changed, 197 insertions, 25 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 77810a4d69..5fa536dafd 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
@@ -529,31 +529,32 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
     {
     }
 
-    protected final void handleExceptionOnOpen(RuntimeException e, AbstractConfiguredObject<?> configuredObject)
+    protected final void handleExceptionOnOpen(RuntimeException e)
     {
         if (e instanceof ServerScopedRuntimeException)
         {
             throw e;
         }
 
-        LOGGER.error("Exception occurred on open for " + configuredObject.getName(), e);
+        LOGGER.error("Exception occurred on open for " + getName(), e);
 
         try
         {
-            configuredObject.onExceptionInOpen(e);
+            onExceptionInOpen(e);
         }
         catch (RuntimeException re)
         {
-            LOGGER.error("Unexpected exception while handling exception on open for " + configuredObject.getName(), e);
+            LOGGER.error("Unexpected exception while handling exception on open for " + getName(), e);
         }
 
-        if (!configuredObject._openComplete)
+        if (!_openComplete)
         {
-            configuredObject._openFailed = true;
-            configuredObject._dynamicState.compareAndSet(DynamicState.OPENED, DynamicState.UNINIT);
+            _openFailed = true;
+            _dynamicState.compareAndSet(DynamicState.OPENED, DynamicState.UNINIT);
         }
-        configuredObject.closeChildren();
-        configuredObject.setState(State.ERRORED);
+
+        //TODO: children of ERRORED CO will continue to remain in ACTIVE state
+        setState(State.ERRORED);
     }
 
     /**
@@ -604,7 +605,7 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
                 @Override
                 public void performAction(final ConfiguredObject<?> child)
                 {
-                    if (child instanceof AbstractConfiguredObject)
+                    if (child.getState() != State.ERRORED && child instanceof AbstractConfiguredObject)
                     {
                         AbstractConfiguredObject configuredObject = (AbstractConfiguredObject) child;
                         try
@@ -631,7 +632,7 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
                 @Override
                 public void performAction(final ConfiguredObject<?> child)
                 {
-                    if (child instanceof AbstractConfiguredObject)
+                    if (child.getState() != State.ERRORED && child instanceof AbstractConfiguredObject)
                     {
                         AbstractConfiguredObject configuredObject = (AbstractConfiguredObject) child;
                         try
@@ -1878,18 +1879,18 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
         void handleException(RuntimeException exception, AbstractConfiguredObject<?> source);
     }
 
-    private class OpenExceptionHandler implements AbstractConfiguredObjectExceptionHandler
+    private static class OpenExceptionHandler implements AbstractConfiguredObjectExceptionHandler
     {
         @Override
         public void handleException(RuntimeException exception, AbstractConfiguredObject<?> source)
         {
-            handleExceptionOnOpen(exception, source);
+            source.handleExceptionOnOpen(exception);
         }
     }
 
-    private class CreateExceptionHandler implements AbstractConfiguredObjectExceptionHandler
+    private static class CreateExceptionHandler implements AbstractConfiguredObjectExceptionHandler
     {
-        private boolean _unregister;
+        private final boolean _unregister;
 
         private CreateExceptionHandler()
         {
@@ -1898,7 +1899,7 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
 
         private CreateExceptionHandler(boolean unregister)
         {
-            this._unregister = unregister;
+            _unregister = unregister;
         }
 
         @Override
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 4c6f0710c4..a1a363d5fe 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
@@ -388,4 +388,60 @@ public class AbstractConfiguredObjectTest extends TestCase
         assertFalse("Unexpected opened", object.isOpened());
         assertEquals("Unexpected state", State.DELETED, object.getState());
     }
+
+    public void testUnresolvedChildInERROREDStateIsNotValidatedOrOpenedOrAttainedDesiredStateOnParentOpen() throws Exception
+    {
+        TestConfiguredObject parent = new TestConfiguredObject("parent");
+        TestConfiguredObject child1 = new TestConfiguredObject("child1", parent, parent.getTaskExecutor());
+        child1.registerWithParents();
+        TestConfiguredObject child2 = new TestConfiguredObject("child2", parent, parent.getTaskExecutor());
+        child2.registerWithParents();
+
+        child1.setThrowExceptionOnPostResolve(true);
+
+        parent.open();
+
+        assertTrue("Parent should be resolved", parent.isResolved());
+        assertTrue("Parent should be validated", parent.isValidated());
+        assertTrue("Parent should be opened", parent.isOpened());
+        assertEquals("Unexpected parent state", State.ACTIVE, parent.getState());
+
+        assertTrue("Child2 should be resolved", child2.isResolved());
+        assertTrue("Child2 should be validated", child2.isValidated());
+        assertTrue("Child2 should be opened", child2.isOpened());
+        assertEquals("Unexpected child2 state", State.ACTIVE, child2.getState());
+
+        assertFalse("Child2 should not be resolved", child1.isResolved());
+        assertFalse("Child1 should not be validated", child1.isValidated());
+        assertFalse("Child1 should not be opened", child1.isOpened());
+        assertEquals("Unexpected child1 state", State.ERRORED, child1.getState());
+    }
+
+    public void testUnvalidatedChildInERROREDStateIsNotOpenedOrAttainedDesiredStateOnParentOpen() throws Exception
+    {
+        TestConfiguredObject parent = new TestConfiguredObject("parent");
+        TestConfiguredObject child1 = new TestConfiguredObject("child1", parent, parent.getTaskExecutor());
+        child1.registerWithParents();
+        TestConfiguredObject child2 = new TestConfiguredObject("child2", parent, parent.getTaskExecutor());
+        child2.registerWithParents();
+
+        child1.setThrowExceptionOnValidate(true);
+
+        parent.open();
+
+        assertTrue("Parent should be resolved", parent.isResolved());
+        assertTrue("Parent should be validated", parent.isValidated());
+        assertTrue("Parent should be opened", parent.isOpened());
+        assertEquals("Unexpected parent state", State.ACTIVE, parent.getState());
+
+        assertTrue("Child2 should be resolved", child2.isResolved());
+        assertTrue("Child2 should be validated", child2.isValidated());
+        assertTrue("Child2 should be opened", child2.isOpened());
+        assertEquals("Unexpected child2 state", State.ACTIVE, child2.getState());
+
+        assertTrue("Child1 should be resolved", child1.isResolved());
+        assertFalse("Child1 should not be validated", child1.isValidated());
+        assertFalse("Child1 should not be opened", child1.isOpened());
+        assertEquals("Unexpected child1 state", State.ERRORED, child1.getState());
+    }
 }
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 0ba5673f91..90d9955c1f 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
@@ -2,6 +2,8 @@ package org.apache.qpid.server.model.testmodel;
 
 import static org.mockito.Mockito.mock;
 
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Map;
 
@@ -10,35 +12,40 @@ import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor;
 import org.apache.qpid.server.configuration.updater.TaskExecutor;
 import org.apache.qpid.server.model.AbstractConfiguredObject;
 import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.ConfiguredObjectFactory;
+import org.apache.qpid.server.model.ConfiguredObjectFactoryImpl;
+import org.apache.qpid.server.model.ConfiguredObjectTypeRegistry;
 import org.apache.qpid.server.model.ManagedObject;
 import org.apache.qpid.server.model.Model;
 import org.apache.qpid.server.model.State;
 import org.apache.qpid.server.model.StateTransition;
+import org.apache.qpid.server.plugin.ConfiguredObjectRegistration;
 
 @ManagedObject
 public class TestConfiguredObject extends AbstractConfiguredObject
 {
-    private boolean _throwExceptionOnOpen;
     private boolean _opened;
+    private boolean _validated;
+    private boolean _resolved;
+    private boolean _throwExceptionOnOpen;
     private boolean _throwExceptionOnValidationOnCreate;
     private boolean _throwExceptionOnPostResolve;
     private boolean _throwExceptionOnCreate;
+    private boolean _throwExceptionOnValidate;
 
-    public TestConfiguredObject(String name)
+    public final static Map<Class<? extends ConfiguredObject>, ConfiguredObject<?>> createParents(ConfiguredObject<?> parent)
     {
-        this(createParents(), Collections.<String, Object>singletonMap(ConfiguredObject.NAME, name), createTaskExecutor(), new TestModel(null));
+        return Collections.<Class<? extends ConfiguredObject>, ConfiguredObject<?>>singletonMap(parent.getCategoryClass(), parent);
     }
 
-    public final static Map<Class<? extends ConfiguredObject>, ConfiguredObject<?>> createParents()
+    public TestConfiguredObject(String name)
     {
-        return Collections.<Class<? extends ConfiguredObject>, ConfiguredObject<?>>singletonMap(null, mock(ConfiguredObject.class));
+        this(name, mock(ConfiguredObject.class), CurrentThreadTaskExecutor.newStartedInstance());
     }
 
-    public final static TaskExecutor createTaskExecutor()
+    public TestConfiguredObject(String name, ConfiguredObject<?> parent, TaskExecutor taskExecutor)
     {
-        TaskExecutor taskExecutor = new CurrentThreadTaskExecutor();
-        taskExecutor.start();
-        return taskExecutor;
+        this(createParents(parent), Collections.<String, Object>singletonMap(ConfiguredObject.NAME, name), taskExecutor, TestConfiguredObjectModel.INSTANCE);
     }
 
     public TestConfiguredObject(Map parents, Map<String, Object> attributes, TaskExecutor taskExecutor, Model model)
@@ -54,6 +61,7 @@ public class TestConfiguredObject extends AbstractConfiguredObject
         {
             throw new IllegalConfigurationException("Cannot resolve");
         }
+        _resolved = true;
     }
 
     @Override
@@ -84,6 +92,16 @@ public class TestConfiguredObject extends AbstractConfiguredObject
         }
     }
 
+    @Override
+    public void onValidate()
+    {
+        if (_throwExceptionOnValidate)
+        {
+            throw new IllegalConfigurationException("Cannot validate");
+        }
+        _validated = true;
+    }
+
     @StateTransition( currentState = {State.ERRORED, State.UNINITIALIZED}, desiredState = State.ACTIVE )
     protected void activate()
     {
@@ -120,4 +138,101 @@ public class TestConfiguredObject extends AbstractConfiguredObject
     {
         _throwExceptionOnCreate = throwExceptionOnCreate;
     }
+
+    public void setThrowExceptionOnValidate(boolean throwException)
+    {
+        _throwExceptionOnValidate= throwException;
+    }
+
+    public boolean isValidated()
+    {
+        return _validated;
+    }
+
+    public boolean isResolved()
+    {
+        return _resolved;
+    }
+
+    public static class TestConfiguredObjectModel extends  Model
+    {
+
+        private Collection<Class<? extends ConfiguredObject>> CATEGORIES = Collections.<Class<? extends ConfiguredObject>>singleton(TestConfiguredObject.class);
+        private ConfiguredObjectFactoryImpl _configuredObjectFactory;
+
+        private static TestConfiguredObjectModel INSTANCE = new TestConfiguredObjectModel();
+        private ConfiguredObjectTypeRegistry _configuredObjectTypeRegistry;
+
+        private TestConfiguredObjectModel()
+        {
+            _configuredObjectFactory = new ConfiguredObjectFactoryImpl(this);
+            ConfiguredObjectRegistration configuredObjectRegistration = new ConfiguredObjectRegistration()
+            {
+                @Override
+                public Collection<Class<? extends ConfiguredObject>> getConfiguredObjectClasses()
+                {
+                    return CATEGORIES;
+                }
+
+                @Override
+                public String getType()
+                {
+                    return TestConfiguredObjectModel.class.getSimpleName();
+                }
+            };
+            _configuredObjectTypeRegistry = new ConfiguredObjectTypeRegistry(Arrays.asList(configuredObjectRegistration), CATEGORIES);
+        }
+
+        @Override
+        public Collection<Class<? extends ConfiguredObject>> getSupportedCategories()
+        {
+            return CATEGORIES;
+        }
+
+        @Override
+        public Collection<Class<? extends ConfiguredObject>> getChildTypes(Class<? extends ConfiguredObject> parent)
+        {
+            return TestConfiguredObject.class.isAssignableFrom(parent)
+                    ? CATEGORIES
+                    : Collections.<Class<? extends ConfiguredObject>>emptySet();
+        }
+
+        @Override
+        public Class<? extends ConfiguredObject> getRootCategory()
+        {
+            return TestConfiguredObject.class;
+        }
+
+        @Override
+        public Collection<Class<? extends ConfiguredObject>> getParentTypes(final Class<? extends ConfiguredObject> child)
+        {
+            return TestConfiguredObject.class.isAssignableFrom(child)
+                    ? CATEGORIES
+                    : Collections.<Class<? extends ConfiguredObject>>emptySet();
+        }
+
+        @Override
+        public int getMajorVersion()
+        {
+            return 99;
+        }
+
+        @Override
+        public int getMinorVersion()
+        {
+            return 99;
+        }
+
+        @Override
+        public ConfiguredObjectFactory getObjectFactory()
+        {
+            return _configuredObjectFactory;
+        }
+
+        @Override
+        public ConfiguredObjectTypeRegistry getTypeRegistry()
+        {
+            return _configuredObjectTypeRegistry;
+        }
+    }
 }