felix-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From clem...@apache.org
Subject svn commit: r655622 - in /felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo: ./ architecture/ handlers/configuration/ handlers/dependency/
Date Mon, 12 May 2008 19:52:58 GMT
Author: clement
Date: Mon May 12 12:52:58 2008
New Revision: 655622

URL: http://svn.apache.org/viewvc?rev=655622&view=rev
Log:
Fix a synchronization bug in the instance manager.
Refactor the factory classes.

Modified:
    felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/ComponentFactory.java
    felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/HandlerFactory.java
    felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/HandlerManager.java
    felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/IPojoFactory.java
    felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/InstanceManager.java
    felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/architecture/ComponentTypeDescription.java
    felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/handlers/configuration/ConfigurationHandler.java
    felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/handlers/dependency/Dependency.java

Modified: felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/ComponentFactory.java
URL: http://svn.apache.org/viewvc/felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/ComponentFactory.java?rev=655622&r1=655621&r2=655622&view=diff
==============================================================================
--- felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/ComponentFactory.java (original)
+++ felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/ComponentFactory.java Mon May 12 12:52:58 2008
@@ -49,28 +49,32 @@
 
     /**
      * Tracker used to track required handler factories.
+     * Immutable once set.
      */
     protected Tracker m_tracker;
 
     /**
      * Class loader to delegate loading.
+     * Immutable once set.
      */
-    private FactoryClassloader m_classLoader = null;
+    private FactoryClassloader m_classLoader;
 
     /**
      * Component Implementation class.
      */
-    private byte[] m_clazz = null;
+    private byte[] m_clazz;
 
     /**
      * Component Implementation Class Name.
+     * Immutable once set.
      */
-    private String m_classname = null;
+    private String m_classname;
 
     /**
      * Manipulation Metadata of the internal POJO.
+     * Immutable once set.
      */
-    private PojoMetadata m_manipulation = null;
+    private PojoMetadata m_manipulation;
 
     /**
      * Create a instance manager factory. The class is given in parameter. The
@@ -101,8 +105,9 @@
     }
 
     /**
-     * Check method : allow a factory to check if given element are correct.
+     * Check method : allows a factory to check if given element is well-formed.
      * A component factory metadata are correct if they contain the 'classname' attribute.
+     * As this method is called from the (single-thread) constructor, no synchronization is needed.
      * @param element : the metadata
      * @throws ConfigurationException occurs when the element describing the factory is malformed.
      */
@@ -111,12 +116,19 @@
         if (m_classname == null) { throw new ConfigurationException("A component needs a class name : " + element); }
     }
 
+    /**
+     * Gets the class name.
+     * No synchronization needed, the classname is immutable.
+     * @return the class name.
+     * @see org.apache.felix.ipojo.IPojoFactory#getClassName()
+     */
     public String getClassName() {
         return m_classname;
     }
 
     /**
-     * Create a primitive instance.
+     * Creates a primitive instance.
+     * This method is called when holding the lock.
      * @param config : instance configuration
      * @param context : service context.
      * @param handlers : handler to use
@@ -139,13 +151,14 @@
     }
 
     /**
-     * Define a class.
+     * Defines a class.
+     * This method need to be synchronized to avoid that the classloader is created twice.
      * @param name : qualified name of the class
      * @param clazz : byte array of the class
      * @param domain : protection domain of the class
      * @return the defined class object
      */
