You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@felix.apache.org by cz...@apache.org on 2020/04/07 10:50:15 UTC

[felix-dev] branch master updated: FELIX-6252 : Deadlock in SCR ComponentRegistry updateChangeCount

This is an automated email from the ASF dual-hosted git repository.

cziegeler pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/felix-dev.git


The following commit(s) were added to refs/heads/master by this push:
     new f25c1c3  FELIX-6252 : Deadlock in SCR ComponentRegistry updateChangeCount
f25c1c3 is described below

commit f25c1c3cfdc0e1df6804c62dc00b47f5cf8a637e
Author: Carsten Ziegeler <cz...@apache.org>
AuthorDate: Tue Apr 7 12:49:45 2020 +0200

    FELIX-6252 : Deadlock in SCR ComponentRegistry updateChangeCount
---
 .../apache/felix/scr/impl/ComponentRegistry.java   | 78 ++++++++++++----------
 1 file changed, 42 insertions(+), 36 deletions(-)

diff --git a/scr/src/main/java/org/apache/felix/scr/impl/ComponentRegistry.java b/scr/src/main/java/org/apache/felix/scr/impl/ComponentRegistry.java
index a76c855..a1574b7 100644
--- a/scr/src/main/java/org/apache/felix/scr/impl/ComponentRegistry.java
+++ b/scr/src/main/java/org/apache/felix/scr/impl/ComponentRegistry.java
@@ -32,6 +32,7 @@ import java.util.Timer;
 import java.util.TimerTask;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.atomic.AtomicLong;
 
 import org.apache.felix.scr.impl.inject.ComponentMethods;
 import org.apache.felix.scr.impl.inject.internal.ComponentMethodsImpl;
@@ -701,18 +702,18 @@ public class ComponentRegistry
 
 	}
 
-	private final Object changeCountLock = new Object();
+    private final AtomicLong changeCount = new AtomicLong();
 
-    private volatile long changeCount;
+    private volatile Timer changeCountTimer;
 
-    private volatile Timer timer;
+    private final Object changeCountTimerLock = new Object();
 
     private volatile ServiceRegistration<ServiceComponentRuntime> registration;
 
     public Dictionary<String, Object> getServiceRegistrationProperties()
     {
         final Dictionary<String, Object> props = new Hashtable<>();
-        props.put(PROP_CHANGECOUNT, this.changeCount);
+        props.put(PROP_CHANGECOUNT, this.changeCount.get());
 
         return props;
     }
@@ -726,45 +727,50 @@ public class ComponentRegistry
     {
         if ( registration != null )
         {
-            synchronized ( changeCountLock )
-            {
-                this.changeCount++;
-                final long count = this.changeCount;
-                if ( this.timer == null )
-                {
-                    this.timer = new Timer();
+            final long count = this.changeCount.incrementAndGet();
+
+            final Timer timer;
+            synchronized ( this.changeCountTimerLock ) {
+                if ( this.changeCountTimer == null ) {
+                    this.changeCountTimer = new Timer();
                 }
-                try
-                {
-                    timer.schedule(new TimerTask()
-                        {
+                timer = this.changeCountTimer;
+            }
+            try
+            {
+                timer.schedule(new TimerTask()
+                    {
 
-                            @Override
-                            public void run() {
-                                synchronized ( changeCountLock )
+                        @Override
+                        public void run()
+                        {
+                            if ( changeCount.get() == count )
+                            {
+                                try
                                 {
-                                    if ( changeCount == count )
+                                    registration.setProperties(getServiceRegistrationProperties());
+                                }
+                                catch ( final IllegalStateException ise)
+                                {
+                                    // we ignore this as this might happen on shutdown
+                                }
+                                synchronized ( changeCountTimerLock )
+                                {
+                                    if ( changeCount.get() == count )
                                     {
-                                        try
-                                        {
-                                            registration.setProperties(getServiceRegistrationProperties());
-                                        }
-                                        catch ( final IllegalStateException ise)
-                                        {
-                                            // we ignore this as this might happen on shutdown
-                                        }
-                                        timer.cancel();
-                                        timer = null;
+                                        changeCountTimer.cancel();
+                                        changeCountTimer = null;
                                     }
                                 }
+
                             }
-                        }, m_configuration.serviceChangecountTimeout());
-                }
-                catch (Exception e) {
-                    m_logger.log(LogService.LOG_WARNING,
-                        "Service changecount Timer for {0} had a problem", e,
-                        registration.getReference());
-                }
+                        }
+                    }, m_configuration.serviceChangecountTimeout());
+            }
+            catch (Exception e) {
+                m_logger.log(LogService.LOG_WARNING,
+                    "Service changecount Timer for {0} had a problem", e,
+                    registration.getReference());
             }
         }
     }