FELIX-4223 Use read-write lock to prevent activation/deactivation during config update

git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1523553 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 5140c03..e812dc2 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
@@ -37,6 +37,7 @@
 import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import org.apache.felix.scr.Component;
 import org.apache.felix.scr.Reference;
@@ -105,7 +106,7 @@
 
     protected volatile boolean m_internalEnabled;
     
-    private volatile boolean m_disposed;
+    protected volatile boolean m_disposed;
     
     //service event tracking
     private int m_floor;
@@ -117,6 +118,8 @@
     private final Set<Integer> m_missing = new TreeSet<Integer>( );
 
     volatile boolean m_activated;
+    
+    protected final ReentrantReadWriteLock m_activationLock = new ReentrantReadWriteLock();
 
     /**
      * The constructor receives both the activator and the metadata
@@ -170,13 +173,23 @@
         }
     }
 
-    final void obtainWriteLock( String source )
+    final long getLockTimeout()
+    {
+        BundleComponentActivator activator = getActivator();
+        if ( activator != null )
+        {
+            return activator.getConfiguration().lockTimeout();
+        }
+        return ScrConfiguration.DEFAULT_LOCK_TIMEOUT_MILLISECONDS;
+    }
+
+    private void obtainLock( Lock lock, String source )
     {
         try
         {
-            if (!m_stateLock.tryLock( getLockTimeout(), TimeUnit.MILLISECONDS ) )
+            if (!lock.tryLock( getLockTimeout(), TimeUnit.MILLISECONDS ) )
             {
-            	dumpThreads();
+                dumpThreads();
                 throw new IllegalStateException( "Could not obtain lock" );
             }
         }
@@ -184,7 +197,7 @@
         {
             try
             {
-                if (!m_stateLock.tryLock( getLockTimeout(), TimeUnit.MILLISECONDS ) )
+                if (!lock.tryLock( getLockTimeout(), TimeUnit.MILLISECONDS ) )
                 {
                     dumpThreads();
                     throw new IllegalStateException( "Could not obtain lock" );
@@ -199,28 +212,46 @@
             Thread.currentThread().interrupt();
         }
     }
-
-    long getLockTimeout()
+    
+    final void obtainActivationReadLock( String source )
     {
-        BundleComponentActivator activator = getActivator();
-        if ( activator != null )
-        {
-            return activator.getConfiguration().lockTimeout();
-        }
-        return ScrConfiguration.DEFAULT_LOCK_TIMEOUT_MILLISECONDS;
+        obtainLock( m_activationLock.readLock(), source);
     }
 
-    final void releaseWriteLock( String source )
+    final void releaseActivationReadLock( String source )
+    {
+        m_activationLock.readLock().unlock();
+    }
+    
+    final void obtainActivationWriteLock( String source )
+    {
+        obtainLock( m_activationLock.writeLock(), source);
+    }
+
+    final void releaseActivationWriteeLock( String source )
+    {
+        if ( m_activationLock.getWriteHoldCount() > 0 )
+        {
+            m_activationLock.writeLock().unlock();
+        }
+    }
+    
+    final void obtainStateLock( String source )
+    {
+        obtainLock( m_stateLock, source );
+    }
+
+    final void releaseStateLock( String source )
     {
         m_stateLock.unlock();
     }
 
-    final boolean isWriteLocked()
+    final boolean isStateLocked()
     {
         return m_stateLock.getHoldCount() > 0;
     }
     
-    void dumpThreads()
+    final void dumpThreads()
     {
         try
         {
@@ -383,10 +414,6 @@
                 activateInternal( m_trackingCount.get() );
             }
         }
-        catch ( InterruptedException e )
-        {
-            Thread.currentThread().interrupt();
-        }
         finally
         {
             if ( !async )
@@ -424,14 +451,38 @@
         }
     }
 
-    CountDownLatch enableLatchWait() throws InterruptedException
+    /**
+     * Use a CountDownLatch as a non-reentrant "lock" that can be passed between threads.
+     * This lock assures that enable, disable, and reconfigure operations do not overlap.
+     * 
+     * @return the latch to count down when the operation is complete (in the calling or another thread)
+     * @throws InterruptedException
+     */
+    CountDownLatch enableLatchWait()
     {
         CountDownLatch enabledLatch;
         CountDownLatch newEnabledLatch;
         do
         {
             enabledLatch = m_enabledLatchRef.get();
-            enabledLatch.await();
+            boolean waited = false;
+            boolean interrupted = false;
+            while ( !waited )
+            {
+                try
+                {
+                    enabledLatch.await();
+                    waited = true;
+                }
+                catch ( InterruptedException e )
+                {
+                    interrupted = true;
+                }
+            }
+            if ( interrupted )
+            {
+                Thread.currentThread().interrupt();
+            }
             newEnabledLatch = new CountDownLatch(1);
         }
         while ( !m_enabledLatchRef.compareAndSet( enabledLatch, newEnabledLatch) );
@@ -466,10 +517,6 @@
             }
             disableInternal();
         }
