felix-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From djen...@apache.org
Subject svn commit: r1522046 - in /felix/trunk/scr/src/main/java/org/apache/felix/scr/impl: helper/ComponentMethods.java manager/AbstractComponentManager.java manager/ComponentFactoryImpl.java manager/DependencyManager.java manager/ImmediateComponentManager.java
Date Wed, 11 Sep 2013 21:13:51 GMT
Author: djencks
Date: Wed Sep 11 21:13:50 2013
New Revision: 1522046

URL: http://svn.apache.org/r1522046
Log:
FELIX-4223 Dependency manager targets should be set during enable; setting during activate
leads to race conditions.  Also prevent activation during modification of target properties.

Modified:
    felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/helper/ComponentMethods.java
    felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java
    felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/ComponentFactoryImpl.java
    felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
    felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/ImmediateComponentManager.java

Modified: felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/helper/ComponentMethods.java
URL: http://svn.apache.org/viewvc/felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/helper/ComponentMethods.java?rev=1522046&r1=1522045&r2=1522046&view=diff
==============================================================================
--- felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/helper/ComponentMethods.java (original)
+++ felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/helper/ComponentMethods.java Wed
Sep 11 21:13:50 2013
@@ -54,9 +54,8 @@ public class ComponentMethods
 
         m_modifiedMethod = new ModifiedMethod( componentMetadata.getModified(), implementationObjectClass,
isDS11, isDS12Felix );
 
-        for ( Iterator it = componentMetadata.getDependencies().iterator(); it.hasNext();
)
+        for ( ReferenceMetadata referenceMetadata: componentMetadata.getDependencies() )
         {
-            ReferenceMetadata referenceMetadata = ( ReferenceMetadata ) it.next();
             String refName = referenceMetadata.getName();
             BindMethods bindMethods = new BindMethods( referenceMetadata, implementationObjectClass,
isDS11, isDS12Felix);
             bindMethodMap.put( refName, bindMethods );

Modified: felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java
URL: http://svn.apache.org/viewvc/felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java?rev=1522046&r1=1522045&r2=1522046&view=diff
==============================================================================
--- felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java
(original)
+++ felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java
Wed Sep 11 21:13:50 2013
@@ -91,7 +91,7 @@ public abstract class AbstractComponentM
 
     private volatile boolean m_dependenciesCollected;
 
-    private final AtomicInteger trackingCount = new AtomicInteger( );
+    private final AtomicInteger m_trackingCount = new AtomicInteger( );
 
     // A reference to the BundleComponentActivator
     private BundleComponentActivator m_activator;
@@ -100,22 +100,21 @@ public abstract class AbstractComponentM
     
     private final ReentrantLock m_stateLock;
 
-    protected volatile boolean enabled;
-    protected volatile CountDownLatch enabledLatch;
-    private final Object enabledLatchLock = new Object();
+    protected volatile boolean m_enabled;
+    protected final AtomicReference< CountDownLatch> m_enabledLatchRef = new AtomicReference<CountDownLatch>(
new CountDownLatch(0) );
 
     protected volatile boolean m_internalEnabled;
     
-    private volatile boolean disposed;
+    private volatile boolean m_disposed;
     
     //service event tracking
-    private int floor;
+    private int m_floor;
 
-    private volatile int ceiling;
+    private volatile int m_ceiling;
 
-    private final Lock missingLock = new ReentrantLock();
-    private final Condition missingCondition = missingLock.newCondition();
-    private final Set<Integer> missing = new TreeSet<Integer>( );
+    private final Lock m_missingLock = new ReentrantLock();
+    private final Condition m_missingCondition = m_missingLock.newCondition();
+    private final Set<Integer> m_missing = new TreeSet<Integer>( );
 
     volatile boolean m_activated;
 
@@ -237,43 +236,43 @@ public abstract class AbstractComponentM
     //service event tracking
     void tracked( int trackingCount )
     {
-        missingLock.lock();
+        m_missingLock.lock();
         try
         {
-            if (trackingCount == floor + 1 )
+            if (trackingCount == m_floor + 1 )
             {
-                floor++;
-                missing.remove( trackingCount );
+                m_floor++;
+                m_missing.remove( trackingCount );
             }
-            else if ( trackingCount < ceiling )
+            else if ( trackingCount < m_ceiling )
             {
-                missing.remove( trackingCount );
+                m_missing.remove( trackingCount );
             }
-            if ( trackingCount > ceiling )
+            if ( trackingCount > m_ceiling )
             {
-                for (int i = ceiling + 1; i < trackingCount; i++ )
+                for (int i = m_ceiling + 1; i < trackingCount; i++ )
                 {
-                    missing.add( i );
+                    m_missing.add( i );
                 }
-                ceiling = trackingCount;
+                m_ceiling = trackingCount;
             }
-            missingCondition.signalAll();
+            m_missingCondition.signalAll();
         }
         finally
         {
-            missingLock.unlock();
+            m_missingLock.unlock();
         }
     }
 
     void waitForTracked( int trackingCount )
     {
-        missingLock.lock();
+        m_missingLock.lock();
         try
         {
-            while ( ceiling  < trackingCount || ( !missing.isEmpty() && missing.iterator().next()
< trackingCount))
+            while ( m_ceiling  < trackingCount || ( !m_missing.isEmpty() && m_missing.iterator().next()
< trackingCount))
             {
                 log( LogService.LOG_DEBUG, "waitForTracked trackingCount: {0} ceiling: {1}
missing: {2}",
-                        new Object[] {trackingCount, ceiling, missing}, null );
+                        new Object[] {trackingCount, m_ceiling, m_missing}, null );
                 try
                 {
                     if ( !doMissingWait())
@@ -293,7 +292,7 @@ public abstract class AbstractComponentM
                     catch ( InterruptedException e1 )
                     {
                         log( LogService.LOG_ERROR, "waitForTracked interrupted twice: {0}
ceiling: {1} missing: {2},  Expect further errors",
-                                new Object[] {trackingCount, ceiling, missing}, e1 );
+                                new Object[] {trackingCount, m_ceiling, m_missing}, e1 );
                     }
                     Thread.currentThread().interrupt();
                 }
@@ -301,18 +300,18 @@ public abstract class AbstractComponentM
         }
         finally
         {
-            missingLock.unlock();
+            m_missingLock.unlock();
         }
     }
     
     private boolean doMissingWait() throws InterruptedException
     {
-        if ( !missingCondition.await( getLockTimeout(), TimeUnit.MILLISECONDS ))
+        if ( !m_missingCondition.await( getLockTimeout(), TimeUnit.MILLISECONDS ))
         {
             log( LogService.LOG_ERROR, "waitForTracked timed out: {0} ceiling: {1} missing:
{2},  Expect further errors",
-                    new Object[] {trackingCount, ceiling, missing}, null );
+                    new Object[] {m_trackingCount, m_ceiling, m_missing}, null );
             dumpThreads();
-            missing.clear();
+            m_missing.clear();
             return false;
         }
         return true;
