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"