-        catch ( InterruptedException e )
-        {
-            Thread.currentThread().interrupt();
-        }
         finally
         {
             if (!async)
@@ -758,24 +805,32 @@
             return;
         }
 
-        // Before creating the implementation object, we are going to
-        // test if all the mandatory dependencies are satisfied
-        if ( !verifyDependencyManagers() )
+        obtainActivationReadLock( "activateInternal" );
+        try
         {
-            log( LogService.LOG_DEBUG, "Not all dependencies satisfied, cannot activate", null );
-            return;
+            // Before creating the implementation object, we are going to
+            // test if all the mandatory dependencies are satisfied
+            if ( !verifyDependencyManagers() )
+            {
+                log( LogService.LOG_DEBUG, "Not all dependencies satisfied, cannot activate", null );
+                return;
+            }
+
+            if ( !registerService() )
+            {
+                //some other thread is activating us, or we got concurrently deactivated.
+                return;
+            }
+
+
+            if ( ( isImmediate() || getComponentMetadata().isFactory() ) )
+            {
+                getServiceInternal();
+            }
         }
-
-        if ( !registerService() )
+        finally
         {
-            //some other thread is activating us, or we got concurrently deactivated.
-            return;
-        }
-
-
-        if ( ( isImmediate() || getComponentMetadata().isFactory() ) )
-        {
-            getServiceInternal();
+            releaseActivationReadLock( "activateInternal" );
         }
     }
 
@@ -794,7 +849,15 @@
 
         // catch any problems from deleting the component to prevent the
         // component to remain in the deactivating state !
-        doDeactivate( reason, disable );
+        obtainActivationReadLock( "deactivateInternal" );
+        try
+        {
+            doDeactivate( reason, disable );
+        }
+        finally 
+        {
+            releaseActivationReadLock( "deactivateInternal" );
+        }
         if ( isFactory() )
         {
             clear();
@@ -835,7 +898,7 @@
             {
                 log( LogService.LOG_DEBUG, "Component deactivation occuring on another thread", null );
             }
-            obtainWriteLock( "AbstractComponentManager.State.doDeactivate.1" );
+            obtainStateLock( "AbstractComponentManager.State.doDeactivate.1" );
             try
             {
                 if ( m_activated )
@@ -852,7 +915,7 @@
             }
             finally
             {
-                releaseWriteLock( "AbstractComponentManager.State.doDeactivate.1" );
+                releaseStateLock( "AbstractComponentManager.State.doDeactivate.1" );
             }
         }
         catch ( Throwable t )
