felix-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rickh...@apache.org
Subject svn commit: r628190 - in /felix/trunk/framework/src/main/java/org/apache/felix/framework: ServiceRegistrationImpl.java ServiceRegistry.java
Date Fri, 15 Feb 2008 22:20:00 GMT
Author: rickhall
Date: Fri Feb 15 14:19:59 2008
New Revision: 628190

URL: http://svn.apache.org/viewvc?rev=628190&view=rev
Log:
Modified the service registry to use more fine-grained locking to avoid
callbacks to service factories while holding locks. This complicates the
implementation considerably, but should hopefully address FELIX-489.

Modified:
    felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistrationImpl.java
    felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java

Modified: felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistrationImpl.java
URL: http://svn.apache.org/viewvc/felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistrationImpl.java?rev=628190&r1=628189&r2=628190&view=diff
==============================================================================
--- felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistrationImpl.java
(original)
+++ felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistrationImpl.java
Fri Feb 15 14:19:59 2008
@@ -44,6 +44,8 @@
     private Map m_propMap =  new StringMap(false);
     // Re-usable service reference.
     private ServiceReferenceImpl m_ref = null;
+    // Flag indicating that we are unregistering.
+    private boolean m_isUnregistering = false;
 
     public ServiceRegistrationImpl(
         ServiceRegistry registry, Bundle bundle,
@@ -68,11 +70,16 @@
         m_ref = new ServiceReferenceImpl(this, m_bundle);
     }
 
-    protected boolean isValid()
+    protected synchronized boolean isValid()
     {
         return (m_svcObj != null);
     }
 
+    protected synchronized void invalidate()
+    {
+        m_svcObj = null;
+    }
+
     public ServiceReference getReference()
     {
         return m_ref;
@@ -94,15 +101,19 @@
 
     public void unregister()
     {
-        if (m_svcObj != null)
+        synchronized (this)
         {
-            m_registry.unregisterService(m_bundle, this);
-            m_svcObj = null;
-            m_factory = null;
+            if (!isValid() || m_isUnregistering)
+            {
+                throw new IllegalStateException("Service already unregistered.");
+            }
+            m_isUnregistering = true;
         }
-        else
+        m_registry.unregisterService(m_bundle, this);
+        synchronized (this)
         {
-            throw new IllegalStateException("Service already unregistered.");
+            m_svcObj = null;
+            m_factory = null;
         }
     }
 
@@ -195,7 +206,7 @@
     protected void ungetService(Bundle relBundle, Object svcObj)
     {
         // If the service object is a service factory, then
-        // let is release the service object.
+        // let it release the service object.
         if (m_factory != null)
         {
             try

Modified: felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java
URL: http://svn.apache.org/viewvc/felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java?rev=628190&r1=628189&r2=628190&view=diff
==============================================================================
--- felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java (original)
+++ felix/trunk/framework/src/main/java/org/apache/felix/framework/ServiceRegistry.java Fri
Feb 15 14:19:59 2008
@@ -29,6 +29,10 @@
     private long m_currentServiceId = 1L;
     // Maps bundle to an array of service registrations.
     private Map m_serviceRegsMap = new HashMap();
+    // Maps registration to thread to keep track when a
+    // registration is in use, which will cause other
+    // threads to wait.
+    private Map m_lockedRegsMap = new HashMap();
     // Maps bundle to an array of usage counts.
     private Map m_inUseMap = new HashMap();
 
@@ -74,9 +78,15 @@
 
     public void unregisterService(Bundle bundle, ServiceRegistration reg)
     {
-        // First remove the registered service.
         synchronized (this)
         {
+            // Note that we don't lock the service registration here using
+            // the m_lockedRegsMap because we want to allow bundles to get
+            // the service during the unregistration process. However, since
+            // we do remove the registration from the service registry, no
+            // new bundles will be able to look up the service.
+
+            // Now remove the registered service.
             ServiceRegistration[] regs = (ServiceRegistration[]) m_serviceRegsMap.get(bundle);
             m_serviceRegsMap.put(bundle, removeServiceRegistration(regs, reg));
         }
@@ -94,6 +104,7 @@
                 while (ungetService(clients[i], reg.getReference()))
                     ; // Keep removing until it is no longer possible
             }
+            ((ServiceRegistrationImpl) reg).invalidate();
         }
     }
 
@@ -113,6 +124,11 @@
             regs = (ServiceRegistration[]) m_serviceRegsMap.get(bundle);
         }
 
+        // Note, there is no race condition here with respect to the
+        // bundle registering more services, because its bundle context
+        // has already been invalidated by this point, so it would not
+        // be able to register more services.
+
         // Unregister each service.
         for (int i = 0; (regs != null) && (i < regs.length); i++)
         {
@@ -120,11 +136,13 @@
         }
 
         // Now remove the bundle itself.
