qpid-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kw...@apache.org
Subject svn commit: r1694776 - in /qpid/java/trunk/broker-core/src: main/java/org/apache/qpid/server/model/ test/java/org/apache/qpid/server/model/testmodels/hierarchy/
Date Sat, 08 Aug 2015 07:29:01 GMT
Author: kwall
Date: Sat Aug  8 07:29:00 2015
New Revision: 1694776

URL: http://svn.apache.org/r1694776
Log:
QPID-6671: [Java Broker] Closing a parent object must not clear child maps until all child
close futures complete

* Added supporting unit test

Work based on analysis by Alex Rudyy <orudyy@apache.org>

Added:
    qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestAbstractEngineImpl.java
      - copied, changed from r1694713, qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestPetrolEngineImpl.java
Modified:
    qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
    qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/AbstractConfiguredObjectTest.java
    qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestElecEngineImpl.java
    qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestEngine.java
    qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestHybridEngineImpl.java
    qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestPetrolEngineImpl.java

Modified: qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java?rev=1694776&r1=1694775&r2=1694776&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
(original)
+++ qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
Sat Aug  8 07:29:00 2015
@@ -61,7 +61,6 @@ import com.google.common.util.concurrent
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
-import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.SettableFuture;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -569,70 +568,64 @@ public abstract class AbstractConfigured
 
     protected final ListenableFuture<Void> closeChildren()
     {
-        final SettableFuture<Void> returnVal = SettableFuture.create();
-        final ChildCounter counter = new ChildCounter(new Runnable()
-        {
-            @Override
-            public void run()
-            {
-                returnVal.set(null);
-                LOGGER.debug("All children closed " + AbstractConfiguredObject.this.getClass().getSimpleName()
+ " : " + getName() );
-
-            }
-        });
-        counter.incrementCount();
-
+        final List<ListenableFuture<Void>> childCloseFutures = new ArrayList<>();
 
         applyToChildren(new Action<ConfiguredObject<?>>()
         {
             @Override
             public void performAction(final ConfiguredObject<?> child)
             {
-                counter.incrementCount();
-                ListenableFuture<Void> close = child.closeAsync();
-                Futures.addCallback(close, new FutureCallback<Void>()
+                ListenableFuture<Void> childCloseFuture = child.closeAsync();
+                Futures.addCallback(childCloseFuture, new FutureCallback<Void>()
                 {
                     @Override
                     public void onSuccess(final Void result)
                     {
-                        counter.decrementCount();
                     }
 
                     @Override
                     public void onFailure(final Throwable t)
                     {
-                        LOGGER.error("Exception occurred while closing "
-                                     + child.getClass().getSimpleName()
-                                     + " : '"
-                                     + child.getName()
-                                     + "'", t);
-                        // No need to decrement counter as setting the exception will complete
the future
-                        returnVal.setException(t);
+                        LOGGER.error("Exception occurred while closing {} : {}",
+                                     new Object[] {child.getClass().getSimpleName(),
+                                     child.getName(),
+                                     t});
                     }
-                }, MoreExecutors.directExecutor());
+                });
+                childCloseFutures.add(childCloseFuture);
             }
         });
 
-        counter.decrementCount();
-
-        for(Collection<ConfiguredObject<?>> childList : _children.values())
+        ListenableFuture<List<Void>> combinedFuture = Futures.allAsList(childCloseFutures);
+        return doAfter(combinedFuture, new Runnable()
         {
-            childList.clear();
-        }
+            @Override
+            public void run()
+            {
+                // TODO consider removing each child from the parent as each child close
completes, rather
+                // than awaiting the completion of the combined future.  This would make
it easy to give
+                // clearer debug that would highlight the children that have failed to closed.
+                for(Collection<ConfiguredObject<?>> childList : _children.values())
+                {
+                    childList.clear();
+                }
 
-        for(Map<UUID,ConfiguredObject<?>> childIdMap : _childrenById.values())
-        {
-            childIdMap.clear();
-        }
+                for(Map<UUID,ConfiguredObject<?>> childIdMap : _childrenById.values())
+                {
+                    childIdMap.clear();
+                }
 
-        for(Map<String,ConfiguredObject<?>> childNameMap : _childrenByName.values())
-        {
-            childNameMap.clear();
-        }
+                for(Map<String,ConfiguredObject<?>> childNameMap : _childrenByName.values())
+                {
+                    childNameMap.clear();
+                }
 
-        return returnVal;
+                LOGGER.debug("All children closed {} : {}", AbstractConfiguredObject.this.getClass().getSimpleName(),
getName());
+            }
+        });
     }
 