@@ -365,24 +364,18 @@ public abstract class AbstractComponentM
 
     public final void enable( final boolean async )
     {
-        if (enabled)
+        if (m_enabled)
         {
             return;
         }
+        CountDownLatch enableLatch = null;
         try
         {
-            synchronized ( enabledLatchLock )
-            {
-                if ( enabledLatch != null )
-                {
-                    enabledLatch.await();
-                }
-                enabledLatch  = new CountDownLatch( 1 );
-            }
+            enableLatch = enableLatchWait();
             enableInternal();
             if ( !async )
             {
-                activateInternal( trackingCount.get() );
+                activateInternal( m_trackingCount.get() );
             }
         }
         catch ( InterruptedException e )
@@ -393,13 +386,14 @@ public abstract class AbstractComponentM
         {
             if ( !async )
             {
-                enabledLatch.countDown();
+                enableLatch.countDown();
             }
-            enabled = true;
+            m_enabled = true;
         }
 
         if ( async )
         {
+            final CountDownLatch latch = enableLatch;
             m_activator.schedule( new Runnable()
             {
 
@@ -409,11 +403,11 @@ public abstract class AbstractComponentM
                 {
                     try
                     {
-                        activateInternal( trackingCount.get() );
+                        activateInternal( m_trackingCount.get() );
                     }
                     finally
                     {
-                        enabledLatch.countDown();
+                        latch.countDown();
                     }
                 }
 
@@ -425,6 +419,20 @@ public abstract class AbstractComponentM
         }
     }
 
+    CountDownLatch enableLatchWait() throws InterruptedException
+    {
+        CountDownLatch enabledLatch;
+        CountDownLatch newEnabledLatch;
+        do
+        {
+            enabledLatch = m_enabledLatchRef.get();
+            enabledLatch.await();
+            newEnabledLatch = new CountDownLatch(1);
+        }
+        while ( !m_enabledLatchRef.compareAndSet( enabledLatch, newEnabledLatch) );
+        return newEnabledLatch;  
+    }
+
     /**
      * Disables this component and - if active - first deactivates it. The
      * component may be reenabled by calling the {@link #enable()} method.
@@ -439,23 +447,17 @@ public abstract class AbstractComponentM
 
     public final void disable( final boolean async )
     {
-        if (!enabled)
+        if (!m_enabled)
         {
             return;
         }
+        CountDownLatch enableLatch = null;
         try
         {
-            synchronized ( enabledLatchLock )
-            {
-                if (enabledLatch != null)
-                {
-                    enabledLatch.await();
-                }
-                enabledLatch = new CountDownLatch( 1 );
-            }
+            enableLatch = enableLatchWait();
             if ( !async )
             {
-                deactivateInternal( ComponentConstants.DEACTIVATION_REASON_DISABLED, true,
trackingCount.get() );
+                deactivateInternal( ComponentConstants.DEACTIVATION_REASON_DISABLED, true,
m_trackingCount.get() );
             }
             disableInternal();
         }
@@ -467,13 +469,14 @@ public abstract class AbstractComponentM
         {
             if (!async)
             {
-                enabledLatch.countDown();
+                enableLatch.countDown();
             }
-            enabled = false;
+            m_enabled = false;
         }
 
         if ( async )
         {
+            final CountDownLatch latch = enableLatch;
             m_activator.schedule( new Runnable()
             {
 
@@ -483,11 +486,11 @@ public abstract class AbstractComponentM
                 {
                     try
                     {
-                        deactivateInternal( ComponentConstants.DEACTIVATION_REASON_DISABLED,
true, trackingCount.get() );
+                        deactivateInternal( ComponentConstants.DEACTIVATION_REASON_DISABLED,
true, m_trackingCount.get() );
                     }
                     finally
                     {
-                        enabledLatch.countDown();
+                        latch.countDown();
                     }
                 }
 
@@ -517,7 +520,7 @@ public abstract class AbstractComponentM
      */
     public void dispose( int reason )
     {
-        disposed = true;
+        m_disposed = true;
         disposeInternal( reason );
     }
     
@@ -678,7 +681,7 @@ public abstract class AbstractComponentM
 
     final void enableInternal()
     {
-        if ( disposed )
+        if ( m_disposed )
         {
             throw new IllegalStateException( "enable: " + this );
         }
@@ -690,6 +693,15 @@ public abstract class AbstractComponentM
         }
 
         registerComponentId();
+        // Before creating the implementation object, we are going to
+        // test if we have configuration if such is required
+        if ( hasConfiguration() || !getComponentMetadata().isConfigurationRequired() )
+        {
+            // Update our target filters.
+            log( LogService.LOG_DEBUG, "Updating target filters", null );
+            updateTargets( getProperties() );
+        }
+
         m_internalEnabled = true;
         log( LogService.LOG_DEBUG, "Component enabled", null );
     }
@@ -698,7 +710,7 @@ public abstract class AbstractComponentM
     {
         log( LogService.LOG_DEBUG, "ActivateInternal",
                 null );
-        if ( disposed )
+        if ( m_disposed )
         {
             log( LogService.LOG_DEBUG, "ActivateInternal: disposed",
                     null );
@@ -741,10 +753,6 @@ public abstract class AbstractComponentM
             return;
         }
 
-        // Update our target filters.
-        log( LogService.LOG_DEBUG, "Updating target filters", null );
-        updateTargets( getProperties() );
-
         // Before creating the implementation object, we are going to
         // test if all the mandatory dependencies are satisfied
         if ( !verifyDependencyManagers() )
@@ -768,7 +776,7 @@ public abstract class AbstractComponentM
 
     final void deactivateInternal( int reason, boolean disable, int trackingCount )
     {
-        if ( disposed )
+        if ( m_disposed )
         {
             return;
         }
@@ -792,7 +800,7 @@ public abstract class AbstractComponentM
     final void disableInternal()
     {
         m_internalEnabled = false;
-        if ( disposed )
+        if ( m_disposed )
         {
             throw new IllegalStateException( "Cannot disable a disposed component " + getName()
);
         }
@@ -976,7 +984,7 @@ public abstract class AbstractComponentM
 
     AtomicInteger getTrackingCount()
     {
-        return trackingCount;
+        return m_trackingCount;
     }
 
 
@@ -1170,14 +1178,11 @@ public abstract class AbstractComponentM
         return depMgrList;
     }
 
-    protected void updateTargets(Dictionary<String, Object> properties)
+    final void updateTargets(Dictionary<String, Object> properties)
     {
-        if ( m_internalEnabled )
+        for ( DependencyManager<S, ?> dm: getDependencyManagers() )
         {
-            for ( DependencyManager<S, ?> dm: getDependencyManagers() )
-            {
-                dm.setTargetFilter( properties );
-            }
+            dm.setTargetFilter( properties );
         }
     }
 
@@ -1360,7 +1365,7 @@ public abstract class AbstractComponentM
 
     public int getState()
     {
-        if (disposed)
+        if (m_disposed)
         {
             return Component.STATE_DISPOSED;
         }

Modified: felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/ComponentFactoryImpl.java
URL: http://svn.apache.org/viewvc/felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/ComponentFactoryImpl.java?rev=1522046&r1=1522045&r2=1522046&view=diff
==============================================================================
--- felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/ComponentFactoryImpl.java
(original)
+++ felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/ComponentFactoryImpl.java
Wed Sep 11 21:13:50 2013
@@ -358,7 +358,7 @@ public class ComponentFactoryImpl<S> ext
         {
             log( LogService.LOG_INFO, "Configuration PID updated for Component Factory",
null );
 
-            // Ignore the configuration is our policy is 'ignore'
+            // Ignore the configuration if our policy is 'ignore'
             if ( getComponentMetadata().isConfigurationIgnored() )
             {
                 return false;

Modified: felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
URL: http://svn.apache.org/viewvc/felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java?rev=1522046&r1=1522045&r2=1522046&view=diff
==============================================================================
--- felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
(original)
+++ felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
Wed Sep 11 21:13:50 2013
@@ -61,7 +61,7 @@ import org.osgi.service.log.LogService;
 public class DependencyManager<S, T> implements Reference
 {
     // mask of states ok to send events
-    private static final int STATE_MASK = //Component.STATE_UNSATISFIED |
+    private static final int STATE_MASK = 
          Component.STATE_ACTIVE | Component.STATE_REGISTERED | Component.STATE_FACTORY;
 
     // the component to which this dependency belongs
@@ -72,11 +72,13 @@ public class DependencyManager<S, T> imp
     
     private final int m_index;
 
-    private final AtomicReference<ServiceTracker<T, RefPair<T>>> trackerRef
= new AtomicReference<ServiceTracker<T, RefPair<T>>>();
+    private final Customizer<T> m_customizer;
 
-    private final Customizer customizer;
+    //only set once, but it's not clear there is enough other synchronization to get the
correct object before it's used.
+    private volatile BindMethods m_bindMethods;
 
-    private BindMethods m_bindMethods;
+    //reset on filter change
+    private volatile ServiceTracker<T, RefPair<T>> m_tracker;
 
     // the target service filter string
     private volatile String m_target;
@@ -84,7 +86,7 @@ public class DependencyManager<S, T> imp
     // the target service filter
     private volatile Filter m_targetFilter;
 
-    private boolean registered;
+    //private volatile boolean m_registered;
 
     /**
      * Constructor that receives several parameters.
@@ -96,7 +98,7 @@ public class DependencyManager<S, T> imp
         m_componentManager = componentManager;
         m_dependencyMetadata = dependency;
         m_index = index;
-        customizer = newCustomizer();
+        m_customizer = newCustomizer();
 
         // dump the reference information if DEBUG is enabled
         if ( m_componentManager.isLogEnabled( LogService.LOG_DEBUG ) )
@@ -156,7 +158,7 @@ public class DependencyManager<S, T> imp
 
         public void setTracker( ServiceTracker<T, RefPair<T>> tracker )
         {
-            trackerRef.set( tracker );
+            m_tracker = tracker;
             m_componentManager.log( LogService.LOG_DEBUG, "dm {0} tracker reset (closed)",
new Object[] {getName()}, null );
             trackerOpened = false;
         }
@@ -173,7 +175,7 @@ public class DependencyManager<S, T> imp
 
         protected ServiceTracker<T, RefPair<T>> getTracker()
         {
-            return trackerRef.get();
+            return m_tracker;
         }
 
         /**
@@ -1110,7 +1112,7 @@ public class DependencyManager<S, T> imp
 
     void deactivate()
     {
-        customizer.close();
+        m_customizer.close();
     }
 
 
@@ -1127,7 +1129,7 @@ public class DependencyManager<S, T> imp
     int size()
     {
         AtomicInteger trackingCount = new AtomicInteger( );
-        return trackerRef.get().getTracked( null, trackingCount ).size();
+        return m_tracker.getTracked( null, trackingCount ).size();
     }
 
 
@@ -1185,11 +1187,7 @@ public class DependencyManager<S, T> imp
      */
     private RefPair<T> getBestRefPair()
     {
-        if (customizer == null )
-        {
-            return null;
-        }
-        Collection<RefPair<T>> refs = customizer.getRefs( new AtomicInteger(
) );
+        Collection<RefPair<T>> refs = m_customizer.getRefs( new AtomicInteger(
) );
         if (refs.isEmpty())
         {
             return null;
@@ -1220,7 +1218,7 @@ public class DependencyManager<S, T> imp
      */
     T[] getServices()
     {
-        Collection<RefPair<T>> refs = customizer.getRefs(  new AtomicInteger(
) );
+        Collection<RefPair<T>> refs = m_customizer.getRefs(  new AtomicInteger(
) );
         List<T> services = new ArrayList<T>( refs.size() );
         for ( RefPair<T> ref: refs)
         {
@@ -1243,7 +1241,7 @@ public class DependencyManager<S, T> imp
      */
     public ServiceReference<T>[] getServiceReferences()
     {
-        Collection<RefPair<T>> bound = customizer.getRefs(  new AtomicInteger(
) );
+        Collection<RefPair<T>> bound = m_customizer.getRefs(  new AtomicInteger(
) );
         if ( bound.isEmpty() )
         {
             return null;
@@ -1280,7 +1278,7 @@ public class DependencyManager<S, T> imp
     private RefPair<T> getRefPair( ServiceReference<T> serviceReference )
     {
         AtomicInteger trackingCount = new AtomicInteger( );
-        return trackerRef.get().getTracked( null, trackingCount ).get( serviceReference );
+        return m_tracker.getTracked( null, trackingCount ).get( serviceReference );
     }
 
 
@@ -1369,7 +1367,7 @@ public class DependencyManager<S, T> imp
      */
     public boolean isSatisfied()
     {
-        return customizer.isSatisfied();
+        return m_customizer.isSatisfied();
     }
 
 
@@ -1391,7 +1389,7 @@ public class DependencyManager<S, T> imp
 
     boolean prebind()
     {
-        return customizer.prebind();
+        return m_customizer.prebind();
     }
 
     /**
@@ -1429,9 +1427,9 @@ public class DependencyManager<S, T> imp
         AtomicInteger trackingCount =  new AtomicInteger( );
         Collection<RefPair<T>> refs;
         CountDownLatch openLatch = new CountDownLatch(1);
-        synchronized ( trackerRef.get().tracked() )
+        synchronized ( m_tracker.tracked() )
         {
-            refs = customizer.getRefs( trackingCount );
+            refs = m_customizer.getRefs( trackingCount );
             edgeInfo.setOpen( trackingCount.get() );
             edgeInfo.setOpenLatch( openLatch );
         }
@@ -1470,9 +1468,9 @@ public class DependencyManager<S, T> imp
         AtomicInteger trackingCount = new AtomicInteger();
         Collection<RefPair<T>> refPairs;
         CountDownLatch latch = new CountDownLatch( 1 );
-        synchronized ( trackerRef.get().tracked() )
+        synchronized ( m_tracker.tracked() )
         {
-            refPairs = customizer.getRefs( trackingCount );
+            refPairs = m_customizer.getRefs( trackingCount );
             edgeInfo.setClose( trackingCount.get() );
             edgeInfo.setCloseLatch( latch );
         }
@@ -1500,7 +1498,7 @@ public class DependencyManager<S, T> imp
         }
         if ( !isMultiple() )
         {
-            Collection<RefPair<T>> refs = customizer.getRefs( new AtomicInteger(
) );
+            Collection<RefPair<T>> refs = m_customizer.getRefs( new AtomicInteger(
) );
             if (refs.isEmpty())
             {
                 return;
@@ -1513,7 +1511,7 @@ public class DependencyManager<S, T> imp
             }
         }
         //TODO dynamic reluctant
-        RefPair<T> refPair = trackerRef.get().getService( ref );
+        RefPair<T> refPair = m_tracker.getService( ref );
         if (refPair.getServiceObject() != null)
         {
             m_componentManager.log( LogService.LOG_DEBUG,
@@ -1552,7 +1550,7 @@ public class DependencyManager<S, T> imp
         // null. This is valid for both immediate and delayed components
         if ( componentInstance != null )
         {
-            synchronized ( trackerRef.get().tracked() )
+            synchronized ( m_tracker.tracked() )
             {
                 if (info.outOfRange( trackingCount ) )
                 {
@@ -1603,15 +1601,7 @@ public class DependencyManager<S, T> imp
         // null. This is valid for both immediate and delayed components
         if ( componentInstance != null )
         {
-            if (refPair == null)
-            {
-
-                //TODO should this be possible? If so, reduce or eliminate logging
-                m_componentManager.log( LogService.LOG_WARNING,
-                        "DependencyManager : invokeUpdatedMethod : Component set, but reference
not present", null );
-                return;
-            }
-            synchronized ( trackerRef.get().tracked() )
+            synchronized ( m_tracker.tracked() )
             {
                 if (info.outOfRange( trackingCount ) )
                 {
@@ -1742,13 +1732,6 @@ public class DependencyManager<S, T> imp
                 //ignore
             }
 
-            if (refPair == null)
-            {
-                //TODO should this be possible? If so, reduce or eliminate logging
-                m_componentManager.log( LogService.LOG_WARNING,
-                        "DependencyManager : invokeUnbindMethod : Component set, but reference
not present {0}",  new Object[] {getName()}, null );
-                return;
-            }
             if ( !getServiceObject( m_bindMethods.getUnbind(), refPair ))
             {
                 m_componentManager.log( LogService.LOG_WARNING,
@@ -1863,15 +1846,7 @@ public class DependencyManager<S, T> imp
      */
     void setTargetFilter( Dictionary<String, Object> properties )
     {
-        try
-        {
-            setTargetFilter( ( String ) properties.get( m_dependencyMetadata.getTargetPropertyName()
) );
-        }
-        catch ( InvalidSyntaxException e )
-        {
-            // this should not occur.  The only choice would be if the filter for the object
class was invalid,
-            // but we already set this once when we enabled.
-        }
+        setTargetFilter( ( String ) properties.get( m_dependencyMetadata.getTargetPropertyName()
) );
     }
 
 
@@ -1885,7 +1860,7 @@ public class DependencyManager<S, T> imp
      * @param target The new target filter to be set. This may be
      *      <code>null</code> if no target filtering is to be used.
      */
-    private void setTargetFilter( String target) throws InvalidSyntaxException
+    private void setTargetFilter( String target)
     {
         // if configuration does not set filter, use the value from metadata
         if (target == null)
@@ -1896,8 +1871,8 @@ public class DependencyManager<S, T> imp
         if ( ( m_target == null && target == null ) || ( m_target != null &&
m_target.equals( target ) ) )
         {
             m_componentManager.log( LogService.LOG_DEBUG, "No change in target property for
dependency {0}: currently registered: {1}", new Object[]
-                    {getName(), registered}, null );
-            if (registered)
+                    {getName(), m_tracker != null}, null );
+            if (m_tracker != null)
             {
                 return;
             }
@@ -1909,31 +1884,24 @@ public class DependencyManager<S, T> imp
             filterString = "(&" + filterString + m_target + ")";
         }
 
-        SortedMap<ServiceReference<T>, RefPair<T>> refMap;
-        if ( registered )
-        {
-            refMap = unregisterServiceListener();
-        }
-        else
-        {
-            refMap = new TreeMap<ServiceReference<T>, RefPair<T>>(Collections.reverseOrder());
-        }
+        final ServiceTracker<T, RefPair<T>> oldTracker = m_tracker;
+        SortedMap<ServiceReference<T>, RefPair<T>> refMap = unregisterServiceListener();
         m_componentManager.log( LogService.LOG_DEBUG, "Setting target property for dependency
{0} to {1}", new Object[]
                 {getName(), target}, null );
         BundleContext bundleContext = m_componentManager.getBundleContext();
         if ( bundleContext != null )
         {
-        try
-        {
+            try
+            {
                 m_targetFilter = bundleContext.createFilter( filterString );
-        }
-        catch ( InvalidSyntaxException ise )
-        {
-            m_componentManager.log( LogService.LOG_ERROR, "Invalid syntax in target property
for dependency {0} to {1}", new Object[]
-                    {getName(), target}, null );
-            // TODO this is an error, how do we recover?
-            return; //avoid an NPE
-        }
+            }
+            catch ( InvalidSyntaxException ise )
+            {
+                m_componentManager.log( LogService.LOG_ERROR, "Invalid syntax in target property
for dependency {0} to {1}", new Object[]
+                        {getName(), target}, null );
+                // TODO this is an error, how do we recover?
+                return; //avoid an NPE
+            }
         }
         else
         {
@@ -1942,21 +1910,15 @@ public class DependencyManager<S, T> imp
             return;                
         }
 
-        registerServiceListener( bundleContext, refMap );
-    }
-
-    private void registerServiceListener( BundleContext bundleContext, SortedMap<ServiceReference<T>,
RefPair<T>> refMap ) throws InvalidSyntaxException
-    {
-        final ServiceTracker<T, RefPair<T>> oldTracker = trackerRef.get();
-        customizer.setPreviousRefMap( refMap );
+        m_customizer.setPreviousRefMap( refMap );
         boolean initialActive = oldTracker != null && oldTracker.isActive();
-        m_componentManager.log( LogService.LOG_INFO, "New service tracker for {0}, initial
active: {1}", new Object[]
-                {getName(), initialActive}, null );
-        ServiceTracker<T, RefPair<T>> tracker = new ServiceTracker<T, RefPair<T>>(
bundleContext, m_targetFilter, customizer, initialActive );
-        customizer.setTracker( tracker );
-        registered = true;
+        m_componentManager.log( LogService.LOG_INFO, "New service tracker for {0}, initial
active: {1}, previous references: {2}", new Object[]
+                {getName(), initialActive, refMap}, null );
+        ServiceTracker<T, RefPair<T>> tracker = new ServiceTracker<T, RefPair<T>>(
bundleContext, m_targetFilter, m_customizer, initialActive );
+        m_customizer.setTracker( tracker );
+        //        m_registered = true;
         tracker.open( m_componentManager.getTrackingCount() );
-        customizer.setTrackerOpened();
+        m_customizer.setTrackerOpened();
         if ( oldTracker != null )
         {
             oldTracker.completeClose( refMap );
@@ -2013,20 +1975,22 @@ public class DependencyManager<S, T> imp
     SortedMap<ServiceReference<T>, RefPair<T>> unregisterServiceListener()
     {
         SortedMap<ServiceReference<T>, RefPair<T>> refMap;
-        ServiceTracker<T, RefPair<T>> tracker = trackerRef.get();
-//        trackerRef.set( null ); //???
+        ServiceTracker<T, RefPair<T>> tracker = m_tracker;
         if ( tracker != null )
         {
             AtomicInteger trackingCount = new AtomicInteger( );
             refMap = tracker.close( trackingCount );
+            m_tracker = null;
+            m_componentManager.log( LogService.LOG_DEBUG, "unregistering service listener
for dependency {0}", new Object[]
+                    {getName()}, null );
         }
         else
         {
             refMap = new TreeMap<ServiceReference<T>, RefPair<T>>(Collections.reverseOrder());
+            m_componentManager.log( LogService.LOG_DEBUG, " No existing service listener
to unregister for dependency {0}", new Object[]
+                    {getName()}, null );
         }
-        registered = false;
-        m_componentManager.log( LogService.LOG_DEBUG, "unregistering service listener for
dependency {0}", new Object[]
-                {getName()}, null );
+//        m_registered = false;
         return refMap;
     }
 

Modified: felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/ImmediateComponentManager.java
URL: http://svn.apache.org/viewvc/felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/ImmediateComponentManager.java?rev=1522046&r1=1522045&r2=1522046&view=diff
==============================================================================
--- felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/ImmediateComponentManager.java
(original)
+++ felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/ImmediateComponentManager.java
Wed Sep 11 21:13:50 2013
@@ -21,6 +21,7 @@ package org.apache.felix.scr.impl.manage
 
 import java.util.Dictionary;
 import java.util.Hashtable;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import org.apache.felix.scr.impl.BundleComponentActivator;
@@ -528,81 +529,115 @@ public class ImmediateComponentManager<S
      */
     public void reconfigure( Dictionary<String, Object> configuration, long changeCount,
TargetedPID targetedPID )
     {
-        if ( targetedPID == null || !targetedPID.equals( m_targetedPID ) )
-        {
-            m_targetedPID = targetedPID;
-            m_changeCount = -1;
-        }
-        if ( configuration != null )
+        CountDownLatch enableLatch = null;
+        try
         {
-            if ( changeCount <= m_changeCount )
+            enableLatch = enableLatchWait();
+            if ( targetedPID == null || !targetedPID.equals( m_targetedPID ) )
             {
-                log( LogService.LOG_DEBUG,
-                        "ImmediateComponentHolder out of order configuration updated for
pid {0} with existing count {1}, new count {2}",
-                        new Object[] { getConfigurationPid(), m_changeCount, changeCount
}, null );
+                m_targetedPID = targetedPID;
+                m_changeCount = -1;
+            }
+            if ( configuration != null )
+            {
+                if ( changeCount <= m_changeCount )
+                {
+                    log( LogService.LOG_DEBUG,
+                            "ImmediateComponentHolder out of order configuration updated
for pid {0} with existing count {1}, new count {2}",
+                            new Object[] { getConfigurationPid(), m_changeCount, changeCount
}, null );
+                    return;
+                }
+                m_changeCount = changeCount;
+            }
+            else 
+            {
+                m_changeCount = -1;
+            }
+            // nothing to do if there is no configuration (see FELIX-714)
+            if ( configuration == null && m_configurationProperties == null )
+            {
+                log( LogService.LOG_DEBUG, "No configuration provided (or deleted), nothing
to do", null );
                 return;
             }
-            m_changeCount = changeCount;
-        }
-        else 
-        {
-            m_changeCount = -1;
-        }
-        // nothing to do if there is no configuration (see FELIX-714)
-        if ( configuration == null && m_configurationProperties == null )
-        {
-            log( LogService.LOG_DEBUG, "No configuration provided (or deleted), nothing to
do", null );
-            return;
-        }
 
-        // store the properties
-        m_configurationProperties = configuration;
+            // store the properties
+            m_configurationProperties = configuration;
 
-        // clear the current properties to force using the configuration data
-        m_properties = null;
+            // clear the current properties to force using the configuration data
+            m_properties = null;
 
-        // 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
);
-            updateTargets( getProperties() );
-            activateInternal( getTrackingCount().get() );
-            return;
-        }
+            // 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() );
+                return;
+            }
 
-        // reactivate the component to ensure it is provided with the
-        // configuration data
-        if ( ( getState() & ( STATE_ACTIVE | STATE_FACTORY | STATE_REGISTERED ) ) ==
0 )
-        {
-            // 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 );
-            updateTargets( getProperties() );
-            return;
-        }
+            // reactivate the component to ensure it is provided with the
+            // configuration data
+            if ( ( getState() & ( STATE_DISPOSED | STATE_DISABLED ) ) != 0 )
+            {
+                // 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;
+            }
 
-        // if the configuration has been deleted but configuration is required
-        // this component must be deactivated
-        if ( configuration == null && getComponentMetadata().isConfigurationRequired()
)
+            // if the configuration has been deleted but configuration is required
+            // this component must be deactivated
+            if ( configuration == null && getComponentMetadata().isConfigurationRequired()
)
+            {
+                //deactivate and remove service listeners
+                deactivateInternal( ComponentConstants.DEACTIVATION_REASON_CONFIGURATION_DELETED,
true, getTrackingCount().get() );
+                //do not reset targets as that will reinstall the service listeners which
may activate the component.
+                //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
+                {
+                    updateTargets( getProperties() );
+                }
+                finally 
+                {
+                    m_internalEnabled = true;
+                }
+                activateInternal( getTrackingCount().get() );
+            }
+        }
+        catch ( InterruptedException e )
         {
-            deactivateInternal( ComponentConstants.DEACTIVATION_REASON_CONFIGURATION_DELETED,
true, getTrackingCount().get() );
-            updateTargets( getProperties() );
-            return;
+            Thread.currentThread().interrupt();
         }
-        if ( !modify() )
+        finally
         {
-            // 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() );
-            updateTargets( getProperties() );
-            activateInternal( getTrackingCount().get() );
+            enableLatch.countDown();
         }
     }
 
@@ -640,6 +675,7 @@ public class ImmediateComponentManager<S
         obtainWriteLock( "ImmediateComponentManager.modify" );
         try
         {
+            //cf 112.5.12 where invoking modified method before updating target services
is specified.
             final MethodResult result = invokeModifiedMethod();
             updateTargets( props );
             if ( result == null )



Mime
View raw message