FELIX-3891 Fix the new way to avoid deadlocks, and preserve state change ordering, with an actual unit test
git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1452208 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java b/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java
index 0860302..1fadcf5 100644
--- a/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java
+++ b/scr/src/main/java/org/apache/felix/scr/impl/manager/AbstractComponentManager.java
@@ -97,14 +97,8 @@
// A reference to the BundleComponentActivator
private BundleComponentActivator m_activator;
- // The ServiceRegistration
- private enum RegState {unregistered, registered};
- private final Lock registrationLock = new ReentrantLock();
- //Deque, ArrayDeque if we had java 6
- private final List<RegState> opqueue = new ArrayList<RegState>();
-
- private volatile ServiceRegistration<S> m_serviceRegistration;
-
+ // The ServiceRegistration is now tracked in the RegistrationManager
+
private final ReentrantLock m_stateLock;
protected volatile boolean enabled;
@@ -652,7 +646,7 @@
final ServiceReference getServiceReference()
{
- ServiceRegistration reg = m_serviceRegistration;
+ ServiceRegistration reg = getServiceRegistration();
if (reg != null)
{
return reg.getReference();
@@ -702,6 +696,34 @@
return null;
}
+
+ private final RegistrationManager<ServiceRegistration<S>> registrationManager = new RegistrationManager<ServiceRegistration<S>>()
+ {
+
+ @Override
+ ServiceRegistration<S> register(String[] services)
+ {
+ final Dictionary<String, Object> serviceProperties = getServiceProperties();
+ ServiceRegistration<S> serviceRegistration = ( ServiceRegistration<S> ) getActivator().getBundleContext()
+ .registerService( services, getService(), serviceProperties );
+ return serviceRegistration;
+ }
+
+ @Override
+ void unregister(ServiceRegistration<S> serviceRegistration)
+ {
+ serviceRegistration.unregister();
+ }
+
+ @Override
+ void log(int level, String message, Object[] arguments, Throwable ex)
+ {
+ AbstractComponentManager.this.log(level, message, arguments, ex);
+ }
+
+ };
+
+
/**
* Registers the service on behalf of the component.
*
@@ -711,7 +733,7 @@
String[] services = getProvidedServices();
if ( services != null )
{
- return changeRegistration( RegState.registered, services);
+ return registrationManager.changeRegistration( RegistrationManager.RegState.registered, services);
}
return true;
}
@@ -721,92 +743,11 @@
String[] services = getProvidedServices();
if ( services != null )
{
- return changeRegistration( RegState.unregistered, services );
+ return registrationManager.changeRegistration( RegistrationManager.RegState.unregistered, services );
}
return true;
}
- /**
- *
- * @param desired
- * @return true if this thread reached the requested state
- */
- private boolean changeRegistration( RegState desired, String[] services )
- {
- registrationLock.lock();
- try
- {
- if (opqueue.isEmpty())
- {
- if ((desired == RegState.unregistered) == (m_serviceRegistration == null))
- {
- return false; //already in desired state
- }
- }
- else if (opqueue.get(0) == desired)
- {
- return false; //another thread will do our work.
- }
- opqueue.add( desired );
- if (opqueue.size() > 1)
- {
- return false; //some other thread will do it later
- }
- //we're next
- do
- {
- log( LogService.LOG_DEBUG, "registration change queue {0}", new Object[]
- {opqueue}, null );
- desired = opqueue.get( 0 );
- ServiceRegistration<S> serviceRegistration = m_serviceRegistration;
- if ( desired == RegState.unregistered)
- {
- m_serviceRegistration = null;
- }
-
- registrationLock.unlock();
- try
- {
- if (desired == RegState.registered)
- {
- final Dictionary<String, Object> serviceProperties = getServiceProperties();
- serviceRegistration = ( ServiceRegistration<S> ) getActivator().getBundleContext()
- .registerService( services, getService(), serviceProperties );
-
- }
- else
- {
- if ( serviceRegistration != null )
- {
- serviceRegistration.unregister();
- }
- else
- {
- log( LogService.LOG_ERROR, "Unexpected unregistration request with no registration present", new Object[]
- {}, new Exception("Stack trace") );
-
- }
- }
- }
- finally
- {
- registrationLock.lock();
- opqueue.remove(0);
- if ( desired == RegState.registered)
- {
- m_serviceRegistration = serviceRegistration;
- }
- }
- }
- while (!opqueue.isEmpty());
- return true;
- }
- finally
- {
- registrationLock.unlock();
- }
-
- }
AtomicInteger getTrackingCount()
{
@@ -907,9 +848,9 @@
}
- final ServiceRegistration<?> getServiceRegistration()
+ final ServiceRegistration<S> getServiceRegistration()
{
- return m_serviceRegistration;
+ return registrationManager.getServiceRegistration();
}
@@ -1225,7 +1166,7 @@
void changeState( State newState )
{
log( LogService.LOG_DEBUG, "State transition : {0} -> {1} : service reg: {2}", new Object[]
- { m_state, newState, m_serviceRegistration }, null );
+ { m_state, newState, getServiceRegistration() }, null );
m_state = newState;
}
@@ -1330,7 +1271,7 @@
private void log( AbstractComponentManager acm, String event )
{
acm.log( LogService.LOG_DEBUG, "Current state: {0}, Event: {1}, Service registration: {2}", new Object[]
- { m_name, event, acm.m_serviceRegistration }, null );
+ { m_name, event, acm.getServiceRegistration() }, null );
}
void doDeactivate( AbstractComponentManager acm, int reason, boolean disable )
diff --git a/scr/src/main/java/org/apache/felix/scr/impl/manager/RegistrationManager.java b/scr/src/main/java/org/apache/felix/scr/impl/manager/RegistrationManager.java
new file mode 100644
index 0000000..10f7395
--- /dev/null
+++ b/scr/src/main/java/org/apache/felix/scr/impl/manager/RegistrationManager.java
@@ -0,0 +1,111 @@
+package org.apache.felix.scr.impl.manager;
+
+import java.util.ArrayList;
+import java.util.Dictionary;
+import java.util.List;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import org.osgi.service.log.LogService;
+
+abstract class RegistrationManager<T>
+{
+ enum RegState {unregistered, registered};
+ private final Lock registrationLock = new ReentrantLock();
+ //Deque, ArrayDeque if we had java 6
+ private final List<RegState> opqueue = new ArrayList<RegState>();
+
+ private volatile T m_serviceRegistration;
+ /**
+ *
+ * @param desired
+ * @param services TODO
+ * @return true if this thread reached the requested state
+ */
+ boolean changeRegistration( RegState desired, String[] services )
+ {
+ registrationLock.lock();
+ try
+ {
+ if (opqueue.isEmpty())
+ {
+ if ((desired == RegState.unregistered) == (m_serviceRegistration == null))
+ {
+ return false; //already in desired state
+ }
+ }
+ else if (opqueue.get( opqueue.size() - 1 ) == desired)
+ {
+ return false; //another thread will do our work.
+ }
+ opqueue.add( desired );
+ if (opqueue.size() > 1)
+ {
+ return false; //some other thread will do it later
+ }
+ //we're next
+ do
+ {
+ log( LogService.LOG_DEBUG, "registration change queue {0}", new Object[]
+ {opqueue}, null );
+ desired = opqueue.get( 0 );
+ T serviceRegistration = m_serviceRegistration;
+ if ( desired == RegState.unregistered)
+ {
+ m_serviceRegistration = null;
+ }
+
+ registrationLock.unlock();
+ try
+ {
+ if (desired == RegState.registered)
+ {
+ serviceRegistration = register(services );
+
+ }
+ else
+ {
+ if ( serviceRegistration != null )
+ {
+ unregister( serviceRegistration );
+ }
+ else
+ {
+ log( LogService.LOG_ERROR, "Unexpected unregistration request with no registration present", new Object[]
+ {}, new Exception("Stack trace") );
+
+ }
+ }
+ }
+ finally
+ {
+ registrationLock.lock();
+ opqueue.remove(0);
+ if ( desired == RegState.registered)
+ {
+ m_serviceRegistration = serviceRegistration;
+ }
+ }
+ }
+ while (!opqueue.isEmpty());
+ return true;
+ }
+ finally
+ {
+ registrationLock.unlock();
+ }
+
+ }
+
+ abstract T register(String[] services);
+
+ abstract void unregister(T serviceRegistration);
+
+ abstract void log( int level, String message, Object[] arguments, Throwable ex );
+
+ T getServiceRegistration()
+ {
+ return m_serviceRegistration;
+ }
+
+}
diff --git a/scr/src/test/java/org/apache/felix/scr/impl/manager/RegistrationManagerTest.java b/scr/src/test/java/org/apache/felix/scr/impl/manager/RegistrationManagerTest.java
new file mode 100644
index 0000000..1229b56
--- /dev/null
+++ b/scr/src/test/java/org/apache/felix/scr/impl/manager/RegistrationManagerTest.java
@@ -0,0 +1,113 @@
+package org.apache.felix.scr.impl.manager;
+
+import static org.junit.Assert.fail;
+
+import java.util.ArrayList;
+import java.util.concurrent.CountDownLatch;
+
+import org.apache.felix.scr.impl.manager.RegistrationManager.RegState;
+import org.junit.Test;
+
+
+public class RegistrationManagerTest
+{
+
+ private volatile boolean fail;
+
+ private int n = 10;
+ private ArrayList<Thread> threads = new ArrayList<Thread>();
+
+ private TRM trm = new TRM();
+
+ @Test
+ public void testRegistrationManager( ) throws Exception
+ {
+
+ for (int setup = 0; setup < 1 << n; setup++ )
+ {
+ runTest(setup);
+ if (fail)
+ {
+ fail("failed at " + setup);
+ }
+ }
+ }
+
+ private void runTest(int setup) throws InterruptedException
+ {
+ final CountDownLatch done = new CountDownLatch( n );
+ for (int i = 0; i < n; i++)
+ {
+ boolean b = ((setup >>> i) & 1) == 0;
+ final RegState change = b? RegState.registered: RegState.unregistered;
+ new Thread(new Runnable() {
+
+ public void run()
+ {
+ trm.changeRegistration( change, null );
+ }
+
+ }).start();
+ done.countDown();
+ }
+ done.await();
+ }
+
+
+ private class TRM extends RegistrationManager<Object>
+ {
+
+ @Override
+ Object register(String[] services)
+ {
+ try
+ {
+ Thread.sleep( 1 );
+ }
+ catch ( InterruptedException e )
+ {
+ // TODO Auto-generated catch block
+ e.printStackTrace();
+ }
+ return new Object();
+ }
+
+ @Override
+ void unregister(Object serviceRegistration)
+ {
+ try
+ {
+ Thread.sleep( 1 );
+ }
+ catch ( InterruptedException e )
+ {
+ // TODO Auto-generated catch block
+ e.printStackTrace();
+ }
+ }
+
+ @Override
+ void log(int level, String message, Object[] arguments, Throwable ex)
+ {
+ if ( arguments.length == 1 && (arguments[0] instanceof ArrayList))
+ {
+ ArrayList<RegState> opqueue = ( ArrayList<org.apache.felix.scr.impl.manager.RegistrationManager.RegState> ) arguments[0];
+// System.out.println("opqueue: " + opqueue);
+ if (opqueue.size() > 1)
+ {
+ for (int i = 1; i < opqueue.size(); i++)
+ {
+ if (opqueue.get( i -1 ) == opqueue.get(i))
+ {
+ fail = true;
+ System.out.println("opqueue: " + opqueue);
+ return;
+ }
+ }
+ }
+ }
+
+ }
+
+ }
+}