diff --git a/scr/src/main/java/org/apache/felix/scr/impl/manager/ImmediateComponentManager.java b/scr/src/main/java/org/apache/felix/scr/impl/manager/ImmediateComponentManager.java
index 4f09836..df9077a 100644
--- a/scr/src/main/java/org/apache/felix/scr/impl/manager/ImmediateComponentManager.java
+++ b/scr/src/main/java/org/apache/felix/scr/impl/manager/ImmediateComponentManager.java
@@ -118,7 +118,7 @@
     // also be overwritten
     protected boolean createComponent()
     {
-        if ( !isWriteLocked() )
+        if ( !isStateLocked() )
         {
             throw new IllegalStateException( "need write lock (createComponent)" );
         }
@@ -160,7 +160,7 @@
 
     protected void deleteComponent( int reason )
     {
-        if ( !isWriteLocked() )
+        if ( !isStateLocked() )
         {
             throw new IllegalStateException( "need write lock (deleteComponent)" );
         }
@@ -574,34 +574,11 @@
             
             // reactivate the component to ensure it is provided with the
             // configuration data
-            if ( ( getState() & ( STATE_DISPOSED | STATE_DISABLED ) ) != 0 )
+            if ( m_disposed || !m_internalEnabled )
             {
                 // nothing to do for inactive components, leave this method
-                log( LogService.LOG_DEBUG, "Component can not be configured in state {0}", new Object[] { getState() }, null );
-                //m_internalEnabled is false, we don't need to worry about activation
-                updateTargets( getProperties() );
-                return;
-            }
-
-            //TODO wait for activation/deactivation to complete, then lock(?) or internal disable...
-            
-            // unsatisfied component and non-ignored configuration may change targets
-            // to satisfy references
-            if ( getState() == STATE_UNSATISFIED
-                    && !getComponentMetadata().isConfigurationIgnored() )
-            {
-                log( LogService.LOG_DEBUG, "Attempting to activate unsatisfied component", null );
-                //do not allow activation before all targets are reset
-                m_internalEnabled = false;
-                try
-                {
-                    updateTargets( getProperties() );
-                }
-                finally 
-                {
-                    m_internalEnabled = true;
-                }
-                activateInternal( getTrackingCount().get() );
+                log( LogService.LOG_DEBUG, "Component can not be activated due to configuration in state {0}", new Object[] { getState() }, null );
+                //enabling the component will set the target properties, do nothing now.
                 return;
             }
 
@@ -615,33 +592,51 @@
                 //when a configuration arrives the properties will get set based on the new configuration.
                 return;
             }
-            if ( !modify() )
-            {
-                // SCR 112.7.1 - deactivate if configuration is deleted or no modified method declared
-                log( LogService.LOG_DEBUG, "Deactivating and Activating to reconfigure from configuration", null );
-                int reason = ( configuration == null ) ? ComponentConstants.DEACTIVATION_REASON_CONFIGURATION_DELETED
-                        : ComponentConstants.DEACTIVATION_REASON_CONFIGURATION_MODIFIED;
 
-                // FELIX-2368: cycle component immediately, reconfigure() is
-                //     called through ConfigurationListener API which itself is
-                //     called asynchronously by the Configuration Admin Service
-                deactivateInternal( reason, false, getTrackingCount().get() );
-                //do not allow reactivation before all targets are reset
-                m_internalEnabled = false;
-                try
+            // unsatisfied component and non-ignored configuration may change targets
+            // to satisfy references
+            obtainActivationWriteLock( "reconfigure" );
+            try
+            {
+                if ( getState() == STATE_UNSATISFIED
+                        && !getComponentMetadata().isConfigurationIgnored() )
                 {
+                    log( LogService.LOG_DEBUG, "Attempting to activate unsatisfied component", null );
                     updateTargets( getProperties() );
+                    releaseActivationWriteeLock( "reconfigure.unsatisfied" );
+                    activateInternal( getTrackingCount().get() );
+                    return;
                 }
-                finally 
+
+                if ( !modify() )
                 {
-                    m_internalEnabled = true;
+                    // SCR 112.7.1 - deactivate if configuration is deleted or no modified method declared
+                    log( LogService.LOG_DEBUG, "Deactivating and Activating to reconfigure from configuration", null );
+                    int reason = ( configuration == null ) ? ComponentConstants.DEACTIVATION_REASON_CONFIGURATION_DELETED
+                            : ComponentConstants.DEACTIVATION_REASON_CONFIGURATION_MODIFIED;
+
+                    // 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, getTrackingCount().get() );
+                    obtainActivationWriteLock( "reconfigure.deactivate.activate" );
+                    try
+                    {
+                        updateTargets( getProperties() );
+                    }
+                    finally
+                    {
+                        releaseActivationWriteeLock( "reconfigure.deactivate.activate" );;
+                    }
+                    activateInternal( getTrackingCount().get() );
                 }
-                activateInternal( getTrackingCount().get() );
             }
-        }
-        catch ( InterruptedException e )
-        {
-            Thread.currentThread().interrupt();
+            finally
+            {
+                //used if modify succeeds or if there's an exception.
+                releaseActivationWriteeLock( "reconfigure.end" );;
+            }
         }
         finally
         {
@@ -680,7 +675,7 @@
         // 4. call method (nothing to do when failed, since it has already been logged)
         //   (call with non-null default result to continue even if the
         //    modify method call failed)
-        obtainWriteLock( "ImmediateComponentManager.modify" );
+        obtainStateLock( "ImmediateComponentManager.modify" );
         try
         {
             //cf 112.5.12 where invoking modified method before updating target services is specified.
@@ -720,7 +715,7 @@
         }
         finally
         {
-            releaseWriteLock( "ImmediateComponentManager.modify" );
+            releaseStateLock( "ImmediateComponentManager.modify" );
         }
     }
 
@@ -818,7 +813,7 @@
                             null );
                     success = false;
                 }
-                obtainWriteLock( "ImmediateComponentManager.getService.1" );
+                obtainStateLock( "ImmediateComponentManager.getService.1" );
                 try
                 {
                     if ( m_componentContext == null )
@@ -837,7 +832,7 @@
                 }
                 finally
                 {
-                    releaseWriteLock( "ImmediateComponentManager.getService.1" );
+                    releaseStateLock( "ImmediateComponentManager.getService.1" );
                 }
             }
             return success;
@@ -896,7 +891,7 @@
             // be kept (FELIX-3039)
             if ( useCount == 0 && !isImmediate() && !keepInstances() )
             {
-                obtainWriteLock( "ImmediateComponentManager.ungetService.1" );
+                obtainStateLock( "ImmediateComponentManager.ungetService.1" );
                 try
                 {
                     if ( m_useCount.get() == 0 )
@@ -907,7 +902,7 @@
                 }
                 finally
                 {
-                    releaseWriteLock( "ImmediateComponentManager.ungetService.1" );
+                    releaseStateLock( "ImmediateComponentManager.ungetService.1" );
                 }
             }
         }
diff --git a/scr/src/main/java/org/apache/felix/scr/impl/manager/ServiceFactoryComponentManager.java b/scr/src/main/java/org/apache/felix/scr/impl/manager/ServiceFactoryComponentManager.java
index da43d9c..4658333 100644
--- a/scr/src/main/java/org/apache/felix/scr/impl/manager/ServiceFactoryComponentManager.java
+++ b/scr/src/main/java/org/apache/felix/scr/impl/manager/ServiceFactoryComponentManager.java
@@ -78,7 +78,7 @@
      */
     protected void deleteComponent( int reason )
     {
-        if ( !isWriteLocked() )
+        if ( !isStateLocked() )
         {
             throw new IllegalStateException( "need write lock (deleteComponent)" );
         }