+
     @Override
     public void close()
     {

Modified: qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/AbstractConfiguredObjectTest.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/AbstractConfiguredObjectTest.java?rev=1694776&r1=1694775&r2=1694776&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/AbstractConfiguredObjectTest.java
(original)
+++ qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/AbstractConfiguredObjectTest.java
Sat Aug  8 07:29:00 2015
@@ -25,6 +25,7 @@ import java.util.Map;
 import java.util.UUID;
 
 import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.SettableFuture;
 
 import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.qpid.server.model.AbstractConfiguredObject;
@@ -185,9 +186,35 @@ public class AbstractConfiguredObjectTes
 
         assertTrue("Engine should have now attained state", attainedChild.isDone());
         assertEquals(engine, attainedChild.get());
+    }
+
+    public void testCloseAwaitsChildCloseCompletion()
+    {
+        final String carName = "myCar";
+        Map<String, Object> carAttributes = new HashMap<>();
+        carAttributes.put(ConfiguredObject.NAME, carName);
+        carAttributes.put(ConfiguredObject.TYPE, TestKitCarImpl.TEST_KITCAR_TYPE);
+
+        TestCar car = _model.getObjectFactory().create(TestCar.class, carAttributes);
+
+        SettableFuture<Void> engineCloseControllingFuture = SettableFuture.create();
+
+        String engineName = "myEngine";
+        Map<String, Object> engineAttributes = new HashMap<>();
+        engineAttributes.put(ConfiguredObject.NAME, engineName);
+        engineAttributes.put(ConfiguredObject.TYPE, TestElecEngineImpl.TEST_ELEC_ENGINE_TYPE);
+
+        TestEngine engine = (TestEngine) car.createChild(TestEngine.class, engineAttributes);
+        engine.setBeforeCloseFuture(engineCloseControllingFuture);
 
+        ListenableFuture carListenableFuture = car.closeAsync();
+        assertFalse("car close future has completed before engine closed", carListenableFuture.isDone());
+        assertSame("engine deregistered from car too early", engine, car.getChildById(TestEngine.class,
engine.getId()));
 
+        engineCloseControllingFuture.set(null);
 
+        assertTrue("car close future has not completed", carListenableFuture.isDone());
+        assertNull("engine not deregistered", car.getChildById(TestEngine.class, engine.getId()));
     }
 
     public void testDefaultContextVariableWhichRefersToAncestor()

Copied: qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestAbstractEngineImpl.java
(from r1694713, qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestPetrolEngineImpl.java)
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestAbstractEngineImpl.java?p2=qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestAbstractEngineImpl.java&p1=qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestPetrolEngineImpl.java&r1=1694713&r2=1694776&rev=1694776&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestPetrolEngineImpl.java
(original)
+++ qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestAbstractEngineImpl.java
Sat Aug  8 07:29:00 2015
@@ -18,23 +18,36 @@
  * under the License.
  *
  */
+
 package org.apache.qpid.server.model.testmodels.hierarchy;
 
 import java.util.Map;
 
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+
 import org.apache.qpid.server.model.AbstractConfiguredObject;
-import org.apache.qpid.server.model.ManagedObject;
-import org.apache.qpid.server.model.ManagedObjectFactoryConstructor;
+import org.apache.qpid.server.model.ConfiguredObject;
 
-@ManagedObject( category = false, type = TestPetrolEngineImpl.TEST_PETROL_ENGINE_TYPE)
-public class TestPetrolEngineImpl
-        extends AbstractConfiguredObject<TestPetrolEngineImpl> implements TestPetrolEngine<TestPetrolEngineImpl>
+public class TestAbstractEngineImpl<X extends TestAbstractEngineImpl<X>> extends
AbstractConfiguredObject<X> implements TestEngine<X>
 {
-    public static final String TEST_PETROL_ENGINE_TYPE = "PETROL";
+    private ListenableFuture<Void> _beforeCloseFuture = Futures.immediateFuture(null);
+
+    public TestAbstractEngineImpl(final Map<Class<? extends ConfiguredObject>, ConfiguredObject<?>>
parents,
+                                  final Map<String, Object> attributes)
+    {
+        super(parents, attributes);
+    }
+
+    @Override
+    public void setBeforeCloseFuture(final ListenableFuture listenableFuture)
+    {
+        _beforeCloseFuture = listenableFuture;
+    }
 
-    @ManagedObjectFactoryConstructor
-    public TestPetrolEngineImpl(final Map<String, Object> attributes, TestCar<?>
parent)
+    @Override
+    protected ListenableFuture<Void> beforeClose()
     {
-        super(parentsMap(parent), attributes);
+        return _beforeCloseFuture;
     }
 }

