felix-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1005430 - in /felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm: impl/ComponentImpl.java tracker/ServiceTracker.java
Date Thu, 07 Oct 2010 12:22:46 GMT
Author: marrs
Date: Thu Oct  7 12:22:46 2010
New Revision: 1005430

URL: http://svn.apache.org/viewvc?rev=1005430&view=rev
Log:
Fixed two issues:
 - non-atomic additions of a list of new dependencies could lead to intermittent states becoming
visible and being acted upon;
 - certain ordering of adding and removing aspects could confuse the ServiceTracker.

Modified:
    felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm/impl/ComponentImpl.java
    felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm/tracker/ServiceTracker.java

Modified: felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm/impl/ComponentImpl.java
URL: http://svn.apache.org/viewvc/felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm/impl/ComponentImpl.java?rev=1005430&r1=1005429&r2=1005430&view=diff
==============================================================================
--- felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm/impl/ComponentImpl.java
(original)
+++ felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm/impl/ComponentImpl.java
Thu Oct  7 12:22:46 2010
@@ -130,6 +130,8 @@ public class ComponentImpl implements Co
     }
     
     private void calculateStateChanges(final State oldState, final State newState) {
+        System.out.println("SS from " + oldState + "\n   to   " + newState);
+        
         if (oldState.isInactive() && (newState.isTrackingOptional())) {
             m_executor.enqueue(new Runnable() {
                 public void run() {
@@ -158,6 +160,11 @@ public class ComponentImpl implements Co
             m_executor.enqueue(new Runnable() {
                 public void run() {
                     // TODO as far as I can see there is nothing left to do here
+                    
+                    
+                    //////////unbindService(newState);
+                    
+                    
                 }});
         }
         if (oldState.isTrackingOptional() && newState.isWaitingForRequired()) {
@@ -222,6 +229,7 @@ public class ComponentImpl implements Co
         m_executor.execute();
     }
     
+    // TODO fix code duplication between add(Dependency) and add(List)
     public Component add(final Dependency dependency) {
     	State oldState, newState;
         synchronized (m_dependencies) {
@@ -229,36 +237,85 @@ public class ComponentImpl implements Co
             m_dependencies.add(dependency);
         }
         
-        if (dependency.isInstanceBound()) {
-            // At this point: this dependency is added from init(): but we don't want to
start it now, 
-            // because if we start it, and if the required dependency is available, then
the service.start() 
-            // method will be called, and this is a problem if a further
-            // required (but unavailable) dependency is then added again from the init()
method ...
-            // Once the init() method will return, the activateService method will then calculate
the state changes,
-            // but at this point, all added extra-dependencies will be known.
-            return this;
-        } 
+//        if (dependency.isInstanceBound()) {
+//            // At this point: this dependency is added from init(): but we don't want to
start it now, 
+//            // because if we start it, and if the required dependency is available, then
the service.start() 
+//            // method will be called, and this is a problem if a further
+//            // required (but unavailable) dependency is then added again from the init()
method ...
+//            // Once the init() method will return, the activateService method will then
calculate the state changes,
+//            // but at this point, all added extra-dependencies will be known.
+//            return this;
+//        } 
+        
+        ///
+        
+//        if (oldState.isAllRequiredAvailable() || (oldState.isWaitingForRequiredInstantiated()
&& dependency.isRequired()) || (oldState.isWaitingForRequired() && dependency.isRequired()))
{
+//        	((DependencyActivation) dependency).start(this);
+//        }
         
-        if (oldState.isAllRequiredAvailable() || (oldState.isWaitingForRequiredInstantiated()
&& dependency.isRequired()) || (oldState.isWaitingForRequired() && dependency.isRequired()))
{
-        	((DependencyActivation) dependency).start(this);
+        // if we're inactive, don't do anything, otherwise we might want to start
+        // the dependency
+        if (!oldState.isInactive()) {
+            // if the dependency is required, it should be started regardless of the state
+            // we're in
+            if (dependency.isRequired()) {
+                ((DependencyActivation) dependency).start(this);
+            }
+            else {
+                // if the dependency is optional, it should only be started if we're in
+                // bound state
+                if (oldState.isBound()) {
+                    ((DependencyActivation) dependency).start(this);
+                }
+            }
         }
 
         synchronized (m_dependencies) {
             // starting the dependency above might have triggered another state change, so
             // we have to fetch the current state again
-            oldState = m_state;
             newState = new State((List) m_dependencies.clone(), !oldState.isInactive(), m_isInstantiated,
m_isBound);
             m_state = newState;
         }
         calculateStateChanges(oldState, newState);
         return this;
     }
-
+    
     public Component add(List dependencies) {
-        // TODO review if this can be done more smartly
-        for (int i = 0; i < dependencies.size(); i++) {
-            add((Dependency) dependencies.get(i));
+        State oldState, newState;
+        synchronized (m_dependencies) {
+            oldState = m_state;
+            for (int i = 0; i < dependencies.size(); i++) {
+                m_dependencies.add(dependencies.get(i));
+            }
+        }
+        
+        // if we're inactive, don't do anything, otherwise we might want to start
+        // the dependencies
+        if (!oldState.isInactive()) {
+            for (int i = 0; i < dependencies.size(); i++) {
+                Dependency dependency = (Dependency) dependencies.get(i);
+                // if the dependency is required, it should be started regardless of the
state
+                // we're in
+                if (dependency.isRequired()) {
+                    ((DependencyActivation) dependency).start(this);
+                }
+                else {
+                    // if the dependency is optional, it should only be started if we're
in
+                    // bound state
+                    if (oldState.isBound()) {
+                        ((DependencyActivation) dependency).start(this);
+                    }
+                }
+            }
         }
+
+        synchronized (m_dependencies) {
+            // starting the dependency above might have triggered another state change, so
+            // we have to fetch the current state again
+            newState = new State((List) m_dependencies.clone(), !oldState.isInactive(), m_isInstantiated,
m_isBound);
+            m_state = newState;
+        }
+        calculateStateChanges(oldState, newState);
         return this;
     }
 
@@ -544,8 +601,8 @@ public class ComponentImpl implements Co
         // then we invoke the init callback so the service can further initialize
         // itself
         invoke(init);
-        // start extra/required dependencies which might have been added from the init()
method.
-        startExtraRequiredDependencies();
+//        // start extra/required dependencies which might have been added from the init()
method.
+//        startExtraRequiredDependencies();
         // see if any of this caused further state changes
         calculateStateChanges();
     }
@@ -567,7 +624,7 @@ public class ComponentImpl implements Co
             start = m_callbackStart;
         }
         
-        // configure service with extra-dependencies which might have been added from init()
method.
+//        // configure service with extra-dependencies which might have been added from init()
method.
         configureServiceWithExtraDependencies(state);
         // inform the state listeners we're starting
         stateListenersStarting();

Modified: felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm/tracker/ServiceTracker.java
URL: http://svn.apache.org/viewvc/felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm/tracker/ServiceTracker.java?rev=1005430&r1=1005429&r2=1005430&view=diff
==============================================================================
--- felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm/tracker/ServiceTracker.java
(original)
+++ felix/trunk/dependencymanager/core/src/main/java/org/apache/felix/dm/tracker/ServiceTracker.java
Thu Oct  7 12:22:46 2010
@@ -822,22 +822,24 @@ public class ServiceTracker implements S
 	    private ServiceReference highestHidden(long serviceId) {
 	        ServiceReference result = null;
 	        int max = Integer.MIN_VALUE;
-	        for (int i = 0; i < m_hidden.size(); i++) {
-	            ServiceReference ref = (ServiceReference) m_hidden.get(i);
-	            Long sid = (Long) ref.getProperty(Constants.SERVICE_ID);
-	            Long aid = (Long) ref.getProperty(DependencyManager.ASPECT);
-	            if ((aid != null && aid.longValue() == serviceId) 
-	                || (aid == null && sid != null && sid.longValue() == serviceId))
{
-	                Integer ranking = (Integer) ref.getProperty(Constants.SERVICE_RANKING);
-	                int r = 0;
-	                if (ranking != null) {
-	                    r = ranking.intValue();
-	                }
-	                if (r > max) {
-	                    max = r;
-	                    result = ref;
-	                }
-	            }
+	        synchronized (m_hidden) {
+    	        for (int i = 0; i < m_hidden.size(); i++) {
+    	            ServiceReference ref = (ServiceReference) m_hidden.get(i);
+    	            Long sid = (Long) ref.getProperty(Constants.SERVICE_ID);
+    	            Long aid = (Long) ref.getProperty(DependencyManager.ASPECT);
+    	            if ((aid != null && aid.longValue() == serviceId) 
+    	                || (aid == null && sid != null && sid.longValue() ==
serviceId)) {
+    	                Integer ranking = (Integer) ref.getProperty(Constants.SERVICE_RANKING);
+    	                int r = 0;
+    	                if (ranking != null) {
+    	                    r = ranking.intValue();
+    	                }
+    	                if (r > max) {
+    	                    max = r;
+    	                    result = ref;
+    	                }
+    	            }
+    	        }
 	        }
 	        return result;
 	    }
@@ -879,7 +881,10 @@ public class ServiceTracker implements S
          */
         private void hide(ServiceReference ref) {
             if (DEBUG) { System.out.println("ServiceTracker.Tracked.hide " + ServiceUtil.toString(ref));
}
-            m_hidden.add(ref);
+            synchronized (m_hidden) {
+                if (DEBUG) { if (m_hidden.contains(ref)) { System.out.println("ServiceTracker.Tracked.hide
ERROR: " + ServiceUtil.toString(ref)); }};
+                m_hidden.add(ref);
+            }
         }
         
         /**
@@ -889,7 +894,10 @@ public class ServiceTracker implements S
          */
         private void unhide(ServiceReference ref) {
             if (DEBUG) { System.out.println("ServiceTracker.Tracked.unhide " + ServiceUtil.toString(ref));
}
-            m_hidden.remove(ref);
+            synchronized (m_hidden) {
+                if (DEBUG) { if (!m_hidden.contains(ref)) { System.out.println("ServiceTracker.Tracked.unhide
ERROR: " + ServiceUtil.toString(ref)); }};
+                m_hidden.remove(ref);
+            }
         }
 	    
 		/**
@@ -979,7 +987,7 @@ public class ServiceTracker implements S
 				    ServiceReference higher = null;
 				    ServiceReference lower = null;
 				    boolean isAspect = ServiceUtil.isAspect(reference);
-				    if (isAspect) {
+				    if (true /* WAS isAspect */) {
     				    long sid = ServiceUtil.getServiceId(reference);
     				    ServiceReference sr = highestTracked(sid);
     				    if (sr != null) {
@@ -1054,7 +1062,7 @@ public class ServiceTracker implements S
 						else {
 		                    higher = null;
 		                    isAspect = ServiceUtil.isAspect(reference);
-		                    if (isAspect) {
+		                    if (true /* WAS isAspect */) {
 		                        long sid = ServiceUtil.getServiceId(reference);
 		                        ServiceReference sr = highestHidden(sid);
 		                        if (sr != null) {
@@ -1084,7 +1092,7 @@ public class ServiceTracker implements S
 				case ServiceEvent.UNREGISTERING :
                     higher = null;
                     isAspect = ServiceUtil.isAspect(reference);
-                    if (isAspect) {
+                    if (true /* WAS isAspect */) {
                         long sid = ServiceUtil.getServiceId(reference);
                         ServiceReference sr = highestHidden(sid);
                         if (sr != null) {



Mime
View raw message