Apply patch from FELIX-3789 - Deadlock due to synchronization on INSTANCE_NAME

We changed how the instance names are checked and computed. Notice that a side effect of this approach is that factories must have unique names. Even if it was the official way, the former implementation was not enforcing this.  

When an instance target a specific version, and if the name is set and already taken, the factory version is appended to the end of the instance name (given_instance_name-factory_version)

Also fixed name extraction issue in handlers for composite.




git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1423321 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/ipojo/runtime/core/src/main/java/org/apache/felix/ipojo/HandlerManagerFactory.java b/ipojo/runtime/core/src/main/java/org/apache/felix/ipojo/HandlerManagerFactory.java
index 8cef70e..cf5994e 100644
--- a/ipojo/runtime/core/src/main/java/org/apache/felix/ipojo/HandlerManagerFactory.java
+++ b/ipojo/runtime/core/src/main/java/org/apache/felix/ipojo/HandlerManagerFactory.java
@@ -109,6 +109,14 @@
         return new HandlerTypeDescription(this);

     }

 

+    public String getFactoryName() {

+        if (m_type != null  && "composite".equals(m_type)  && IPOJO_NAMESPACE.equals(m_namespace)) {

+            // Artificially change the factory name, to avoid name clash when we generate the instance name.

+            return m_namespace + ".composite:" + getName();

+        }

+        return getHandlerName();

+    }

+

     /**

      * Stops the factory.

      * This method does not disposed created instances.

diff --git a/ipojo/runtime/core/src/main/java/org/apache/felix/ipojo/IPojoFactory.java b/ipojo/runtime/core/src/main/java/org/apache/felix/ipojo/IPojoFactory.java
index 3302790..5cf04a9 100644
--- a/ipojo/runtime/core/src/main/java/org/apache/felix/ipojo/IPojoFactory.java
+++ b/ipojo/runtime/core/src/main/java/org/apache/felix/ipojo/IPojoFactory.java
@@ -19,6 +19,7 @@
 package org.apache.felix.ipojo;

 

 import java.util.ArrayList;

+import java.util.Collections;

 import java.util.Dictionary;

 import java.util.HashMap;

 import java.util.Iterator;

@@ -53,11 +54,9 @@
 

     /**

      * The list of the managed instance name.

-     * This list is shared by all factories and is

-     * used to assert name unicity.

-     * All accesses must be protected using the INSTANCE_NAME lock.

+     * This list is shared by all factories and is used to assert name uniqueness.

      */

-    protected static final List INSTANCE_NAME = new ArrayList();

+    protected static final List INSTANCE_NAME = Collections.synchronizedList(new ArrayList());

 

     /**

      * The component type description exposed by the {@link Factory} service.

@@ -132,17 +131,17 @@
     protected int m_state = Factory.INVALID;

 

     /**

-     * The index used to generate instance name if not set.

-     */

-    private long m_index = 0;

-

-    /**

      * The flag indicating if this factory has already a

      * computed description or not.

      */

     private boolean m_described;

 

     /**

+     * Generates a unique instance name if not provided by the configuration.

+     */

+    private NameGenerator m_generator = new UniquenessNameGenerator(new SwitchNameGenerator());

+