-    public Class defineClass(String name, byte[] clazz, ProtectionDomain domain) {
+    public synchronized Class defineClass(String name, byte[] clazz, ProtectionDomain domain) {
         if (m_classLoader == null) {
             m_classLoader = new FactoryClassloader();
         }
@@ -153,63 +166,64 @@
     }
 
     /**
-     * Return the URL of a resource.
+     * Returns the URL of a resource.
      * @param resName : resource name
      * @return the URL of the resource
      */
     public URL getResource(String resName) {
+        //No synchronization needed, the context is immutable and the call is managed by the underlying framework.
         return m_context.getBundle().getResource(resName);
     }
 
     /**
-     * Load a class.
+     * Loads a class.
      * @param className : name of the class to load
      * @return the resulting Class object
      * @throws ClassNotFoundException 
      * @throws ClassNotFoundException : happen when the class is not found
      */
     public Class loadClass(String className) throws ClassNotFoundException {
-        if (m_clazz != null && className.equals(m_classname)) {
-            // Used the factory classloader to load the component implementation
-            // class
-            if (m_classLoader == null) {
-                m_classLoader = new FactoryClassloader();
-            }
-            return m_classLoader.defineClass(m_classname, m_clazz, null);
+        if (m_clazz != null && m_classname.equals(className)) {  // Immutable fields.
+            return defineClass(className, m_clazz, null);
         }
         return m_context.getBundle().loadClass(className);
     }
 
     /**
-     * Start the factory.
+     * Starts the factory.
+     * This method is called with the lock.
      */
-    public synchronized void starting() {
-        if (m_requiredHandlers.size() != 0) {
-            try {
-                String filter = "(&(" + Handler.HANDLER_TYPE_PROPERTY + "=" + PrimitiveHandler.HANDLER_TYPE + ")" + "(factory.state=1)" + ")";
-                m_tracker = new Tracker(m_context, m_context.createFilter(filter), this);
-                m_tracker.open();
-            } catch (InvalidSyntaxException e) {
-                m_logger.log(Logger.ERROR, "A factory filter is not valid: " + e.getMessage());
-                stop();
+    public void starting() {
+        if (m_tracker != null) {
+            return; // Already started
+        } else {
+            if (m_requiredHandlers.size() != 0) {
+                try {
+                    String filter = "(&(" + Handler.HANDLER_TYPE_PROPERTY + "=" + PrimitiveHandler.HANDLER_TYPE + ")" + "(factory.state=1)" + ")";
+                    m_tracker = new Tracker(m_context, m_context.createFilter(filter), this);
+                    m_tracker.open();
+                } catch (InvalidSyntaxException e) {
+                    m_logger.log(Logger.ERROR, "A factory filter is not valid: " + e.getMessage()); //Holding the lock should not be an issue here.
+                    stop();
+                }
             }
         }
     }
 
     /**
-     * Stop all the instance managers.
+     * Stops all the instance managers.
+     * This method is called with the lock.
      */
-    public synchronized void stopping() {
+    public void stopping() {
         if (m_tracker != null) {
             m_tracker.close();
             m_tracker = null;
         }
-        m_classLoader = null;
-        m_clazz = null;
     }
 
     /**
-     * Compute the factory name.
+     * Computes the factory name.
+     * This method does not manipulate any non-immutable fields, so does not need to be synchronized. 
      * @return the factory name.
      */
     public String getFactoryName() {
@@ -224,7 +238,8 @@
     }
 
     /**
-     * Compute required handlers.
+     * Computes required handlers.
+     * This method does not manipulate any non-immutable fields, so does not need to be synchronized.
      * @return the required handler list.
      */
     public List getRequiredHandlerList() {
@@ -259,11 +274,12 @@
     /**
      * A new handler factory is detected.
      * Test if the factory can be used or not.
+     * This method need to be synchronized as it accesses to the content of required handlers.
      * @param reference : the new service reference.
      * @return true if the given factory reference match with a required handler.
      * @see org.apache.felix.ipojo.util.TrackerCustomizer#addingService(org.osgi.framework.ServiceReference)
      */
-    public boolean addingService(ServiceReference reference) {
+    public synchronized boolean addingService(ServiceReference reference) {        
         for (int i = 0; i < m_requiredHandlers.size(); i++) {
             RequiredHandler req = (RequiredHandler) m_requiredHandlers.get(i);
             if (req.getReference() == null && match(req, reference)) {
@@ -271,6 +287,7 @@
                 req.setReference(reference);
                 // If the priority has changed, sort the list.
                 if (oldP != req.getLevel()) {
+                    // Manipulate the list.
                     Collections.sort(m_requiredHandlers);
                 }
                 return true;
@@ -280,11 +297,12 @@
     }
 
     /**
-     * A matching service has been added to the tracker, we can no compute the factory state.
+     * A matching service has been added to the tracker, we can no compute the factory state. This method is synchronized to avoid concurrent calls to
+     * method modifying the factory state.
      * @param reference : added reference.
      * @see org.apache.felix.ipojo.util.TrackerCustomizer#addedService(org.osgi.framework.ServiceReference)
      */
-    public void addedService(ServiceReference reference) {
+    public synchronized void addedService(ServiceReference reference) {
         if (m_state == INVALID) {
             computeFactoryState();
         }
@@ -292,11 +310,12 @@
 
     /**
      * A used factory disappears.
+     * This method is synchronized to avoid concurrent calls to method modifying the factory state.
      * @param reference : service reference.
      * @param service : factory object.
      * @see org.apache.felix.ipojo.util.TrackerCustomizer#removedService(org.osgi.framework.ServiceReference, java.lang.Object)
      */
-    public void removedService(ServiceReference reference, Object service) {
+    public synchronized void removedService(ServiceReference reference, Object service) {
         // Look for the implied reference and invalid the handler identifier
         for (int i = 0; i < m_requiredHandlers.size(); i++) {
             RequiredHandler req = (RequiredHandler) m_requiredHandlers.get(i);
@@ -324,8 +343,10 @@
      * @return manipulation metadata of this component type.
      */
     public PojoMetadata getPojoMetadata() {
-        if (m_manipulation == null) {
-            m_manipulation = new PojoMetadata(m_componentMetadata);
+        synchronized (this) {
+            if (m_manipulation == null) {
+                m_manipulation = new PojoMetadata(m_componentMetadata);
+            }
         }
         return m_manipulation;
     }

Modified: felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/HandlerFactory.java
URL: http://svn.apache.org/viewvc/felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/HandlerFactory.java?rev=655622&r1=655621&r2=655622&view=diff
==============================================================================
--- felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/HandlerFactory.java (original)
+++ felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/HandlerFactory.java Mon May 12 12:52:58 2008
@@ -19,7 +19,6 @@
 package org.apache.felix.ipojo;
 
 import java.util.Dictionary;
-import java.util.Properties;
 
 import org.apache.felix.ipojo.architecture.ComponentTypeDescription;
 import org.apache.felix.ipojo.metadata.Element;
@@ -41,24 +40,23 @@
 
     /**
      * Handler type (composite|primitive).
-     * Default: handler.
      */
-    private String m_type = "primitive";
+    private final String m_type;
 
     /**
-     * Default iPOJO Namespace.
+     * iPOJO Handler Namespace.
+     * (Set the the iPOJO default namespace is not specified)
      */
-    private String m_namespace = IPOJO_NAMESPACE;
+    private final String m_namespace;
 
     /**
-     * Get the handler start level.
-     * Lower level are priority are configured and started before higher level, and are stopped after.
-     * 
+     * Handler start level.
+     * Lower level are priority are configured and started before higher level, and are stopped after. 
      */
-    private int m_level = Integer.MAX_VALUE;
+    private final int m_level;
 
     /**
-     * Create a composite factory.
+     * Creates a handler factory.
      * @param context : bundle context
      * @param metadata : metadata of the component to create
      * @throws ConfigurationException occurs when the element describing the factory is malformed.
@@ -74,17 +72,23 @@
         String type = metadata.getAttribute("type");
         if (type != null) {
             m_type = type;
+        } else {
+            m_type = "primitive"; // Set to primitive if not specified.
         }
 
         String level = metadata.getAttribute("level");
         if (level != null) {
             m_level = new Integer(level).intValue();
+        } else {
+            m_level = Integer.MAX_VALUE; // Set to max if not specified.
         }
 
         // Get the namespace
         String namespace = metadata.getAttribute("namespace");
         if (namespace != null) {
             m_namespace = namespace.toLowerCase();
+        } else {
+            m_namespace = IPOJO_NAMESPACE; // Set to the iPOJO default namespace if not specified.
         }
     }
 
@@ -109,11 +113,12 @@
     }
 
     /**
-     * Stop the factory.
+     * Stops the factory.
      * This method does not disposed created instances.
      * These instances will be disposed by the instance managers.
+     * This method is called with the lock.
      */
-    public synchronized void stopping() {
+    public void stopping() {
         if (m_tracker != null) {
             m_tracker.close();
             m_tracker = null;
@@ -121,20 +126,8 @@
     }
 
     /**
-     * Compute factory service properties.
-     * This method add three mandatory handler factory properties (name, namespace and type)
-     * @return the properties.
-     * @see org.apache.felix.ipojo.ComponentFactory#getProperties()
-     */
-    protected Properties getProperties() {
-        Properties props = new Properties();
-
-        return props;
-    }
-
-    /**
-     * Create an instance. The given configuration needs to contain the 'name'
-     * property.
+     * Creates an instance. The given configuration needs to contain the 'name'
+     * property. This method is called when holding the lock.
      * @param configuration : configuration of the created instance.
      * @param context : the service context to push for this instance.
      * @param handlers : handler array to used.

Modified: felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/HandlerManager.java
URL: http://svn.apache.org/viewvc/felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/HandlerManager.java?rev=655622&r1=655621&r2=655622&view=diff
==============================================================================
--- felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/HandlerManager.java (original)
+++ felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/HandlerManager.java Mon May 12 12:52:58 2008
@@ -18,7 +18,9 @@
  */
 package org.apache.felix.ipojo;
 
+import java.util.ArrayList;
 import java.util.Dictionary;
+import java.util.List;
 
 import org.apache.felix.ipojo.metadata.Element;
 import org.osgi.framework.BundleContext;
@@ -32,6 +34,7 @@
 
     /**
      * Handler object (contained).
+     * Immutable once set.
      */
     private Handler m_handler;
 
@@ -74,6 +77,7 @@
     /**
      * Create the handler object.
      * This method does nothing if the object is already created.
+     * This method does not need locking protocol as only one thread (the creator thread) can create an instance.
      */
     private void createHandlerObject() {
         if (m_handler != null) { return; }
@@ -83,14 +87,20 @@
     /**
      * Start the instance manager.
      */
-    public synchronized void start() {
-        if (m_state != STOPPED) { return; } // Instance already started
+    public void start() {
+        synchronized (this) {
+            if (m_state != STOPPED) { 
+                return; // Instance already started
+            } else { 
+                m_state = -2; // Temporary starting state, avoiding concurrent starts.
+            }
+        }
 
         for (int i = 0; i < m_handlers.length; i++) {
             m_handlers[i].addInstanceStateListener(this);
             m_handlers[i].start();
         }
-
+        
         m_handler.start(); // Call the handler start method.
 
         for (int i = 0; i < m_handlers.length; i++) {
@@ -104,13 +114,21 @@
         } else {
             setState(INVALID);
         }
+        
+        // Now, the state is necessary different from the temporary state.
     }
 
     /**
      * Stop the instance manager.
      */
-    public synchronized void stop() {
-        if (m_state == STOPPED) { return; } // Instance already stopped
+    public void stop() {
+        synchronized (this) {
+            if (m_state == STOPPED) { 
+                return; // Instance already stopped
+            } else {
+                m_state = -2; // Temporary state avoiding concurrent stopping. 
+            }
+        }
 
         setState(INVALID);
 
@@ -124,10 +142,17 @@
             m_handlers[i].stop();
         }
 
-        m_state = STOPPED;
-        if (m_listeners != null) {
-            for (int i = 0; i < m_listeners.size(); i++) {
-                ((InstanceStateListener) m_listeners.get(i)).stateChanged(this, STOPPED);
+        List listeners = null;
+        synchronized (this) {
+            m_state = STOPPED;
+            if (m_listeners != null) {
+                listeners = new ArrayList(m_listeners); // Stack confinement.
+            }
+        }
+        
+        if (listeners != null) {
+            for (int i = 0; i < listeners.size(); i++) {
+                ((InstanceStateListener) listeners.get(i)).stateChanged(this, STOPPED);
             }
         }
     }
@@ -136,7 +161,7 @@
      * Dispose the instance.
      * @see org.apache.felix.ipojo.ComponentInstance#dispose()
      */
-    public synchronized void dispose() {
+    public void dispose() {
         super.dispose();
         m_handler = null;
     }
@@ -157,16 +182,22 @@
      * @param newState : new state
      * @see org.apache.felix.ipojo.InstanceStateListener#stateChanged(org.apache.felix.ipojo.ComponentInstance, int)
      */
-    public synchronized void stateChanged(ComponentInstance instance, int newState) {
-        if (m_state <= STOPPED) { return; }
-
+    public void stateChanged(ComponentInstance instance, int newState) {
+        int state;
+        synchronized (this) {
+            if (m_state <= STOPPED) { 
+                return;
+            } else {
+                state = m_state; // Stack confinement
+            }
+        }
         // Update the component state if necessary
-        if (newState == INVALID && m_state == VALID) {
+        if (newState == INVALID && state == VALID) {
             // Need to update the state to UNRESOLVED
             setState(INVALID);
             return;
         }
-        if (newState == VALID && m_state == INVALID) {
+        if (newState == VALID && state == INVALID) {
             // An handler becomes valid => check if all handlers are valid
             if (!m_handler.getValidity()) { return; }
             for (int i = 0; i < m_handlers.length; i++) {

Modified: felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/IPojoFactory.java
URL: http://svn.apache.org/viewvc/felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/IPojoFactory.java?rev=655622&r1=655621&r2=655622&view=diff
==============================================================================
--- felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/IPojoFactory.java (original)
+++ felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/IPojoFactory.java Mon May 12 12:52:58 2008
@@ -21,6 +21,7 @@
 import java.util.ArrayList;
 import java.util.Dictionary;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
@@ -40,6 +41,10 @@
  * @author <a href="mailto:dev@felix.apache.org">Felix Project Team</a>
  */
 public abstract class IPojoFactory implements Factory, ManagedServiceFactory {
+    /*
+     * TODO there is potentially an issue when calling FactoryStateListener callbacks with the lock
+     * It should be called by a separate thread dispatching events to listeners.
+     */
 
     /**
      * List of the managed instance name. This list is shared by all factories.
@@ -54,20 +59,21 @@
     /**
      * List of the managed instance managers. The key of this map is the name (i.e. instance names) of the created instance
      */
-    protected Map m_componentInstances = new HashMap();
+    protected final Map m_componentInstances = new HashMap();
 
     /**
      * Component Type provided by this factory.
      */
-    protected Element m_componentMetadata;
+    protected final Element m_componentMetadata;
 
     /**
      * The bundle context reference.
      */
-    protected BundleContext m_context = null;
+    protected final BundleContext m_context;
 
     /**
      * Factory Name. Could be the component class name if the factory name is not set.
+     * Immutable once set.
      */
     protected String m_factoryName;
 
@@ -79,17 +85,17 @@
     /**
      * List of listeners.
      */
-    protected List m_listeners = new ArrayList(2);
+    protected List m_listeners = new ArrayList(1);
 
     /**
      * Logger for the factory (and all component instance).
      */
-    protected Logger m_logger;
+    protected final Logger m_logger;
 
     /**
      * Is the factory public (expose as a service).
      */
-    protected boolean m_isPublic;
+    protected final boolean m_isPublic;
 
     /**
      * Service Registration of this factory (Factory & ManagedServiceFactory).
@@ -124,7 +130,7 @@
         String fac = metadata.getAttribute("factory");
         m_isPublic = fac == null || !fac.equalsIgnoreCase("false");
         m_logger = new Logger(m_context, m_factoryName);
-        m_requiredHandlers = getRequiredHandlerList();
+        m_requiredHandlers = getRequiredHandlerList(); // Call sub-class to get the list of required handlers.
     }
 
     public ComponentTypeDescription getComponentTypeDescription() {
@@ -132,18 +138,18 @@
     }
 
     /**
-     * Add a factory listener.
+     * Adds a factory listener.
      * @param listener : the factory listener to add.
      * @see org.apache.felix.ipojo.Factory#addFactoryStateListener(org.apache.felix.ipojo.FactoryStateListener)
      */
     public void addFactoryStateListener(FactoryStateListener listener) {
-        synchronized (m_listeners) {
+        synchronized (this) {
             m_listeners.add(listener);
         }
     }
 
     /**
-     * Get the logger used by instances of he current factory.
+     * Gets the logger used by instances of he current factory.
      * @return the factory logger.
      */
     public Logger getLogger() {
@@ -151,19 +157,20 @@
     }
 
     /**
-     * Compute the factory name.
+     * Computes the factory name.
      * @return the factory name.
      */
     public abstract String getFactoryName();
 
     /**
-     * Compute the required handler list.
+     * Computes the required handler list.
      * @return the required handler list
      */
     public abstract List getRequiredHandlerList();
 
     /**
-     * Create an instance.
+     * Creates an instance.
+     * This method is called with the lock.
      * @param config : instance configuration
      * @param context : ipojo context to use
      * @param handlers : handler array to use
@@ -174,7 +181,7 @@
             throws ConfigurationException;
 
     /**
-     * Create an instance. The given configuration needs to contain the 'name' property.
+     * Creates an instance. The given configuration needs to contain the 'name' property.
      * @param configuration : configuration of the created instance.
      * @return the created component instance.
      * @throws UnacceptableConfiguration : occurs if the given configuration is not consistent with the component type of this factory.
@@ -188,7 +195,9 @@
     }
 
     /**
-     * Create an instance. The given configuration needs to contain the 'name' property.
+     * Creates an instance. The given configuration needs to contain the 'name' property.
+     * This method is synchronized to assert the validity of the factory during the creation.
+     * Callbacks to sub-class and to create instance need to be aware that they are holding the lock.
      * @param configuration : configuration of the created instance.
      * @param serviceContext : the service context to push for this instance.
      * @return the created component instance.
@@ -197,25 +206,29 @@
      * @throws org.apache.felix.ipojo.ConfigurationException : when the instance configuration failed.
      * @see org.apache.felix.ipojo.Factory#createComponentInstance(java.util.Dictionary)
      */
-    public ComponentInstance createComponentInstance(Dictionary configuration, ServiceContext serviceContext) throws UnacceptableConfiguration, // NOPMD
+    public synchronized ComponentInstance createComponentInstance(Dictionary configuration, ServiceContext serviceContext) throws UnacceptableConfiguration, // NOPMD
             MissingHandlerException, ConfigurationException {
         if (configuration == null) {
             configuration = new Properties();
         }
+        
+        IPojoContext context = null;
+        if (serviceContext == null) {
+            context = new IPojoContext(m_context);
+        } else {
+            context = new IPojoContext(m_context, serviceContext);
+        }
 
         try {
             checkAcceptability(configuration);
         } catch (UnacceptableConfiguration e) {
             m_logger.log(Logger.ERROR, "The configuration is not acceptable : " + e.getMessage());
             throw new UnacceptableConfiguration("The configuration "
-                    + configuration
-                    + " is not acceptable for "
-                    + m_factoryName
-                    + ": "
-                    + e.getMessage());
+                    + configuration + " is not acceptable for " + m_factoryName
+                    + ": " + e.getMessage());
         }
 
-        String name = null;
+        String name;
         if (configuration.get("name") == null) {
             name = generateName();
             configuration.put("name", name);
@@ -226,21 +239,15 @@
                 throw new UnacceptableConfiguration("Name already used : " + name);
             }
         }
-
-        IPojoContext context = null;
-        if (serviceContext == null) {
-            context = new IPojoContext(m_context);
-        } else {
-            context = new IPojoContext(m_context, serviceContext);
-        }
-
+        // Here we are sure to be valid until the end of the method.
         HandlerManager[] handlers = new HandlerManager[m_requiredHandlers.size()];
         for (int i = 0; i < handlers.length; i++) {
             RequiredHandler req = (RequiredHandler) m_requiredHandlers.get(i);
             handlers[i] = getHandler(req, serviceContext);
         }
+
         try {
-            ComponentInstance instance = createInstance(configuration, context, handlers);
+            ComponentInstance instance = createInstance(configuration, context, handlers); // This method is called with the lock.
             m_instancesName.add(name);
             m_componentInstances.put(name, instance);
             return instance;
@@ -255,26 +262,27 @@
     }
 
     /**
-     * Get the factory class name.
-     * @return the factory classname.
+     * Gets the factory class name.
+     * @return the factory class name.
      * @see org.apache.felix.ipojo.Factory#getClassName()
      */
     public abstract String getClassName();
 
     /**
-     * Get the component type description.
+     * Gets the component type description.
      * @return the component type description object. Null if not already computed.
      */
-    public ComponentTypeDescription getComponentDescription() {
+    public synchronized ComponentTypeDescription getComponentDescription() {
         return m_componentDesc;
     }
 
     /**
-     * Get the component type description (Element-Attribute form).
+     * Gets the component type description (Element-Attribute form).
      * @return the component type description.
      * @see org.apache.felix.ipojo.Factory#getDescription()
      */
-    public Element getDescription() {
+    public synchronized Element getDescription() {
+        // Can be null, if not already computed.
         if (m_componentDesc == null) {
             return new Element("No description available for " + m_factoryName, "");
         }
@@ -282,7 +290,7 @@
     }
 
     /**
-     * Compute the list of missing handlers.
+     * Computes the list of missing handlers. This method is called with the lock.
      * @return list of missing handlers.
      * @see org.apache.felix.ipojo.Factory#getMissingHandlers()
      */
@@ -297,16 +305,24 @@
         return list;
     }
 
+    /**
+     * Gets the factory name.
+     * This name is immutable once set.
+     * @return the factory name.
+     * @see org.apache.felix.ipojo.Factory#getName()
+     */
     public String getName() {
         return m_factoryName;
     }
 
     /**
-     * Get the list of required handlers.
+     * Gets the list of required handlers.
+     * This method is synchronized to avoid the concurrent modification
+     * of the required handlers.
      * @return list of required handlers.
      * @see org.apache.felix.ipojo.Factory#getRequiredHandlers()
      */
-    public List getRequiredHandlers() {
+    public synchronized List getRequiredHandlers() {
         List list = new ArrayList();
         for (int i = 0; i < m_requiredHandlers.size(); i++) {
             RequiredHandler req = (RequiredHandler) m_requiredHandlers.get(i);
@@ -315,12 +331,18 @@
         return list;
     }
 
-    public int getState() {
+    /**
+     * Gets the actual factory state.
+     * Must be synchronized as this state is dependent of handler availability.
+     * @return the actual factory state.
+     * @see org.apache.felix.ipojo.Factory#getState()
+     */
+    public synchronized int getState() {
         return m_state;
     }
 
     /**
-     * Check if the configuration is acceptable.
+     * Checks if the configuration is acceptable.
      * @param conf : the configuration to test.
      * @return true if the configuration is acceptable.
      * @see org.apache.felix.ipojo.Factory#isAcceptable(java.util.Dictionary)
@@ -337,27 +359,29 @@
     }
 
     /**
-     * Check if the configuration is acceptable.
+     * Checks if the configuration is acceptable.
      * @param conf : the configuration to test.
      * @throws UnacceptableConfiguration occurs if the configuration is unacceptable.
      * @throws MissingHandlerException occurs if an handler is missing.
      */
     public void checkAcceptability(Dictionary conf) throws UnacceptableConfiguration, MissingHandlerException {
-        if (m_state == Factory.INVALID) {
-            throw new MissingHandlerException(getMissingHandlers());
+        PropertyDescription[] props;
+        synchronized (this) {
+            if (m_state == Factory.INVALID) {
+                throw new MissingHandlerException(getMissingHandlers());
+            }
+            props = m_componentDesc.getProperties(); // Stack confinement.
+            // The property list is up to date, as the factory is valid.
         }
 
         // Check that the configuration does not override immutable properties.
-        PropertyDescription[] props = m_componentDesc.getProperties();
+        
         for (int i = 0; i < props.length; i++) {
             // Is the property immutable
             if (props[i].isImmutable() && conf.get(props[i].getName()) != null) {
-                throw new UnacceptableConfiguration("The property " + props[i] + " cannot be overide : immutable property"); // The instance
-                // configuration try
-                // to override an
-                // immutable property.
+                throw new UnacceptableConfiguration("The property " + props[i] + " cannot be overide : immutable property"); // The instance configuration tries to override an immutable property.
             }
-            // Is the property required.
+            // Is the property required ?
             if (props[i].getValue() == null && conf.get(props[i].getName()) == null) {
                 throw new UnacceptableConfiguration("The property " + props[i].getName() + " is missing"); // The property must be set.
             }
@@ -365,7 +389,8 @@
     }
 
     /**
-     * Reconfigure an existing instance.
+     * Reconfigures an existing instance.
+     * This method is synchronized to assert the validity of the factory during the reconfiguration.
      * @param properties : the new configuration to push.
      * @throws UnacceptableConfiguration : occurs if the new configuration is not consistent with the component type.
      * @throws MissingHandlerException : occurs if the current factory is not valid.
@@ -376,55 +401,63 @@
             throw new UnacceptableConfiguration("The configuration does not contains the \"name\" property");
         }
         String name = (String) properties.get("name");
+        
         ComponentInstance instance = (ComponentInstance) m_componentInstances.get(name);
-
-        if (instance == null) {
-            return; // The instance does not exist.
-        } else {
-            checkAcceptability(properties); // Test if the configuration is acceptable
-            instance.reconfigure(properties); // re-configure the component
+        if (instance == null) { // The instance does not exists.
+            return;
         }
+        
+        checkAcceptability(properties); // Test if the configuration is acceptable
+        instance.reconfigure(properties); // re-configure the instance
     }
 
     /**
-     * Remove a factory listener.
+     * Removes a factory listener.
      * @param listener : the factory listener to remove.
      * @see org.apache.felix.ipojo.Factory#removeFactoryStateListener(org.apache.felix.ipojo.FactoryStateListener)
      */
     public void removeFactoryStateListener(FactoryStateListener listener) {
-        synchronized (m_listeners) {
+        synchronized (this) {
             m_listeners.remove(listener);
         }
     }
 
     /**
      * Stopping method. This method is call when the factory is stopping.
+     * This method is called when holding the lock on the factory.
      */
     public abstract void stopping();
 
     /**
-     * Stop all the instance managers.
+     * Stops all the instance managers.
      */
     public synchronized void stop() {
+        ComponentInstance[] instances;
         if (m_sr != null) {
             m_sr.unregister();
             m_sr = null;
         }
+        stopping(); // Method called when holding the lock.
+        m_state = INVALID; // Set here to avoid to create instances during the stops.
 
-        stopping();
+        Set col = m_componentInstances.keySet();
+        Iterator it = col.iterator();
+        instances = new ComponentInstance[col.size()]; // Stack confinement
+        int index = 0;
+        while (it.hasNext()) {
+            instances[index] = (ComponentInstance) (m_componentInstances.get(it.next()));
+            index++;
+        }
 
         if (m_state == VALID) {
             for (int i = 0; i < m_listeners.size(); i++) {
                 ((FactoryStateListener) m_listeners.get(i)).stateChanged(this, INVALID);
             }
         }
-        m_state = INVALID;
 
         // Dispose created instances.
-        Set col = m_componentInstances.keySet();
-        String[] keys = (String[]) col.toArray(new String[col.size()]);
-        for (int i = 0; i < keys.length; i++) {
-            ComponentInstance instance = (ComponentInstance) m_componentInstances.get(keys[i]);
+        for (int i = 0; i < instances.length; i++) {
+            ComponentInstance instance = instances[i];
             if (instance.getState() != ComponentInstance.DISPOSED) {
                 instance.dispose();
             }
@@ -434,33 +467,27 @@
         for (int i = 0; i < m_requiredHandlers.size(); i++) {
             ((RequiredHandler) m_requiredHandlers.get(i)).unRef();
         }
-
         m_described = false;
         m_componentDesc = null;
         m_componentInstances.clear();
     }
 
     /**
-     * Destroy the factory. The factory cannot be restarted.
-     * Only the extender can call this method.
+     * Destroys the factory. The factory cannot be restarted. Only the extender can call this method.
      */
-    void dispose() {
-        stop();
-        m_componentMetadata = null;
-        m_componentInstances = null;
-        m_context = null;
+    synchronized void dispose() {
+        stop(); // Does not hold the lock.
         m_requiredHandlers = null;
         m_listeners = null;
-        m_logger = null;
     }
 
     /**
-     * Starting method. This method is called when the factory is starting.
+     * Starting method. This method is called when the factory is starting. This method is called when holding the lock on the factory.
      */
     public abstract void starting();
 
     /**
-     * Start the factory.
+     * Starts the factory.
      */
     public synchronized void start() {
         if (m_described) { // Already started.
@@ -482,17 +509,22 @@
     }
 
     /**
-     * Create of update an instance.
+     * Creates or updates an instance.
      * @param name : name of the instance
      * @param properties : configuration of the instance
      * @throws org.osgi.service.cm.ConfigurationException : if the configuration is not consistent for this component type
      * @see org.osgi.service.cm.ManagedServiceFactory#updated(java.lang.String, java.util.Dictionary)
      */
-    public synchronized void updated(String name, Dictionary properties) throws org.osgi.service.cm.ConfigurationException {
-        InstanceManager instance = (InstanceManager) m_componentInstances.get(name);
+    public void updated(String name, Dictionary properties) throws org.osgi.service.cm.ConfigurationException {
+        InstanceManager instance;
+        synchronized (this) {
+            instance = (InstanceManager) m_componentInstances.get(name);
+        }
+        
         if (instance == null) {
             try {
                 properties.put("name", name); // Add the name in the configuration
+                // If an instance with this name was created before, this creation will failed.
                 createComponentInstance(properties);
             } catch (UnacceptableConfiguration e) {
                 m_logger.log(Logger.ERROR, "The configuration is not acceptable : " + e.getMessage());
@@ -519,7 +551,7 @@
     }
 
     /**
-     * Delete an instance.
+     * Deletes an instance.
      * @param name : name of the instance to delete
      * @see org.osgi.service.cm.ManagedServiceFactory#deleted(java.lang.String)
      */
@@ -537,12 +569,15 @@
      */
     public void disposed(ComponentInstance instance) {
         String name = instance.getInstanceName();
-        m_instancesName.remove(name);
-        m_componentInstances.remove(name);
+        synchronized (this) {
+            m_instancesName.remove(name);
+            m_componentInstances.remove(name);
+        }
     }
 
     /**
-     * Compute the component type description. The factory must be valid when calling this method.
+     * Computes the component type description. The factory must be valid when calling this method.
+     * This method is called with the lock.
      */
     protected void computeDescription() {
         for (int i = 0; i < m_requiredHandlers.size(); i++) {
@@ -562,7 +597,8 @@
     }
 
     /**
-     * Compute factory state.
+     * Computes factory state.
+     * This method is call when holding the lock on the current factory.
      */
     protected void computeFactoryState() {
         boolean isValid = true;
@@ -624,7 +660,8 @@
     }
 
     /**
-     * Check if the given handler identifier and the service reference can match.
+     * Checks if the given handler identifier and the service reference can match.
+     * Does not need to be synchronized as the method does not use any fields.
      * @param req : the handler identifier.
      * @param ref : the service reference.
      * @return true if the service reference can fulfill the handler requirement
@@ -639,7 +676,8 @@
     }
 
     /**
-     * Return the handler object for the given required handler. The handler is instantiated in the given service context.
+     * Returns the handler object for the given required handler. The handler is instantiated in the given service context.
+     * This method is called with the lock.
      * @param req : handler to create.
      * @param context : service context in which create the handler (instance context).
      * @return the Handler object.
@@ -670,9 +708,10 @@
 
     /**
      * Helping method generating a new unique name.
+     * This method is call when holding the lock to assert generated name unicity.
      * @return an non already used name
      */
-    protected synchronized String generateName() {
+    protected String generateName() {
         String name = m_factoryName + "-" + m_index;
         while (m_instancesName.contains(name)) {
             m_index = m_index + 1;
@@ -683,6 +722,8 @@
 
     /**
      * Structure storing required handlers.
+     * Access to this class must mostly be with the lock on the factory.
+     * (except to access final fields)
      */
     protected class RequiredHandler implements Comparable {
         /**
@@ -693,7 +734,7 @@
         /**
          * Handler name.
          */
-        private String m_name;
+        private final String m_name;
 
         /**
          * Handler start level.
@@ -703,7 +744,7 @@
         /**
          * Handler namespace.
          */
-        private String m_namespace;
+        private final String m_namespace;
 
         /**
          * Service Reference of the handler factory.
@@ -741,7 +782,8 @@
         }
 
         /**
-         * Get the factory object used for this handler. The object is get when used for the first time.
+         * Gets the factory object used for this handler. The object is get when used for the first time.
+         * This method is called with the lock avoiding concurrent modification and on a valid factory.
          * @return the factory object.
          */
         public HandlerFactory getFactory() {
@@ -784,10 +826,10 @@
 
         /**
          * Release the reference of the used factory.
+         * This method is called with the lock on the current factory.
          */
         public void unRef() {
             if (m_reference != null) {
-                // m_context.ungetService(m_reference); // Will be unget automatically
                 m_factory = null;
                 m_reference = null;
             }
@@ -795,6 +837,7 @@
 
         /**
          * Set the service reference. If the new service reference is null, it unget the used factory (if already get).
+         * This method is called with the lock on the current factory.
          * @param ref : new service reference.
          */
         public void setReference(ServiceReference ref) {
@@ -807,6 +850,7 @@
 
         /**
          * Start level Comparison. This method is used to sort the handler array.
+         * This method is called with the lock.
          * @param object : object on which compare.
          * @return -1, 0, +1 according to the comparison of their start level.
          * @see java.lang.Comparable#compareTo(java.lang.Object)

Modified: felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/InstanceManager.java
URL: http://svn.apache.org/viewvc/felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/InstanceManager.java?rev=655622&r1=655621&r2=655622&view=diff
==============================================================================
--- felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/InstanceManager.java (original)
+++ felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/InstanceManager.java Mon May 12 12:52:58 2008
@@ -54,7 +54,7 @@
     /**
      * Handler list.
      */
-    protected HandlerManager[] m_handlers = null;
+    protected final HandlerManager[] m_handlers;
 
     /**
      * Component state (STOPPED at the beginning).
@@ -69,25 +69,28 @@
     /**
      * Parent factory (ComponentFactory).
      */
-    private ComponentFactory m_factory;
+    private final ComponentFactory m_factory;
 
     /**
      * The context of the component.
      */
-    private BundleContext m_context;
+    private final BundleContext m_context;
 
     /**
      * Map [field, field interceptor list] storing handlers interested by the field.
+     * Once configured, this map can't change.
      */
     private Map m_fieldRegistration;
 
     /**
      * Map [method identifier, method interceptor list] storing handlers interested by the method.
+     * Once configure this map can't change.
      */
     private Map m_methodRegistration;
 
     /**
      * Manipulated class.
+     * Once set, this field doesn't change.
      */
     private Class m_clazz;
 
@@ -98,6 +101,7 @@
 
     /**
      * Factory method. Contains the name of the static method used to create POJO objects.
+     * Once set, this field is immutable.
      */
     private String m_factoryMethod = null;
 
@@ -164,12 +168,14 @@
         InstanceDescription desc =
                 new InstanceDescription(m_name, componentState, getContext().getBundle().getBundleId(), m_factory.getComponentDescription());
 
-        if (m_pojoObjects != null) {
-            String[] objects = new String[m_pojoObjects.size()];
-            for (int i = 0; i < m_pojoObjects.size(); i++) {
-                objects[i] = m_pojoObjects.get(i).toString();
+        synchronized (this) { // Must be synchronized, it access to the m_pojoObjects list.
+            if (m_pojoObjects != null) {
+                String[] objects = new String[m_pojoObjects.size()];
+                for (int i = 0; i < m_pojoObjects.size(); i++) {
+                    objects[i] = m_pojoObjects.get(i).toString();
+                }
+                desc.setCreatedObjects(objects);
             }
-            desc.setCreatedObjects(objects);
         }
 
         Handler[] handlers = getRegistredHandlers();
@@ -181,6 +187,7 @@
 
     /**
      * Get the list of handlers plugged on the instance.
+     * This method does not need a synchronized block as the handler set is constant.
      * @return the handler array of plugged handlers.
      */
     public Handler[] getRegistredHandlers() {
@@ -193,6 +200,7 @@
 
     /**
      * Return a specified handler.
+     * This must does not need a synchronized block as the handler set is constant.
      * @param name : class name of the handler to find or its qualified name (namespace:name)
      * @return : the handler, or null if not found
      */
@@ -262,11 +270,15 @@
     /**
      * Start the instance manager.
      */
-    public synchronized void start() {
-        if (m_state != STOPPED) { // Instance already started
-            return;
+    public void start() {
+        synchronized (this) {
+            if (m_state != STOPPED) { // Instance already started
+                return;
+            } else {
+                m_state = -2; // Temporary state.
+            }
         }
-
+        
         for (int i = 0; i < m_handlers.length; i++) {
             m_handlers[i].addInstanceStateListener(this);
             try {
@@ -277,13 +289,12 @@
                 throw e;
             }
         }
-
+        
         for (int i = 0; i < m_handlers.length; i++) {
             if (m_handlers[i].getState() != VALID) {
                 setState(INVALID);
                 return;
             }
-
         }
         setState(VALID);
     }
@@ -291,26 +302,35 @@
     /**
      * Stop the instance manager.
      */
-    public synchronized void stop() {
-        if (m_state == STOPPED) {
-            return;
-        } // Instance already stopped
-
-        setState(INVALID);
-
-        m_state = STOPPED;
+    public void stop() {
+        List listeners = null;
+        synchronized (this) {
+            if (m_state == STOPPED) { // Instance already stopped
+                return;
+            } 
+            m_stateQueue.clear();
+            m_inTransition = false;
+        }
+        
+        setState(INVALID); // Must be called outside a synchronized block.
 
         // Stop all the handlers
         for (int i = m_handlers.length - 1; i > -1; i--) {
             m_handlers[i].removeInstanceStateListener(this);
             m_handlers[i].stop();
         }
+        
+        synchronized (this) {
+            m_state = STOPPED;
+            if (m_listeners != null) {
+                listeners = new ArrayList(m_listeners); // Stack confinement
+            }
+            m_pojoObjects = null;
+        }
 
-        m_pojoObjects = null;
-
-        if (m_listeners != null) {
-            for (int i = 0; i < m_listeners.size(); i++) {
-                ((InstanceStateListener) m_listeners.get(i)).stateChanged(this, STOPPED);
+        if (listeners != null) {
+            for (int i = 0; i < listeners.size(); i++) {
+                ((InstanceStateListener) listeners.get(i)).stateChanged(this, STOPPED);
             }
         }
     }
@@ -319,29 +339,40 @@
      * Dispose the instance.
      * @see org.apache.felix.ipojo.ComponentInstance#dispose()
      */
-    public synchronized void dispose() {
-        if (m_state > STOPPED) { // Valid or invalid
-            stop();
+    public void dispose() {
+        List listeners = null;
+        int state = -2;
+        synchronized (this) {
+            state = m_state; // Stack confinement
+            if (m_listeners != null) {
+                listeners = new ArrayList(m_listeners); // Stack confinement
+            }
+            m_listeners = null;
+        }
+        
+        if (state > STOPPED) { // Valid or invalid
+            stop(); // Does not hold the lock.
+        }
+        
+        synchronized (this) {
+            m_state = DISPOSED;
         }
 
-        m_state = DISPOSED;
-
-        for (int i = 0; m_listeners != null && i < m_listeners.size(); i++) {
-            ((InstanceStateListener) m_listeners.get(i)).stateChanged(this, DISPOSED);
+        for (int i = 0; listeners != null && i < listeners.size(); i++) {
+            ((InstanceStateListener) listeners.get(i)).stateChanged(this, DISPOSED);
         }
-        m_listeners = null;
 
         for (int i = m_handlers.length - 1; i > -1; i--) {
             m_handlers[i].dispose();
         }
 
-        m_handlers = new HandlerManager[0];
-        m_factory.disposed(this);
-        m_fields.clear();
-        m_fieldRegistration = new HashMap();
-        m_methodRegistration = new HashMap();
-        m_clazz = null;
-        m_inTransition = false;
+        synchronized (this) {
+            m_factory.disposed(this);
+            m_fields.clear();
+            m_fieldRegistration = new HashMap();
+            m_methodRegistration = new HashMap();
+            m_clazz = null;
+        }
     }
 
     /**
@@ -350,54 +381,64 @@
      * finished.
      * @param state : the new state
      */
-    public synchronized void setState(int state) {
-        if (m_inTransition) {
-            m_stateQueue.add(new Integer(state));
-            return;
-        }
+    public void setState(int state) {
+        int originalState = -2;
+        List listeners = null;
+        synchronized (this) {
+            if (m_inTransition) {
+                m_stateQueue.add(new Integer(state));
+                return;
+            }
 
-        if (m_state != state) {
-            m_inTransition = true;
+            if (m_state != state) {
+                m_inTransition = true;
+                originalState = m_state; // Stack confinement.
+                m_state = state;
+                if (m_listeners != null) {
+                    listeners = new ArrayList(m_listeners); // Stack confinement.
+                }
+            }
+        }
 
-            if (state > m_state) {
+        // This section can be executed only by one thread at the same time. The m_inTransition pseudo semaphore block access to this section.
+        if (m_inTransition) { // Check that we are really changing.
+            if (state > originalState) {
                 // The state increases (Stopped = > IV, IV => V) => invoke handlers from the higher priority to the lower
-                m_state = state;
                 try {
                     for (int i = 0; i < m_handlers.length; i++) {
                         m_handlers[i].getHandler().stateChanged(state);
                     }
                 } catch (IllegalStateException e) {
                     // When an illegal state exception happens, the instance manager must be stopped immediately.
-                    m_stateQueue.clear();
                     stop();
                     return;
                 }
             } else {
                 // The state decreases (V => IV, IV = > Stopped, Stopped => Disposed)
-                m_state = state;
                 try {
                     for (int i = m_handlers.length - 1; i > -1; i--) {
                         m_handlers[i].getHandler().stateChanged(state);
                     }
                 } catch (IllegalStateException e) {
                     // When an illegal state exception happens, the instance manager must be stopped immediately.
-                    m_stateQueue.clear();
                     stop();
                     return;
                 }
             }
+        }
 
-            if (m_listeners != null) {
-                for (int i = 0; i < m_listeners.size(); i++) {
-                    ((InstanceStateListener) m_listeners.get(i)).stateChanged(this, state);
-                }
+        if (listeners != null) {
+            for (int i = 0; i < listeners.size(); i++) {
+                ((InstanceStateListener) listeners.get(i)).stateChanged(this, state);
             }
         }
 
-        m_inTransition = false;
-        if (!m_stateQueue.isEmpty()) {
-            int newState = ((Integer) (m_stateQueue.remove(0))).intValue();
-            setState(newState);
+        synchronized (this) {
+            m_inTransition = false;
+            if (!m_stateQueue.isEmpty()) {
+                int newState = ((Integer) (m_stateQueue.remove(0))).intValue();
+                setState(newState);
+            }
         }
     }
 
@@ -406,7 +447,7 @@
      * @return the actual state of the component instance.
      * @see org.apache.felix.ipojo.ComponentInstance#getState()
      */
-    public int getState() {
+    public synchronized int getState() {
         return m_state;
     }
 
@@ -415,7 +456,7 @@
      * @return true if the instance is started.
      * @see org.apache.felix.ipojo.ComponentInstance#isStarted()
      */
-    public boolean isStarted() {
+    public synchronized boolean isStarted() {
         return m_state > STOPPED;
     }
 
@@ -428,9 +469,7 @@
         if (m_listeners == null) {
             m_listeners = new ArrayList();
         }
-        synchronized (m_listeners) {
-            m_listeners.add(listener);
-        }
+        m_listeners.add(listener);
     }
 
     /**
@@ -440,11 +479,9 @@
      */
     public synchronized void removeInstanceStateListener(InstanceStateListener listener) {
         if (m_listeners != null) {
-            synchronized (m_listeners) {
-                m_listeners.remove(listener);
-                if (m_listeners.isEmpty()) {
-                    m_listeners = null;
-                }
+            m_listeners.remove(listener);
+            if (m_listeners.isEmpty()) {
+                m_listeners = null;
             }
         }
     }
@@ -475,7 +512,7 @@
      * Get the array of object created by the instance.
      * @return the created instance of the component instance.
      */
-    public Object[] getPojoObjects() {
+    public synchronized Object[] getPojoObjects() {
         if (m_pojoObjects == null) {
             return null;
         }
@@ -491,8 +528,8 @@
             load();
         }
 
+        // The following code doesn't need to be synchronized as is deal only with immutable fields.
         Object instance = null;
-
         if (m_factoryMethod == null) {
             // No factory-method, we use the constructor.
             try {
@@ -627,19 +664,21 @@
             }
 
         }
+        
+        
 
-        // Register the new instance in not already present.
-        if (m_pojoObjects == null) {
-            m_pojoObjects = new ArrayList(1);
-        }
-        if (!m_pojoObjects.contains(instance)) {
-            m_pojoObjects.add(instance);
-            // Call createInstance on Handlers :
-            for (int i = 0; i < m_handlers.length; i++) {
-                ((PrimitiveHandler) m_handlers[i].getHandler()).onCreation(instance);
+        // Add the new instance in the instance list.
+        synchronized (this) {
+            if (m_pojoObjects == null) {
+                m_pojoObjects = new ArrayList(1);
             }
+            m_pojoObjects.add(instance);
         }
-
+        // Call createInstance on Handlers :
+        for (int i = 0; i < m_handlers.length; i++) {
+            ((PrimitiveHandler) m_handlers[i].getHandler()).onCreation(instance);
+        }
+        
         return instance;
     }
 
@@ -647,15 +686,25 @@
      * Get the first object created by the instance. If no object created, create and return one object.
      * @return the instance of the component instance to use for singleton component
      */
-    public synchronized Object getPojoObject() {
-        if (m_pojoObjects == null) {
-            return createPojoObject();
+    public Object getPojoObject() {
+        Object pojo = null;
+        synchronized (this) {
+            if (m_pojoObjects != null) {
+                pojo = m_pojoObjects.get(0); // Stack confinement
+            }
+        }
+        
+        if (pojo == null) {
+            return createPojoObject(); // This method must be called without the lock.
+        } else {
+            return pojo;
         }
-        return m_pojoObjects.get(0);
     }
 
     /**
      * Get the manipulated class.
+     * The method does not need to be synchronized.
+     * Reassigning the internal class will use the same class object.
      * @return the manipulated class
      */
     public Class getClazz() {
@@ -743,13 +792,17 @@
      * @param fieldName : the field name on which the GETFIELD instruction is called
      * @return the value decided by the last asked handler (throw a warning if two fields decide two different values)
      */
-    public Object onGet(Object pojo, String fieldName) {
-        Object initialValue = m_fields.get(fieldName);
+    public Object  onGet(Object pojo, String fieldName) {
+        Object initialValue = null;
+        synchronized (this) { // Stack confinement.
+            initialValue = m_fields.get(fieldName);
+        }
         Object result = initialValue;
         // Get the list of registered handlers
 
-        FieldInterceptor[] list = (FieldInterceptor[]) m_fieldRegistration.get(fieldName);
+        FieldInterceptor[] list = (FieldInterceptor[]) m_fieldRegistration.get(fieldName); // Immutable list.
         for (int i = 0; list != null && i < list.length; i++) {
+            // Call onGet outside of a synchronized block.
             Object handlerResult = list[i].onGet(null, fieldName, initialValue);
             if (handlerResult == initialValue) {
                 continue; // Non-binding case (default implementation).
@@ -768,7 +821,10 @@
 
         if ((result != null && !result.equals(initialValue)) || (result == null && initialValue != null)) {
             // A change occurs => notify the change
-            m_fields.put(fieldName, result);
+            synchronized (this) {
+                m_fields.put(fieldName, result);
+            }
+            // Call onset outside of a synchronized block.
             for (int i = 0; list != null && i < list.length; i++) {
                 list[i].onSet(null, fieldName, result);
             }
@@ -784,13 +840,13 @@
      * @param args : argument array
      */
     public void onEntry(Object pojo, String methodId, Object[] args) {
-        if (m_methodRegistration == null) {
+        if (m_methodRegistration == null) { // Immutable field.
             return;
         }
         MethodInterceptor[] list = (MethodInterceptor[]) m_methodRegistration.get(methodId);
         Method method = getMethodById(methodId);
         for (int i = 0; list != null && i < list.length; i++) {
-            list[i].onEntry(pojo, method, args);
+            list[i].onEntry(pojo, method, args); // Outside a synchronized block.
         }
     }
 
@@ -842,6 +898,7 @@
      * @return : the method object or null if the method cannot be found.
      */
     private Method getMethodById(String methodId) {
+        // Not necessary synchronized as recomputing the methodID will give the same Method twice.
         Method method = (Method) m_methods.get(methodId);
         if (method == null) {
             Method[] mets = m_clazz.getDeclaredMethods();
@@ -873,12 +930,17 @@
      * @param objectValue : the value of the field
      */
     public void onSet(Object pojo, String fieldName, Object objectValue) {
-        Object value = m_fields.get(fieldName);
+        Object value = null; // Stack confinement
+        synchronized (this) {
+            value = m_fields.get(fieldName);
+        }
         if ((value != null && !value.equals(objectValue)) || (value == null && objectValue != null)) {
-            m_fields.put(fieldName, objectValue);
+            synchronized (this) {
+                m_fields.put(fieldName, objectValue);
+            }
             FieldInterceptor[] list = (FieldInterceptor[]) m_fieldRegistration.get(fieldName);
             for (int i = 0; list != null && i < list.length; i++) {
-                list[i].onSet(null, fieldName, objectValue);
+                list[i].onSet(null, fieldName, objectValue); // Outside a synchronized block.
             }
         }
     }
@@ -889,15 +951,15 @@
      * @see org.apache.felix.ipojo.ComponentInstance#getContext()
      */
     public BundleContext getContext() {
-        return m_context;
+        return m_context; // Immutable
     }
 
     public BundleContext getGlobalContext() {
-        return ((IPojoContext) m_context).getGlobalContext();
+        return ((IPojoContext) m_context).getGlobalContext(); // Immutable
     }
 
     public ServiceContext getLocalServiceContext() {
-        return ((IPojoContext) m_context).getServiceContext();
+        return ((IPojoContext) m_context).getServiceContext(); // Immutable
     }
 
     /**
@@ -906,7 +968,7 @@
      * @see org.apache.felix.ipojo.ComponentInstance#getInstanceName()
      */
     public String getInstanceName() {
-        return m_name;
+        return m_name; // Immutable
     }
 
     /**
@@ -918,19 +980,24 @@
         for (int i = 0; i < m_handlers.length; i++) {
             m_handlers[i].getHandler().reconfigure(configuration);
         }
-        if (m_state == INVALID) {
-            // Try to revalidate the instance ofter reconfiguration
-            for (int i = 0; i < m_handlers.length; i++) {
-                if (m_handlers[i].getState() != VALID) {
-                    return;
+        // We synchronized the state computation.
+        synchronized (this) {
+            if (m_state == INVALID) {
+                // Try to revalidate the instance after reconfiguration
+                for (int i = 0; i < m_handlers.length; i++) {
+                    if (m_handlers[i].getState() != VALID) {
+                        return;
+                    }
                 }
+                setState(VALID);
             }
-            setState(VALID);
         }
     }
 
     /**
      * Get the implementation class of the component type.
+     * This method does not need to be synchronized as the
+     * class name is constant once set. 
      * @return the class name of the component type.
      */
     public String getClassName() {
@@ -943,18 +1010,23 @@
      * @param newState : new state
      * @see org.apache.felix.ipojo.InstanceStateListener#stateChanged(org.apache.felix.ipojo.ComponentInstance, int)
      */
-    public synchronized void stateChanged(ComponentInstance instance, int newState) {
-        if (m_state <= STOPPED) {
-            return;
+    public void stateChanged(ComponentInstance instance, int newState) {
+        int state;
+        synchronized (this) {
+            if (m_state <= STOPPED) {
+                return;
+            } else {
+                state = m_state; // Stack confinement
+            }
         }
 
         // Update the component state if necessary
-        if (newState == INVALID && m_state == VALID) {
+        if (newState == INVALID && state == VALID) {
             // Need to update the state to UNRESOLVED
             setState(INVALID);
             return;
         }
-        if (newState == VALID && m_state == INVALID) {
+        if (newState == VALID && state == INVALID) {
             // An handler becomes valid => check if all handlers are valid
             for (int i = 0; i < m_handlers.length; i++) {
                 if (m_handlers[i].getState() != VALID) {

Modified: felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/architecture/ComponentTypeDescription.java
URL: http://svn.apache.org/viewvc/felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/architecture/ComponentTypeDescription.java?rev=655622&r1=655621&r2=655622&view=diff
==============================================================================
--- felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/architecture/ComponentTypeDescription.java (original)
+++ felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/architecture/ComponentTypeDescription.java Mon May 12 12:52:58 2008
@@ -47,7 +47,7 @@
     /**
      * Represented factory.
      */
-    private Factory m_factory;
+    private final Factory m_factory;
     
     /**
      * Constructor.

Modified: felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/handlers/configuration/ConfigurationHandler.java
URL: http://svn.apache.org/viewvc/felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/handlers/configuration/ConfigurationHandler.java?rev=655622&r1=655621&r2=655622&view=diff
==============================================================================
--- felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/handlers/configuration/ConfigurationHandler.java (original)
+++ felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/handlers/configuration/ConfigurationHandler.java Mon May 12 12:52:58 2008
@@ -321,7 +321,7 @@
      * @param configuration : the new configuration
      * @see org.apache.felix.ipojo.Handler#reconfigure(java.util.Dictionary)
      */
-    public synchronized void reconfigure(Dictionary configuration) {      
+    public synchronized void reconfigure(Dictionary configuration) {   
         Properties props = reconfigureProperties(configuration);
         propagate(props, m_propagatedFromInstance);
         m_propagatedFromInstance = props;

Modified: felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/handlers/dependency/Dependency.java
URL: http://svn.apache.org/viewvc/felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/handlers/dependency/Dependency.java?rev=655622&r1=655621&r2=655622&view=diff
==============================================================================
--- felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/handlers/dependency/Dependency.java (original)
+++ felix/trunk/ipojo/core/src/main/java/org/apache/felix/ipojo/handlers/dependency/Dependency.java Mon May 12 12:52:58 2008
@@ -393,8 +393,8 @@
      * @return the service object or a nullable / default implementation if defined.
      * @see org.apache.felix.ipojo.FieldInterceptor#onGet(java.lang.Object, java.lang.String, java.lang.Object)
      */
-    public Object onGet(Object pojo, String fieldName, Object value) {
-     // Initialize the thread local object is not already touched.
+    public Object onGet(Object pojo, String fieldName, Object value) {        
+     //     Initialize the thread local object is not already touched.
         Usage usage = (Usage) m_usage.get();
         if (usage.m_stack == 0) { // uninitialized usage.
             ServiceReference[] refs = super.getServiceReferences();
@@ -441,7 +441,7 @@
      * @param value : set value.
      * @see org.apache.felix.ipojo.FieldInterceptor#onSet(java.lang.Object, java.lang.String, java.lang.Object)
      */
-    public void onSet(Object pojo, String fieldName, Object value) {
+    public void onSet(Object pojo, String fieldName, Object value) {        
         // Nothing to do.
     }
 
@@ -482,8 +482,7 @@
      * @see org.apache.felix.ipojo.MethodInterceptor#onExit(java.lang.Object, java.lang.reflect.Method, java.lang.Object)
      */
     public void onExit(Object pojo, Method method, Object returnedObj) {
-        // Nothing to do  : wait onFinally
-        
+        // Nothing to do  : wait onFinally        
     }
 
     /**



Mime
View raw message