Modified: qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestElecEngineImpl.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestElecEngineImpl.java?rev=1694776&r1=1694775&r2=1694776&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestElecEngineImpl.java
(original)
+++ qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestElecEngineImpl.java
Sat Aug  8 07:29:00 2015
@@ -25,13 +25,16 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.Map;
 
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+
 import org.apache.qpid.server.model.AbstractConfiguredObject;
 import org.apache.qpid.server.model.ManagedObject;
 import org.apache.qpid.server.model.ManagedObjectFactoryConstructor;
 
 @ManagedObject( category = false, type = TestElecEngineImpl.TEST_ELEC_ENGINE_TYPE)
 public class TestElecEngineImpl
-        extends AbstractConfiguredObject<TestElecEngineImpl> implements TestElecEngine<TestElecEngineImpl>
+        extends TestAbstractEngineImpl<TestElecEngineImpl> implements TestElecEngine<TestElecEngineImpl>
 {
     public static final String TEST_ELEC_ENGINE_TYPE = "ELEC";
 

Modified: qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestEngine.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestEngine.java?rev=1694776&r1=1694775&r2=1694776&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestEngine.java
(original)
+++ qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestEngine.java
Sat Aug  8 07:29:00 2015
@@ -20,10 +20,14 @@
  */
 package org.apache.qpid.server.model.testmodels.hierarchy;
 
+import com.google.common.util.concurrent.ListenableFuture;
+
 import org.apache.qpid.server.model.ConfiguredObject;
 import org.apache.qpid.server.model.ManagedObject;
 
 @ManagedObject(category = true)
 public interface TestEngine<X extends TestEngine<X>> extends ConfiguredObject<X>
 {
+    /** Injectable close future, used to control when/how close occurs during test */
+    void setBeforeCloseFuture(ListenableFuture listenableFuture);
 }

Modified: qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestHybridEngineImpl.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestHybridEngineImpl.java?rev=1694776&r1=1694775&r2=1694776&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestHybridEngineImpl.java
(original)
+++ qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestHybridEngineImpl.java
Sat Aug  8 07:29:00 2015
@@ -21,12 +21,16 @@ package org.apache.qpid.server.model.tes
 
 import java.util.Map;
 
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+
+import org.apache.qpid.configuration.Validator;
 import org.apache.qpid.server.model.AbstractConfiguredObject;
 import org.apache.qpid.server.model.ManagedObject;
 import org.apache.qpid.server.model.ManagedObjectFactoryConstructor;
 
 @ManagedObject( category = false, type = TestHybridEngineImpl.TEST_HYBRID_ENGINE_TYPE)
-public class TestHybridEngineImpl extends AbstractConfiguredObject<TestHybridEngineImpl>
implements TestHybridEngine<TestHybridEngineImpl>
+public class TestHybridEngineImpl extends TestAbstractEngineImpl<TestHybridEngineImpl>
implements TestHybridEngine<TestHybridEngineImpl>
 {
     public static final String TEST_HYBRID_ENGINE_TYPE = "HYBRID";
 

Modified: qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestPetrolEngineImpl.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestPetrolEngineImpl.java?rev=1694776&r1=1694775&r2=1694776&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestPetrolEngineImpl.java
(original)
+++ qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestPetrolEngineImpl.java
Sat Aug  8 07:29:00 2015
@@ -22,13 +22,16 @@ package org.apache.qpid.server.model.tes
 
 import java.util.Map;
 
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+
 import org.apache.qpid.server.model.AbstractConfiguredObject;
 import org.apache.qpid.server.model.ManagedObject;
 import org.apache.qpid.server.model.ManagedObjectFactoryConstructor;
 
 @ManagedObject( category = false, type = TestPetrolEngineImpl.TEST_PETROL_ENGINE_TYPE)
 public class TestPetrolEngineImpl
-        extends AbstractConfiguredObject<TestPetrolEngineImpl> implements TestPetrolEngine<TestPetrolEngineImpl>
+        extends TestAbstractEngineImpl<TestPetrolEngineImpl> implements TestPetrolEngine<TestPetrolEngineImpl>
 {
     public static final String TEST_PETROL_ENGINE_TYPE = "PETROL";
 
@@ -37,4 +40,5 @@ public class TestPetrolEngineImpl
     {
         super(parentsMap(parent), attributes);
     }
+
 }



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


Mime
View raw message