FELIX-2813 Make the service registration field volatile and ensure the asynchronous threads are started only after registering the service and stopped before unregistering the service.
git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1066033 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java b/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java
index 52abff7..9786d2c 100644
--- a/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java
+++ b/configadmin/src/main/java/org/apache/felix/cm/impl/ConfigurationManager.java
@@ -102,7 +102,7 @@
private BundleContext bundleContext;
// the service registration of the configuration admin
- private ServiceRegistration configurationAdminRegistration;
+ private volatile ServiceRegistration configurationAdminRegistration;
// the ServiceTracker to emit log services (see log(int, String, Throwable))
private ServiceTracker logTracker;
@@ -241,6 +241,11 @@
props.put( Constants.SERVICE_VENDOR, "Apache Software Foundation" );
configurationAdminRegistration = bundleContext.registerService( ConfigurationAdmin.class.getName(), caf, props );
+ // start processing the event queues only after registering the service
+ // see FELIX-2813 for details
+ this.updateThread.start();
+ this.eventThread.start();
+
// start handling ManagedService[Factory] services
managedServiceTracker = new ManagedServiceTracker(this);
managedServiceFactoryTracker = new ManagedServiceFactoryTracker(this);
@@ -253,6 +258,17 @@
// stop handling bundle events immediately
handleBundleEvents = false;
+ // stop queue processing before unregistering the service
+ // see FELIX-2813 for details
+ if ( updateThread != null )
+ {
+ updateThread.terminate();
+ }
+ if ( eventThread != null )
+ {
+ eventThread.terminate();
+ }
+
// immediately unregister the Configuration Admin before cleaning up
// clearing the field before actually unregistering the service
// prevents IllegalStateException in getServiceReference() if
@@ -278,16 +294,6 @@
configurationListenerTracker.close();
}
- if ( updateThread != null )
- {
- updateThread.terminate();
- }
-
- if ( eventThread != null )
- {
- eventThread.terminate();
- }
-
if ( logTracker != null )
{
logTracker.close();
diff --git a/configadmin/src/main/java/org/apache/felix/cm/impl/UpdateThread.java b/configadmin/src/main/java/org/apache/felix/cm/impl/UpdateThread.java
index f68cc6c..edf2e4a 100644
--- a/configadmin/src/main/java/org/apache/felix/cm/impl/UpdateThread.java
+++ b/configadmin/src/main/java/org/apache/felix/cm/impl/UpdateThread.java
@@ -35,6 +35,9 @@
// (this is mainly used for logging)
private final ConfigurationManager configurationManager;
+ // the thread group into which the worker thread will be placed
+ private final ThreadGroup workerThreadGroup;
+
// the thread's base name
private final String workerBaseName;
@@ -42,18 +45,16 @@
private final LinkedList updateTasks;
// the actual thread
- private final Thread worker;
+ private Thread worker;
public UpdateThread( final ConfigurationManager configurationManager, final ThreadGroup tg, final String name )
{
this.configurationManager = configurationManager;
+ this.workerThreadGroup = tg;
this.workerBaseName = name;
this.updateTasks = new LinkedList();
- this.worker = new Thread( tg, this, name );
- this.worker.setDaemon( true );
- this.worker.start();
}
@@ -94,7 +95,7 @@
try
{
// set the thread name indicating the current task
- worker.setName( workerBaseName + " (" + task + ")" );
+ Thread.currentThread().setName( workerBaseName + " (" + task + ")" );
if ( configurationManager.isLogEnabled( LogService.LOG_DEBUG ) )
{
@@ -110,26 +111,67 @@
finally
{
// reset the thread name to "idle"
- worker.setName( workerBaseName );
+ Thread.currentThread().setName( workerBaseName );
}
}
}
-
- // cause this thread to terminate by adding this thread to the end
- // of the queue and wait for the thread to actually terminate
- void terminate()
+ /**
+ * Starts processing the queued tasks. This method does nothing if the
+ * worker has already been started.
+ */
+ synchronized void start()
{
- schedule( this );
-
- // wait for all updates to terminate
- try
+ if ( this.worker == null )
{
- worker.join();
+ Thread workerThread = new Thread( workerThreadGroup, this, workerBaseName );
+ workerThread.setDaemon( true );
+ workerThread.start();
+ this.worker = workerThread;
}
- catch ( InterruptedException ie )
+ }
+
+
+ /**
+ * Terminates the worker thread and waits for the thread to have processed
+ * all outstanding events up to and including the termination job. All
+ * jobs {@link #schedule(Runnable) scheduled} after termination has been
+ * initiated will not be processed any more. This method does nothing if
+ * the worker thread is not currently active.
+ * <p>
+ * If the worker thread does not terminate within 5 seconds it is killed
+ * by calling the (deprecated) <code>Thread.stop()</code> method. It may
+ * be that the worker thread may be blocked by a deadlock (it should not,
+ * though). In this case hope is that <code>Thread.stop()</code> will be
+ * able to released that deadlock at the expense of one or more tasks to
+ * not be executed any longer.... In any case an ERROR message is logged
+ * with the LogService in this situation.
+ */
+ synchronized void terminate()
+ {
+ if ( this.worker != null )
{
- // don't really care
+ Thread workerThread = this.worker;
+ this.worker = null;
+
+ schedule( this );
+
+ // wait for all updates to terminate (<= 10 seconds !)
+ try
+ {
+ workerThread.join( 5000 );
+ }
+ catch ( InterruptedException ie )
+ {
+ // don't really care
+ }
+
+ if ( workerThread.isAlive() )
+ {
+ this.configurationManager.log( LogService.LOG_ERROR, "Worker thread " + workerBaseName
+ + " did not terminate within 5 seconds; trying to kill", null );
+ workerThread.stop();
+ }
}
}
diff --git a/configadmin/src/test/java/org/apache/felix/cm/integration/FELIX2813_ConfigurationAdminStartupTest.java b/configadmin/src/test/java/org/apache/felix/cm/integration/FELIX2813_ConfigurationAdminStartupTest.java
index 0efb84f..b52f976 100644
--- a/configadmin/src/test/java/org/apache/felix/cm/integration/FELIX2813_ConfigurationAdminStartupTest.java
+++ b/configadmin/src/test/java/org/apache/felix/cm/integration/FELIX2813_ConfigurationAdminStartupTest.java
@@ -1,19 +1,17 @@
package org.apache.felix.cm.integration;
-import static org.ops4j.pax.exam.CoreOptions.mavenBundle;
-import static org.ops4j.pax.exam.CoreOptions.options;
-import static org.ops4j.pax.exam.CoreOptions.provision;
import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Properties;
+import junit.framework.TestCase;
+
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.ops4j.pax.exam.Option;
-import org.ops4j.pax.exam.junit.Configuration;
import org.ops4j.pax.exam.junit.JUnit4TestRunner;
import org.osgi.framework.Bundle;
-import org.osgi.framework.BundleContext;
import org.osgi.framework.BundleException;
import org.osgi.framework.Constants;
import org.osgi.framework.InvalidSyntaxException;
@@ -24,59 +22,116 @@
import org.osgi.service.cm.ConfigurationEvent;
import org.osgi.service.cm.ConfigurationListener;
-@RunWith(JUnit4TestRunner.class)
-public class FELIX2813_ConfigurationAdminStartupTest implements ServiceListener, ConfigurationListener {
- private volatile BundleContext m_context;
- @Configuration
- public static Option[] configuration() {
- return options(
- provision(
- mavenBundle().groupId("org.apache.felix").artifactId("org.apache.felix.configadmin").version("1.2.8").noStart()
- )
- );
- }
+@RunWith(JUnit4TestRunner.class)
+public class FELIX2813_ConfigurationAdminStartupTest extends ConfigurationTestBase implements ServiceListener,
+ ConfigurationListener
+{
+
+ private Object lock = new Object();
+ private boolean eventSeen;
+
@Test
- public void testAddConfigurationWhenConfigurationAdminStarts(BundleContext context) throws InvalidSyntaxException, BundleException {
- m_context = context;
- m_context.registerService(ConfigurationListener.class.getName(), this, null);
- m_context.addServiceListener(this, "(" + Constants.OBJECTCLASS + "=" + ConfigurationAdmin.class.getName() + ")");
- Bundle[] bundles = m_context.getBundles();
- for (Bundle b : bundles) {
- if (b.getSymbolicName().equals("org.apache.felix.configadmin")) {
- b.start();
+ public void testAddConfigurationWhenConfigurationAdminStarts() throws InvalidSyntaxException, BundleException
+ {
+
+ List<Bundle> bundles = new ArrayList<Bundle>();
+ ServiceReference[] refs = configAdminTracker.getServiceReferences();
+ if ( refs != null )
+ {
+ for ( ServiceReference ref : refs )
+ {
+ bundles.add( ref.getBundle() );
+ ref.getBundle().stop();
}
}
-
+
+ bundleContext.registerService( ConfigurationListener.class.getName(), this, null );
+ bundleContext.addServiceListener( this, "(" + Constants.OBJECTCLASS + "=" + ConfigurationAdmin.class.getName()
+ + ")" );
+
+ // ensure we do not have a false positive below
+ eventSeen = false;
+
+ for ( Bundle bundle : bundles )
+ {
+ bundle.start();
+ }
+
/*
* Look at the console output for the following exception:
- *
+ *
* *ERROR* Unexpected problem executing task
* java.lang.NullPointerException: reference and pid must not be null
* at org.osgi.service.cm.ConfigurationEvent.<init>(ConfigurationEvent.java:120)
* at org.apache.felix.cm.impl.ConfigurationManager$FireConfigurationEvent.run(ConfigurationManager.java:1818)
* at org.apache.felix.cm.impl.UpdateThread.run(UpdateThread.java:104)
* at java.lang.Thread.run(Thread.java:680)
- *
+ *
* It is in fact the service reference that is still null, because the service registration
* has not been 'set' yet.
+ *
+ * This following code will ensure the situation did not occurr and the
+ * event has effectively been sent. The eventSeen flag is set by the
+ * configurationEvent method when the event for the test PID has been
+ * received. If the flag is not set, we wait at most 2 seconds for the
+ * event to arrive. If the event does not arrive by then, the test is
+ * assumed to have failed. This will rather generate false negatives
+ * (on slow machines) than false positives.
*/
+ synchronized ( lock )
+ {
+ if ( !eventSeen )
+ {
+ try
+ {
+ lock.wait( 2000 );
+ }
+ catch ( InterruptedException ie )
+ {
+ // don't care ...
+ }
+ }
+
+ if ( !eventSeen )
+ {
+ TestCase.fail( "ConfigurationEvent not received within 2 seconds since bundle start" );
+ }
+ }
+
}
- public void serviceChanged(ServiceEvent event) {
- if (event.getType() == ServiceEvent.REGISTERED) {
+
+ public void serviceChanged( ServiceEvent event )
+ {
+ if ( event.getType() == ServiceEvent.REGISTERED )
+ {
ServiceReference ref = event.getServiceReference();
- ConfigurationAdmin ca = (ConfigurationAdmin) m_context.getService(ref);
- try {
- org.osgi.service.cm.Configuration config = ca.getConfiguration("test");
- config.update(new Properties() {{ put("abc", "123"); }});
+ ConfigurationAdmin ca = ( ConfigurationAdmin ) bundleContext.getService( ref );
+ try
+ {
+ org.osgi.service.cm.Configuration config = ca.getConfiguration( "test" );
+ Properties props = new Properties();
+ props.put( "abc", "123" );
+ config.update( props );
}
- catch (IOException e) {
+ catch ( IOException e )
+ {
}
}
}
- public void configurationEvent(ConfigurationEvent event) {
+
+ public void configurationEvent( ConfigurationEvent event )
+ {
+ if ( "test".equals( event.getPid() ) )
+ {
+ synchronized ( lock )
+ {
+ eventSeen = true;
+ lock.notifyAll();
+ }
+ }
}
}