+    /**

      * Creates an iPOJO Factory.

      * At the end of this method, the required set of handler is computed.

      * But the result is computed by a sub-class.

@@ -275,49 +274,45 @@
                     + ": " + e.getMessage());

         }

 

-        // The following code is doing a sequence of operation on the INSTANCE_NAME list

-        // So, it needs to be protected against concurrent access.

-        synchronized (INSTANCE_NAME) {

-

-            String name;

-            if (configuration.get("instance.name") == null && configuration.get("name") == null) { // Support both instance.name & name

-                // We're holding the INSTANCE_NAME lock, so no problem.

-                name = generateName();

-                configuration.put("instance.name", name);

-            } else {

-                name = (String) configuration.get("instance.name");

-                if (name == null) {

-                    name = (String) configuration.get("name");

-                    getLogger().log(Logger.WARNING, "The 'name' (" + name + ") attribute, used as the instance name, is deprecated, please use the 'instance.name' attribute");

-                    configuration.put("instance.name", name);

-                }

-

-                // We're holding the INSTANCE_NAME's lock.

-                if (INSTANCE_NAME.contains(name)) {

-                        m_logger.log(Logger.ERROR, "The configuration is not acceptable : Name already used");

-                        throw new UnacceptableConfiguration(getFactoryName() + " : Name already used : " + name);

-                }

-

+        // Find name in the configuration

+        String name;

+        if (configuration.get("instance.name") == null && configuration.get("name") == null) {

+            // No name provided

+            name = null;

+        } else {

+            // Support both instance.name & name

+            name = (String) configuration.get("instance.name");

+            if (name == null) {

+                name = (String) configuration.get("name");

+                getLogger().log(Logger.WARNING, "The 'name' (" + name + ") attribute, used as the instance name, is deprecated, please use the 'instance.name' attribute");

             }

-            // 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); // This method is called with the lock.

-                // We're still in the synchronized block, so we can access the INSTANCE_NAME.

-                INSTANCE_NAME.add(name);

-                m_componentInstances.put(name, instance);

-                m_logger.log(Logger.INFO, "Instance " + name + " from factory " + m_factoryName + " created");

-                return instance;

-            } catch (ConfigurationException e) {

-                m_logger.log(Logger.ERROR, e.getMessage());

-                throw new ConfigurationException(e.getMessage(), m_factoryName);

-            }

-        } // End of the synchronized block on INSTANCE_NAME.

+

+        // Generate a unique name if required and verify uniqueness

+        // We extract the version from the configuration because it may help to compute a unique name by appending

+        // the version to the given name.

+        String version = (String) configuration.get("factory.version");

+        name = m_generator.generate(name, version);

+        configuration.put("instance.name", name);

+

+        // 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);

+            m_componentInstances.put(name, instance);

+            m_logger.log(Logger.INFO, "Instance " + name + " from factory " + m_factoryName + " created");

+            return instance;

+        } catch (ConfigurationException e) {

+            INSTANCE_NAME.remove(name);

+            m_logger.log(Logger.ERROR, e.getMessage());

+            throw new ConfigurationException(e.getMessage(), m_factoryName);

+        }

 

 

     }

@@ -608,6 +603,9 @@
 

         if (m_isPublic) {

             // Exposition of the factory service

+            if (m_componentDesc == null) {

+                System.out.println("Description of " + m_factoryName + " " + m_componentDesc);

+            }

             BundleContext bc = SecurityHelper.selectContextToRegisterServices(m_componentDesc.getFactoryInterfacesToPublish(),

                     m_context, getIPOJOBundleContext());

             m_sr =

@@ -684,9 +682,7 @@
      * @see org.osgi.service.cm.ManagedServiceFactory#deleted(java.lang.String)

      */

     public synchronized void deleted(String name) {

-        synchronized (INSTANCE_NAME) {

-            INSTANCE_NAME.remove(name);

-        }

+        INSTANCE_NAME.remove(name);

         ComponentInstance instance = (ComponentInstance) m_componentInstances.remove(name);

         if (instance != null) {

             instance.dispose();

@@ -702,9 +698,7 @@
         synchronized (this) {

             m_componentInstances.remove(name);

         }

-        synchronized (INSTANCE_NAME) {

-            INSTANCE_NAME.remove(name);

-        }

+        INSTANCE_NAME.remove(name);

     }

 

     /**

@@ -718,16 +712,23 @@
     protected void computeDescription() {

         for (int i = 0; i < m_requiredHandlers.size(); i++) {

             RequiredHandler req = (RequiredHandler) m_requiredHandlers.get(i);

-            Handler handler = getHandler(req, null).getHandler();

-            try {

-                handler.setFactory(this);

-                handler.initializeComponentFactory(m_componentDesc, m_componentMetadata);

-                ((Pojo) handler).getComponentInstance().dispose();

-            } catch (org.apache.felix.ipojo.ConfigurationException e) {

-                ((Pojo) handler).getComponentInstance().dispose();

-                m_logger.log(Logger.ERROR, e.getMessage());

-                stop();

-                throw new IllegalStateException(e.getMessage());

+

+            if (getHandler(req, null) == null) {

+                // TODO this does not really looks good.

+                m_logger.log(Logger.ERROR, "Cannot extract handler object from " + m_factoryName + " " + req

+                        .getFullName());

+            } else {

+                Handler handler = getHandler(req, null).getHandler();

+                try {

+                    handler.setFactory(this);

+                    handler.initializeComponentFactory(m_componentDesc, m_componentMetadata);

+                    ((Pojo) handler).getComponentInstance().dispose();

+                } catch (org.apache.felix.ipojo.ConfigurationException e) {

+                    ((Pojo) handler).getComponentInstance().dispose();

+                    m_logger.log(Logger.ERROR, e.getMessage());

+                    stop();

+                    throw new IllegalStateException(e.getMessage());

+                }

             }

         }

     }

@@ -784,10 +785,7 @@
                     if (instance.getState() != ComponentInstance.DISPOSED) {

                         instance.dispose();

                     }

-                    // Need to enter a synchronized block locking the INSTANCE_NAME class to avoid concurrent accesses.

-                    synchronized (INSTANCE_NAME) {

-                        INSTANCE_NAME.remove(instance.getInstanceName());

-                    }

+                    INSTANCE_NAME.remove(instance.getInstanceName());

                 }

 

                 m_componentInstances.clear();

@@ -850,20 +848,6 @@
     }

 

     /**

-     * Helper method generating a new unique name.

-     * This method is call when holding the lock on INSTANCE_NAME to assert generated name unicity.

-     * @return a non already used name

-     */

-    protected String generateName() {

-        String name = m_factoryName + "-" + m_index;

-        while (INSTANCE_NAME.contains(name)) {

-            m_index = m_index + 1;

-            name = m_factoryName + "-" + m_index;

-        }

-        return name;

-    }

-

-    /**

      * Structure storing required handlers.

      * Access to this class must mostly be with the lock on the factory.

      * (except to access final fields)

@@ -1026,4 +1010,111 @@
         }

     }

 

+    /**

+     * Generate a unique name for a component instance.

+     */

+    private interface NameGenerator {

+

+        /**

+         * @return a unique name.

+         * @param name The user provided name (may be null)

+         * @param version the factory version if set. Instances can specify the version of the factory they are bound

+         *                to. This parameter may be used to avoid name conflicts.

+         */

+        String generate(String name, String version) throws UnacceptableConfiguration;

+    }

+

+    /**

+     * This generator implements the default naming strategy.

+     * The name is composed of the factory name suffixed with a unique number identifier (starting from 0).

+     */

+    private class DefaultNameGenerator implements NameGenerator {

+        private long m_nextId = 0;

+

+        /**

+         * This method has to be synchronized to ensure name uniqueness.

+         * @param name The user provided name (may be null)

+         * @param version ignored.

+         */

+        public synchronized String generate(String name, String version) throws UnacceptableConfiguration

+        {

+            // Note: This method is overridden by handlers to get the full name.

+            return IPojoFactory.this.getFactoryName() + "-" + m_nextId++;

+        }

+    }

+

+    /**

+     * This generator implements the naming strategy when client provides the instance name value.

+     */

+    private class UserProvidedNameGenerator implements NameGenerator {

+

+        /**

+         * @param name The user provided name (not null)

+         * @param  version ignored.

+         */

+        public String generate(String name, String version) throws UnacceptableConfiguration

+        {

+            return name;

+        }

+    }

+

+    /**

+     * This generator ensure that the returned name is globally unique.

+     */

+    private class UniquenessNameGenerator implements NameGenerator {

+

+        private NameGenerator delegate;

+

+        private UniquenessNameGenerator(NameGenerator delegate)

+        {

+            this.delegate = delegate;

+        }

+

+        /**

+         * This method has to be synchronized to ensure name uniqueness.

+         * @param name The user provided name (may be null)

+         */

+        public String generate(String name, String version) throws UnacceptableConfiguration

+        {

+            // Produce the name

+            String name2 = delegate.generate(name, version);

+

+            // Needs to be in a synchronized block because we want to ensure that the name is unique

+            synchronized (INSTANCE_NAME) {

+                // Verify uniqueness

+                if (INSTANCE_NAME.contains(name2)  && version != null) {

+                    // Named already used, try to append the version.

+                    name2 = name2 + "-" + version;

+                    if (INSTANCE_NAME.contains(name2)) {

+                        m_logger.log(Logger.ERROR, "The configuration is not acceptable : Name already used");

+                        throw new UnacceptableConfiguration(getFactoryName() + " : Name already used : " + name2);

+                    }

+                } else if (INSTANCE_NAME.contains(name2)) {

+                    m_logger.log(Logger.ERROR, "The configuration is not acceptable : Name already used");

+                    throw new UnacceptableConfiguration(getFactoryName() + " : Name already used : " + name2);

+                }

+                // reserve the name

+                INSTANCE_NAME.add(name2);

+            }

+            return name2;

+        }

+    }

+

+    /**

+     * This generator choose the right NameGenerator.

+     */

+    private class SwitchNameGenerator implements NameGenerator {

+        private NameGenerator computed = new DefaultNameGenerator();

+        private NameGenerator userProvided = new UserProvidedNameGenerator();

+

+        public String generate(String name, String version) throws UnacceptableConfiguration

+        {

+            if (name == null) {

+                return computed.generate(null, null);

+            }

+            return userProvided.generate(name, version);

+        }

+    }

+

+

 }