-        m_serviceRegsMap.remove(bundle);
+        synchronized (this)
+        {
+            m_serviceRegsMap.remove(bundle);
+        }
     }
 
     public synchronized List getServiceReferences(String className, Filter filter)
-        throws InvalidSyntaxException
     {
         // Create a filtered list of service references.
         List list = new ArrayList();
@@ -195,104 +213,204 @@
         return null;
     }
 
-    public synchronized Object getService(Bundle bundle, ServiceReference ref)
+    public Object getService(Bundle bundle, ServiceReference ref)
     {
-        // Variable for service object.
+        UsageCount usage = null;
         Object svcObj = null;
 
         // Get the service registration.
         ServiceRegistrationImpl reg = ((ServiceReferenceImpl) ref).getServiceRegistration();
 
-        // Make sure the service registration is still valid.
-        if (!reg.isValid())
-        {
-            // If the service registration is not valid, then this means
-            // that the service provider unregistered the service. The spec
-            // says that calls to get an unregistered service should always
-            // return null (assumption: even if it is currently cached
-            // by the bundle). So in this case, flush the service reference
-            // from the cache and return null.
-            flushUsageCount(bundle, ref);
-
-            // It is not necessary to unget the service object from
-            // the providing bundle, since the associated service is
-            // unregistered and hence not in the list of registered services
-            // of the providing bundle. This is precisely why the service
-            // registration was not found above in the first place.
-        }
-        else
+        synchronized (this)
         {
-            // Get the usage count, if any.
-            UsageCount usage = getUsageCount(bundle, ref);
+            // First make sure that no existing operation is currently
+            // being performed by another thread on the service registration.
+            for (Object o = m_lockedRegsMap.get(reg); (o != null); o = m_lockedRegsMap.get(reg))
+            {
+                // We don't allow cycles when we call out to the service factory.
+                if (o.equals(Thread.currentThread()))
+                {
+                    throw new IllegalStateException(
+                        "ServiceFactory.getService() resulted in a cycle.");
+                }
 
-            // If the service object is cached, then increase the usage
-            // count and return the cached service object.
-            if (usage != null)
+                // Otherwise, wait for it to be freed.
+                try
+                {
+                    wait();
+                }
+                catch (InterruptedException ex)
+                {
+                }
+            }
+
+            // Lock the service registration.
+            m_lockedRegsMap.put(reg, Thread.currentThread());
+
+            // Make sure the service registration is still valid.
+            if (!reg.isValid())
             {
-                usage.m_count++;
-                svcObj = usage.m_svcObj;
+                // If the service registration is not valid, then this means
+                // that the service provider unregistered the service. The spec
+                // says that calls to get an unregistered service should always
+                // return null (assumption: even if it is currently cached
+                // by the bundle). So in this case, flush the service reference
+                // from the cache and return null.
+                flushUsageCount(bundle, ref);
+
+                // It is not necessary to unget the service object from
+                // the providing bundle, since the associated service is
+                // unregistered and hence not in the list of registered services
+                // of the providing bundle. This is precisely why the service
+                // registration was not found above in the first place.
             }
             else
             {
+                // Get the usage count, if any.
+                usage = getUsageCount(bundle, ref);
+
+                // If the service object is cached, then increase the usage
+                // count and return the cached service object.
+                if (usage != null)
+                {
+                    usage.m_count++;
+                    svcObj = usage.m_svcObj;
+                }
+            }
+        }
+
+        // If we have a valid registration, but no usage count, that means we
+        // don't have a cached service object, so we need to create one now
+        // without holding the lock, since we will potentially call out to a
+        // service factory.
+        try
+        {
+            if (reg.isValid() && (usage == null))
+            {
                 // Get service object from service registration.
                 svcObj = reg.getService(bundle);
 
                 // Cache the service object.
                 if (svcObj != null)
                 {
-                    addUsageCount(bundle, ref, svcObj);
+                    synchronized (this)
+                    {
+                        // Unregistration can happen concurrently, so we need
+                        // to double-check that we are still valid.
+                        if (reg.isValid())
+                        {
+                            addUsageCount(bundle, ref, svcObj);
+                        }
+                        else
+                        {
+                            // The service must have been unregistered in the
+                            // middle of our get operation, so null it.
+                            svcObj = null;
+                        }
+                    }
                 }
             }
         }
+        finally
+        {
+            // Finally, unlock the service registration so that any threads
+            // waiting for it can continue.
+            synchronized (this)
+            {
+                m_lockedRegsMap.remove(reg);
+                notifyAll();
+            }
+        }
 
         return svcObj;
     }
 
