You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by kw...@apache.org on 2015/08/08 09:29:01 UTC
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/
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 <or...@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