FELIX-4585 fix factory component bugs revealed by enable fix
git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1615284 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java b/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java
index 99de1cb..57414ae 100644
--- a/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java
+++ b/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java
@@ -84,7 +84,8 @@
protected final ComponentContainer<S> m_container;
- private final boolean m_factoryInstance;
+ //true for normal spec factory instances. False for "persistent" factory instances and obsolete use of factory component with factory configurations.
+ protected final boolean m_factoryInstance;
// the ID of this component
private long m_componentId;
diff --git a/scr/src/main/java/org/apache/felix/scr/impl/manager/SingleComponentManager.java b/scr/src/main/java/org/apache/felix/scr/impl/manager/SingleComponentManager.java
index b01fa50..cc8c2d9 100644
--- a/scr/src/main/java/org/apache/felix/scr/impl/manager/SingleComponentManager.java
+++ b/scr/src/main/java/org/apache/felix/scr/impl/manager/SingleComponentManager.java
@@ -589,18 +589,23 @@
// FELIX-2368: cycle component immediately, reconfigure() is
// called through ConfigurationListener API which itself is
// called asynchronously by the Configuration Admin Service
- releaseActivationWriteeLock( "reconfigure.modified.1" );;
- deactivateInternal( reason, false, false );
- obtainActivationWriteLock( "reconfigure.deactivate.activate" );
- try
+ releaseActivationWriteeLock( "reconfigure.modified.1" );
+ //we have already determined that modify cannot be called. Therefore factory instances must be disposed.
+ boolean dispose = m_factoryInstance;
+ deactivateInternal( reason, dispose, dispose );
+ if ( !dispose )
{
- updateTargets( getProperties() );
+ obtainActivationWriteLock("reconfigure.deactivate.activate");
+ try
+ {
+ updateTargets(getProperties());
+ }
+ finally
+ {
+ releaseActivationWriteeLock("reconfigure.deactivate.activate");
+ }
+ activateInternal(getTrackingCount().get());
}
- finally
- {
- releaseActivationWriteeLock( "reconfigure.deactivate.activate" );
- }
- activateInternal( getTrackingCount().get() );
}
}
finally
diff --git a/scr/src/test/java/org/apache/felix/scr/integration/ComponentFactoryTest.java b/scr/src/test/java/org/apache/felix/scr/integration/ComponentFactoryTest.java
index fa9abbb..9f9684b 100644
--- a/scr/src/test/java/org/apache/felix/scr/integration/ComponentFactoryTest.java
+++ b/scr/src/test/java/org/apache/felix/scr/integration/ComponentFactoryTest.java
@@ -19,6 +19,7 @@
package org.apache.felix.scr.integration;
+import java.lang.reflect.InvocationTargetException;
import java.util.Hashtable;
import junit.framework.TestCase;
@@ -137,6 +138,44 @@
final String componentname = "factory.component.configuration";
final String componentfactory = "factory.component.factory.configuration";
+ testConfiguredFactory(componentname, componentfactory, false, false);
+
+ }
+
+ @Test
+ public void test_component_factory_optional_configuration() throws Exception
+ {
+ final String componentname = "factory.component.configuration.optional";
+ final String componentfactory = "factory.component.factory.configuration.optional";
+
+ testConfiguredFactory(componentname, componentfactory, true, false);
+
+ }
+
+ @Test
+ public void test_component_factory_optional_configuration_13() throws Exception
+ {
+ final String componentname = "factory.component.configuration.optional.13";
+ final String componentfactory = "factory.component.factory.configuration.optional.13";
+
+ testConfiguredFactory(componentname, componentfactory, true, true);
+ }
+
+ @Test
+ public void test_component_factory_optional_configuration_nomodify() throws Exception
+ {
+ final String componentname = "factory.component.configuration.optional.nomodify";
+ final String componentfactory = "factory.component.factory.configuration.optional.nomodify";
+
+ testConfiguredFactory(componentname, componentfactory, true, false);
+
+ }
+
+
+ private ComponentInstance testConfiguredFactory(final String componentname,
+ final String componentfactory, boolean optional, boolean expectComponent) throws InvocationTargetException,
+ InterruptedException, InvalidSyntaxException
+ {
// ensure there is no configuration for the component
deleteConfig( componentname );
delay();
@@ -147,7 +186,7 @@
// At this point, since we don't have created the configuration, then the ComponentFactory
// should not be available.
- checkNoFactory(componentfactory);
+ checkFactory(componentfactory, optional);
// supply configuration now and ensure active
configure( componentname );
@@ -172,13 +211,26 @@
//deactivates component.
deleteConfig( componentname );
delay();
- checkNoFactory(componentfactory);
- TestCase.assertNull( instance.getInstance() );
- TestCase.assertNull( SimpleComponent.INSTANCE );
+ checkFactory(componentfactory, optional);
- // with removal of the factory, the created instance should also be removed
- checkConfigurationCount(componentname, 0, ComponentConfigurationDTO.ACTIVE);
+ if (expectComponent)
+ {
+ TestCase.assertNotNull( instance.getInstance() );
+ TestCase.assertNotNull( SimpleComponent.INSTANCE );
+
+ // with removal of the factory, the created instance should also be removed
+ checkConfigurationCount(componentname, 1, ComponentConfigurationDTO.ACTIVE);
+ }
+ else
+ {
+ TestCase.assertNull( instance.getInstance() );
+ TestCase.assertNull( SimpleComponent.INSTANCE );
+
+ // with removal of the factory, the created instance should also be removed
+ checkConfigurationCount(componentname, 0, ComponentConfigurationDTO.ACTIVE);
+ }
+ return instance;
}
diff --git a/scr/src/test/java/org/apache/felix/scr/integration/ComponentTestBase.java b/scr/src/test/java/org/apache/felix/scr/integration/ComponentTestBase.java
index e5c054c..af5f978 100644
--- a/scr/src/test/java/org/apache/felix/scr/integration/ComponentTestBase.java
+++ b/scr/src/test/java/org/apache/felix/scr/integration/ComponentTestBase.java
@@ -551,12 +551,21 @@
}
- protected void checkNoFactory(final String componentfactory)
+ protected void checkFactory(final String componentfactory, boolean expectFactoryPresent)
throws InvalidSyntaxException
{
ServiceReference[] refs = bundleContext.getServiceReferences( ComponentFactory.class.getName(), "("
+ ComponentConstants.COMPONENT_FACTORY + "=" + componentfactory + ")" );
- TestCase.assertNull( refs );
+ if ( expectFactoryPresent )
+ {
+ TestCase.assertNotNull( refs );
+ TestCase.assertEquals(1, refs.length);
+
+ }
+ else
+ {
+ TestCase.assertNull( refs );
+ }
}
protected ComponentInstance createFactoryComponentInstance(final String componentfactory)
diff --git a/scr/src/test/java/org/apache/felix/scr/integration/PersistentComponentFactoryTest.java b/scr/src/test/java/org/apache/felix/scr/integration/PersistentComponentFactoryTest.java
index 7c41a50..96afa51 100644
--- a/scr/src/test/java/org/apache/felix/scr/integration/PersistentComponentFactoryTest.java
+++ b/scr/src/test/java/org/apache/felix/scr/integration/PersistentComponentFactoryTest.java
@@ -19,6 +19,7 @@
package org.apache.felix.scr.integration;
+import java.lang.reflect.InvocationTargetException;
import java.util.Hashtable;
import junit.framework.TestCase;
@@ -126,13 +127,50 @@
checkConfigurationCount(componentname, 1, ComponentConfigurationDTO.SATISFIED);
}
-
@Test
public void test_component_factory_require_configuration() throws Exception
{
final String componentname = "factory.component.configuration";
final String componentfactory = "factory.component.factory.configuration";
+ testConfiguredFactory(componentname, componentfactory, false, false);
+
+ }
+
+ @Test
+ public void test_component_factory_optional_configuration() throws Exception
+ {
+ final String componentname = "factory.component.configuration.optional";
+ final String componentfactory = "factory.component.factory.configuration.optional";
+
+ testConfiguredFactory(componentname, componentfactory, true, true);
+
+ }
+
+ @Test
+ public void test_component_factory_optional_configuration_13() throws Exception
+ {
+ final String componentname = "factory.component.configuration.optional.13";
+ final String componentfactory = "factory.component.factory.configuration.optional.13";
+
+ testConfiguredFactory(componentname, componentfactory, true, true);
+ }
+
+ @Test
+ public void test_component_factory_optional_configuration_nomodify() throws Exception
+ {
+ final String componentname = "factory.component.configuration.optional.nomodify";
+ final String componentfactory = "factory.component.factory.configuration.optional.nomodify";
+
+ testConfiguredFactory(componentname, componentfactory, true, true);
+
+ }
+
+
+ private ComponentInstance testConfiguredFactory(final String componentname,
+ final String componentfactory, boolean optional, boolean expectComponent) throws InvocationTargetException,
+ InterruptedException, InvalidSyntaxException
+ {
// ensure there is no configuration for the component
deleteConfig( componentname );
delay();
@@ -143,7 +181,7 @@
// At this point, since we don't have created the configuration, then the ComponentFactory
// should not be available.
- checkNoFactory(componentfactory);
+ checkFactory(componentfactory, optional);
// supply configuration now and ensure active
configure( componentname );
@@ -163,19 +201,35 @@
checkConfigurationCount(componentname, 1, ComponentConfigurationDTO.ACTIVE);
- // delete config, ensure factory is not active anymore and component instance dropped (config required)
+ // delete config, ensure factory is not active anymore and component instance gone
+ //(configuration required >> dispose of instance. Also for pre-1.3 components, removing config unconditionally
+ //deactivates component.
deleteConfig( componentname );
delay();
- checkNoFactory(componentfactory);
- TestCase.assertNull( instance.getInstance() );
- TestCase.assertNull( SimpleComponent.INSTANCE );
+ checkFactory(componentfactory, optional);
- // with removal of the factory, the created instance should also be removed
- checkConfigurationCount(componentname, 0, ComponentConfigurationDTO.UNSATISFIED);
+ if (expectComponent)
+ {
+ TestCase.assertNotNull( instance.getInstance() );
+ TestCase.assertNotNull( SimpleComponent.INSTANCE );
+
+ // with removal of the factory, the created instance should also be removed
+ checkConfigurationCount(componentname, 1, ComponentConfigurationDTO.ACTIVE);
+ }
+ else
+ {
+ TestCase.assertNull( instance.getInstance() );
+ TestCase.assertNull( SimpleComponent.INSTANCE );
+
+ // with removal of the factory, the created instance should also be removed
+ checkConfigurationCount(componentname, 0, ComponentConfigurationDTO.ACTIVE);
+ }
+ return instance;
}
+
@Test
public void test_component_factory_reference() throws Exception
{
diff --git a/scr/src/test/resources/integration_test_persistent_factory_components.xml b/scr/src/test/resources/integration_test_persistent_factory_components.xml
index 141820c..a5778a3 100644
--- a/scr/src/test/resources/integration_test_persistent_factory_components.xml
+++ b/scr/src/test/resources/integration_test_persistent_factory_components.xml
@@ -38,6 +38,32 @@
<implementation class="org.apache.felix.scr.integration.components.SimpleComponent" />
</scr:component>
+ <scr:component name="factory.component.configuration.optional"
+ felix:persistentFactoryComponent="true"
+ enabled="false"
+ configuration-policy="optional"
+ factory="factory.component.factory.configuration.optional"
+ modified="modified" >
+ <implementation class="org.apache.felix.scr.integration.components.SimpleComponent" />
+ </scr:component>
+
+ <scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.3.0" name="factory.component.configuration.optional.13"
+ felix:persistentFactoryComponent="true"
+ enabled="false"
+ configuration-policy="optional"
+ factory="factory.component.factory.configuration.optional.13"
+ modified="modified" >
+ <implementation class="org.apache.felix.scr.integration.components.SimpleComponent" />
+ </scr:component>
+
+ <scr:component name="factory.component.configuration.optional.nomodify"
+ felix:persistentFactoryComponent="true"
+ enabled="false"
+ configuration-policy="optional"
+ factory="factory.component.factory.configuration.optional.nomodify">
+ <implementation class="org.apache.felix.scr.integration.components.SimpleComponent" />
+ </scr:component>
+
<!-- Component Factory Instances, requiring configuration -->
<scr:component name="factory.component.reference"
felix:persistentFactoryComponent="true"
diff --git a/scr/src/test/resources/integration_test_simple_factory_components.xml b/scr/src/test/resources/integration_test_simple_factory_components.xml
index e8e9576..9109745 100644
--- a/scr/src/test/resources/integration_test_simple_factory_components.xml
+++ b/scr/src/test/resources/integration_test_simple_factory_components.xml
@@ -36,7 +36,29 @@
<implementation class="org.apache.felix.scr.integration.components.SimpleComponent" />
</scr:component>
- <!-- Component Factory Instances, requiring configuration -->
+ <scr:component name="factory.component.configuration.optional"
+ enabled="false"
+ configuration-policy="optional"
+ factory="factory.component.factory.configuration.optional"
+ modified="modified" >
+ <implementation class="org.apache.felix.scr.integration.components.SimpleComponent" />
+ </scr:component>
+
+ <scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.3.0" name="factory.component.configuration.optional.13"
+ enabled="false"
+ configuration-policy="optional"
+ factory="factory.component.factory.configuration.optional.13"
+ modified="modified" >
+ <implementation class="org.apache.felix.scr.integration.components.SimpleComponent" />
+ </scr:component>
+
+ <scr:component name="factory.component.configuration.optional.nomodify"
+ enabled="false"
+ configuration-policy="optional"
+ factory="factory.component.factory.configuration.optional.nomodify">
+ <implementation class="org.apache.felix.scr.integration.components.SimpleComponent" />
+ </scr:component>
+
<scr:component name="factory.component.reference"
enabled="false"
configuration-policy="ignore"