felix-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From gno...@apache.org
Subject svn commit: r1509691 - in /felix/trunk/scr/src/main/java/org/apache/felix/scr/impl: helper/BindMethod.java manager/DependencyManager.java manager/RefPair.java
Date Fri, 02 Aug 2013 13:22:57 GMT
Author: gnodet
Date: Fri Aug  2 13:22:57 2013
New Revision: 1509691

URL: http://svn.apache.org/r1509691
Log:
[FELIX-4189] DS should not hold any lock while calling bundleContext#getService and #ungetService

Modified:
    felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/helper/BindMethod.java
    felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
    felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/RefPair.java

Modified: felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/helper/BindMethod.java
URL: http://svn.apache.org/viewvc/felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/helper/BindMethod.java?rev=1509691&r1=1509690&r2=1509691&view=diff
==============================================================================
--- felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/helper/BindMethod.java (original)
+++ felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/helper/BindMethod.java Fri Aug
 2 13:22:57 2013
@@ -578,7 +578,20 @@ public class BindMethod extends BaseMeth
                          "Could not get service from ref {0}", new Object[] {refPair.getRef()},
null );
                     return false;
                 }
-                refPair.setServiceObject( service );
+                Object oldService;
+                synchronized ( refPair )
+                {
+                    oldService = refPair.getServiceObject();
+                    if (oldService == null)
+                    {
+                        refPair.setServiceObject(service);
+                    }
+                }
+                if (oldService != null)
+                {
+                    // Another thread got the service before, so unget our
+                    context.ungetService( refPair.getRef() );
+                }
                 return true;
             }
         }

Modified: felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
URL: http://svn.apache.org/viewvc/felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java?rev=1509691&r1=1509690&r2=1509691&view=diff
==============================================================================
--- felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
(original)
+++ felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
Fri Aug  2 13:22:57 2013
@@ -216,16 +216,21 @@ public class DependencyManager<S, T> imp
 
         protected void ungetService( RefPair<T> ref )
         {
+            Object service;
             synchronized ( ref )
             {
-                if ( ref.getServiceObject() != null )
+                service = ref.getServiceObject();
+                if ( ref != null )
                 {
                     ref.setServiceObject( null );
-                    BundleContext bundleContext = m_componentManager.getBundleContext();
-                    if ( bundleContext != null )
-                    {
-                        bundleContext.ungetService( ref.getRef() );
-                    }
+                }
+            }
+            if ( service != null )
+            {
+                BundleContext bundleContext = m_componentManager.getBundleContext();
+                if ( bundleContext != null )
+                {
+                    bundleContext.ungetService( ref.getRef() );
                 }
             }
         }
@@ -377,16 +382,13 @@ public class DependencyManager<S, T> imp
             SortedMap<ServiceReference<T>, RefPair<T>> tracked = getTracker().getTracked(
true, trackingCount );
             for (RefPair<T> refPair: tracked.values())
             {
-                synchronized (refPair)
+                if (getServiceObject( m_bindMethods.getBind(), refPair ))
                 {
-                    if (getServiceObject( m_bindMethods.getBind(), refPair ))
-                    {
-                         success = true;
-                    }
-                    else
-                    {
-                         m_componentManager.registerMissingDependency( DependencyManager.this,
refPair.getRef(), trackingCount.get() );
-                    }
+                     success = true;
+                }
+                else
+                {
+                     m_componentManager.registerMissingDependency( DependencyManager.this,
refPair.getRef(), trackingCount.get() );
                 }
             }
             return success;
@@ -499,14 +501,11 @@ public class DependencyManager<S, T> imp
             SortedMap<ServiceReference<T>, RefPair<T>> tracked = getTracker().getTracked(
success || !getTracker().isEmpty(), trackingCount );
             for (RefPair<T> refPair: tracked.values())
             {
-                synchronized (refPair)
+                success |= getServiceObject( m_bindMethods.getBind(), refPair );
+                if ( refPair.isFailed() )
                 {
-                    success |= getServiceObject( m_bindMethods.getBind(), refPair );
-                    if ( refPair.isFailed() )
-                    {
-                        m_componentManager.registerMissingDependency( DependencyManager.this,
refPair.getRef(),
-                                trackingCount.get() );
-                    }
+                    m_componentManager.registerMissingDependency( DependencyManager.this,
refPair.getRef(),
+                            trackingCount.get() );
                 }
             }
             return success;
@@ -605,10 +604,7 @@ public class DependencyManager<S, T> imp
                 //another thread is concurrently opening, and it got done already
                 for (RefPair<T> refPair: refs)
                 {
-                    synchronized (refPair)
-                    {
-                        success |= getServiceObject( m_bindMethods.getBind(), refPair );
-                    }
+                    success |= getServiceObject( m_bindMethods.getBind(), refPair );
                 }
                 return success;
             }