-    public synchronized boolean ungetService(Bundle bundle, ServiceReference ref)
+    public boolean ungetService(Bundle bundle, ServiceReference ref)
     {
-        // Result of unget.
-        boolean result = false;
-
-        // Get usage count.
-        UsageCount usage = getUsageCount(bundle, ref);
+        UsageCount usage = null;
+        ServiceRegistrationImpl reg = ((ServiceReferenceImpl) ref).getServiceRegistration();
 
-        // If no usage count, then return.
-        if (usage != null)
+        synchronized (this)
         {
+            // First make sure that no existing operation is currently
+            // being performed by another thread on the service registration.
+            for (Object o = m_lockedRegsMap.get(reg); (o != null); o = m_lockedRegsMap.get(reg))
+            {
+                // We don't allow cycles when we call out to the service factory.
+                if (o.equals(Thread.currentThread()))
+                {
+                    throw new IllegalStateException(
+                        "ServiceFactory.ungetService() resulted in a cycle.");
+                }
+
+                // Otherwise, wait for it to be freed.
+                try
+                {
+                    wait();
+                }
+                catch (InterruptedException ex)
+                {
+                }
+            }
+
+            // Get the usage count.
+            usage = getUsageCount(bundle, ref);
+            // If there is no cached services, then just return immediately.
+            if (usage == null)
+            {
+                return false;
+            }
+
+            // Lock the service registration.
+            m_lockedRegsMap.put(reg, Thread.currentThread());
+
             // Make sure the service registration is still valid.
-            if (!((ServiceReferenceImpl) ref).getServiceRegistration().isValid())
+            if (!reg.isValid())
             {
                 // If the service registration is not valid, then this means
-                // that the service provider unregistered the service. The spec
-                // says that calls to get an unregistered service should always
-                // return null (assumption: even if it is currently cached
-                // by the bundle). So in this case, flush the service reference
-                // from the cache and return null.
+                // that the service provider unregistered the service, so just
+                // flush the usage count and we are done.
                 flushUsageCount(bundle, ref);
             }
-            else
+            else if (reg.isValid())
             {
                 // Decrement usage count.
                 usage.m_count--;
+            }
 
-                // Remove reference when usage count goes to zero
-                // and unget the service object from the exporting
-                // bundle.
-                if (usage.m_count == 0)
-                {
-                    // Remove reference from usages array.
-                    flushUsageCount(bundle, ref);
-                    ((ServiceReferenceImpl) ref)
-                        .getServiceRegistration().ungetService(bundle, usage.m_svcObj);
-                    usage.m_svcObj = null;
-                }
+            // If the usage count has reached zero, the flush it.
+            if (usage.m_count == 0)
+            {
+                flushUsageCount(bundle, ref);
+            }
+        }
+
+        // If usage count goes to zero, then unget the service
+        // from the registration; we do this outside the lock
+        // since this might call out to the service factory.
+        try
+        {
+            if (usage.m_count == 0)
+            {
+                // Remove reference from usages array.
+                ((ServiceReferenceImpl) ref)
+                    .getServiceRegistration().ungetService(bundle, usage.m_svcObj);
+                usage.m_svcObj = null;
+            }
 
-                // Return true if the usage count is greater than zero.
-                result = (usage.m_count > 0);
+        }
+        finally
+        {
+            // Finally, unlock the service registration so that any threads
+            // waiting for it can continue.
+            synchronized (this)
+            {
+                m_lockedRegsMap.remove(reg);
+                notifyAll();
             }
         }
 
-        return result;
+        return (usage.m_count > 0);
     }
 
 
@@ -301,14 +419,24 @@
      * used by the specified bundle.
      * @param bundle the bundle whose services are to be released.
     **/
-    public synchronized void ungetServices(Bundle bundle)
+    public void ungetServices(Bundle bundle)
     {
-        UsageCount[] usages = (UsageCount[]) m_inUseMap.get(bundle);
+        UsageCount[] usages;
+        synchronized (this)
+        {
+            usages = (UsageCount[]) m_inUseMap.get(bundle);
+        }
+        
         if (usages == null)
         {
             return;
         }
 
+        // Note, there is no race condition here with respect to the
+        // bundle using more services, because its bundle context
+        // has already been invalidated by this point, so it would not
+        // be able to look up more services.
+
         // Remove each service object from the
         // service cache.
         for (int i = 0; i < usages.length; i++)
@@ -420,11 +548,15 @@
     protected void fireServiceChanged(ServiceEvent event)
     {
         // Grab a copy of the listener list.
-        ServiceListener listener = m_serviceListener;
+        ServiceListener listener;
+        synchronized (this)
+        {
+            listener = m_serviceListener;
+        }
         // If not null, then dispatch event.
         if (listener != null)
         {
-            m_serviceListener.serviceChanged(event);
+            listener.serviceChanged(event);
         }
     }
 



Mime
View raw message