@@ -617,10 +613,7 @@ public class DependencyManager<S, T> imp
             SortedMap<ServiceReference<T>, RefPair<T>> tracked = getTracker().getTracked(
true, trackingCount );
             for (RefPair<T> refPair: tracked.values())
             {
-                synchronized (refPair)
-                {
-                    success |= getServiceObject( m_bindMethods.getBind(), refPair );
-                }
+                success |= getServiceObject( m_bindMethods.getBind(), refPair );
                 refs.add(refPair) ;
             }
             if ( this.refs.compareAndSet( null, refs ) )
@@ -778,12 +771,9 @@ public class DependencyManager<S, T> imp
             }
             if ( nextRefPair != null )
             {
-                synchronized ( nextRefPair )
+                if ( !getServiceObject( m_bindMethods.getBind(), nextRefPair ) )
                 {
-                    if ( !getServiceObject( m_bindMethods.getBind(), nextRefPair ) )
-                    {
-                        //TODO error???
-                    }
+                    //TODO error???
                 }
                 if ( !nextRefPair.isFailed() )
                 {
@@ -841,14 +831,11 @@ public class DependencyManager<S, T> imp
             }
             if (refPair != null) 
             {
-                synchronized( refPair )
+                success |= getServiceObject( m_bindMethods.getBind(), refPair );
+                if ( refPair.isFailed() )
                 {
-                    success |= getServiceObject( m_bindMethods.getBind(), refPair );
-                    if ( refPair.isFailed() )
-                    {
-                        m_componentManager.registerMissingDependency( DependencyManager.this,
refPair.getRef(),
-                                trackingCount.get() );
-                    }
+                    m_componentManager.registerMissingDependency( DependencyManager.this,
refPair.getRef(),
+                            trackingCount.get() );
                 }
             }
             return success;
@@ -985,14 +972,11 @@ public class DependencyManager<S, T> imp
                 }
                 if ( refPair != null )
                 {
-                    synchronized ( refPair )
+                    success |= getServiceObject( m_bindMethods.getBind(), refPair );
+                    if ( refPair.isFailed() )
                     {
-                        success |= getServiceObject( m_bindMethods.getBind(), refPair );
-                        if ( refPair.isFailed() )
-                        {
-                            m_componentManager.registerMissingDependency( DependencyManager.this,
refPair.getRef(),
-                                    trackingCount.get() );
-                        }
+                        m_componentManager.registerMissingDependency( DependencyManager.this,
refPair.getRef(),
+                                trackingCount.get() );
                     }
                 }
             }
@@ -1536,18 +1520,15 @@ public class DependencyManager<S, T> imp
         }
         //TODO dynamic reluctant
         RefPair<T> refPair = trackerRef.get().getService( ref );
-        synchronized ( refPair )
+        if (refPair.getServiceObject() != null)
         {
-            if (refPair.getServiceObject() != null)
-            {
-                m_componentManager.log( LogService.LOG_DEBUG,
-                        "DependencyManager : late binding of service reference {1} skipped
as service has already been located",
-                        new Object[] {ref}, null );
-                //something else got the reference and may be binding it.
-                return;
-            }
-            getServiceObject( m_bindMethods.getBind(), refPair );
+            m_componentManager.log( LogService.LOG_DEBUG,
+                    "DependencyManager : late binding of service reference {1} skipped as
service has already been located",
+                    new Object[] {ref}, null );
+            //something else got the reference and may be binding it.
+            return;
         }
+        getServiceObject( m_bindMethods.getBind(), refPair );
         m_componentManager.invokeBindMethod( this, refPair, trackingCount );
     }
 

Modified: felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/RefPair.java
URL: http://svn.apache.org/viewvc/felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/RefPair.java?rev=1509691&r1=1509690&r2=1509691&view=diff
==============================================================================
--- felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/RefPair.java (original)
+++ felix/trunk/scr/src/main/java/org/apache/felix/scr/impl/manager/RefPair.java Fri Aug 
2 13:22:57 2013
@@ -42,12 +42,12 @@ public class RefPair<T>
         return ref;
     }
 
-    public T getServiceObject()
+    public synchronized T getServiceObject()
     {
         return serviceObject;
     }
 
-    public void setServiceObject( T serviceObject )
+    public synchronized void setServiceObject( T serviceObject )
     {
         this.serviceObject = serviceObject;
         if ( serviceObject != null)
@@ -56,12 +56,12 @@ public class RefPair<T>
         }
     }
 
-    public void setFailed( )
+    public synchronized void setFailed( )
     {
         this.failed = true;
     }
 
-    public boolean isFailed()
+    public synchronized boolean isFailed()
     {
         return failed;
     }



Mime
View raw message