Improve config system shutdown 77/36777/8
authorTom Pantelis <tpanteli@brocade.com>
Thu, 24 Mar 2016 15:53:39 +0000 (11:53 -0400)
committerAnil Vishnoi <vishnoianil@gmail.com>
Thu, 7 Apr 2016 03:15:01 +0000 (03:15 +0000)
On shutdown, the ModuleFactoryBundleTracker is invoked for every removed
bundle which executes the "blank" transaction which in turn calls
ConfigRegistryImpl#beginConfig. This method is synchronized and may block
if a config commit is in progress (which might be waiting on capabilities or
a ModuleFactory). The ModuleFactoryBundleTracker is invoked
synchronously so this can block karaf shut down. It can affect single feature
tests in particular which start and stop karaf quickly.

I'm not exactly sure what the blank tx is for - I think its purpose is to
indirectly cause the deactivated bundle's module(s) to be destroyed. On
shutdown, the ConfigManagerActivator stop method calls ConfigRegistryImpl#close
which destroys all the modules so we really don't need the blank tx to
run on every bundle on shutdown.

On shutdown, karaf first sends a STOPPING event for the system bundle
(id 0). So I added the ConfigManagerActivator as a bundle listener and when
the system bundle 0 transitions to STOPPING, it calls ConfigRegistryImpl#close.
I also added a closed flag in ConfigRegistryImpl which is checked by
beginConfig and commitConfig. In order to make this work right, I had to change
the synchronization in ConfigRegistryImpl, which really was over-synchronized on
"this". I removed synchronized from the beginConfig and close methods to
break the locking between the two and I made ConfigHolder and TransactionsHolder
classes thread-safe by using ConcurrentHashMap which allows close to access them
safely w/o synchronization.  If the closed flag is set, beginConfig
returns a no-op ObjectName, otherwise it calls the synchronized beginConfigSafe
method. In commitConfig, if closed is set or the ON is the no-op ObjectName,
it returns an an empty CommitStatus.

I also changed from synchronizing "this" to using a Lock to take advantage
of the tryLock functionality. As an added measure of safety against
prolonged blocking, if beginConfig is called with the blank tx flag set,
it tries to acquire the lock for 5 sec. If it fails it returns the no-op
ObjectName.

After the modules are destroyed by ConfigRegistryImpl#close. the container
will proceed to stop the rest of the bundles and the ModuleFactoryBundleTracker
will initiate blank transactions but they'll be no-ops and won't block.

The ModuleFactoryBundleTracker was also changed to only initiate a blank
transaction for bundles that contain a ModuleFactory (via Boolean state
stored with the BundleTracker).

With these changes, karaf shuts down noticeably faster.

The BlankTransactionServiceTracker also initiates a blank transaction
when services are added and removed. Since these are synchronous calls
we really shouldn't block the OSGi container. So I added a
single-threaded executor to process the blank transactions for the
service changes. The blank transaction on bundle removed has to be done
synchronously as it may still need access to the BundleContext which
becomes invalid after the bundle removed event.

Change-Id: I5482caa4182dcc64df9b6bafefac9b8f2d505d3e
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigRegistryImpl.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigTransactionControllerImpl.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTracker.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTracker.java
opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTrackerTest.java
opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/osgi/ConfigPersisterActivator.java

index 45dac31..b58d2af 100644 (file)
@@ -18,8 +18,13 @@ import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 import javax.annotation.concurrent.GuardedBy;
-import javax.annotation.concurrent.NotThreadSafe;
 import javax.annotation.concurrent.ThreadSafe;
 import javax.management.InstanceAlreadyExistsException;
 import javax.management.InstanceNotFoundException;
@@ -60,22 +65,20 @@ import org.slf4j.LoggerFactory;
 @ThreadSafe
 public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBean {
     private static final Logger LOG = LoggerFactory.getLogger(ConfigRegistryImpl.class);
+    private static final ObjectName NOOP_TX_NAME = ObjectNameUtil.createTransactionControllerON("noop");
 
     private final ModuleFactoriesResolver resolver;
     private final MBeanServer configMBeanServer;
     private final BindingContextProvider bindingContextProvider;
 
-    @GuardedBy("this")
-    private long version = 0;
-    @GuardedBy("this")
-    private long versionCounter = 0;
+    private volatile long version = 0;
+    private volatile long versionCounter = 0;
 
     /**
      * Contains current configuration in form of {moduleName:{instanceName,read
      * only module}} for copying state to new transaction. Each running module
      * is present just once, no matter how many interfaces it exposes.
      */
-    @GuardedBy("this")
     private final ConfigHolder currentConfig = new ConfigHolder();
 
     /**
@@ -83,14 +86,12 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * validation but failed in second phase of commit. In this case the server
      * is unstable and its state is undefined.
      */
-    @GuardedBy("this")
-    private boolean isHealthy = true;
+    private volatile boolean isHealthy = true;
 
     /**
      * Holds Map<transactionName, transactionController> and purges it each time
      * its content is requested.
      */
-    @GuardedBy("this")
     private final TransactionsHolder transactionsHolder = new TransactionsHolder();
 
     private final BaseJMXRegistrator baseJMXRegistrator;
@@ -102,12 +103,19 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
     // internal jmx server shared by all transactions
     private final MBeanServer transactionsMBeanServer;
 
+    private final AtomicBoolean closed = new AtomicBoolean();
+
+    private final Object readableSRRegistryLock = new Object();
+
+    private final Lock configTransactionLock = new ReentrantLock();
+
     // Used for finding new factory instances for default module functionality
-    @GuardedBy("this")
+    @GuardedBy("configTransactionLock")
     private List<ModuleFactory> lastListOfFactories = Collections.emptyList();
 
-    @GuardedBy("this") // switched in every 2ndPC
-    private CloseableServiceReferenceReadableRegistry readableSRRegistry = ServiceReferenceRegistryImpl.createInitialSRLookupRegistry();
+    @GuardedBy("readableSRRegistryLock") // switched in every 2ndPC
+    private CloseableServiceReferenceReadableRegistry readableSRRegistry =
+            ServiceReferenceRegistryImpl.createInitialSRLookupRegistry();
 
     // constructor
     public ConfigRegistryImpl(ModuleFactoriesResolver resolver,
@@ -135,7 +143,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * Create new {@link ConfigTransactionControllerImpl }
      */
     @Override
-    public synchronized ObjectName beginConfig() {
+    public ObjectName beginConfig() {
         return beginConfig(false);
     }
 
@@ -143,11 +151,46 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * @param blankTransaction true if this transaction is created automatically by
      *                         org.opendaylight.controller.config.manager.impl.osgi.BlankTransactionServiceTracker
      */
-    public synchronized ObjectName beginConfig(boolean blankTransaction) {
-        return beginConfigInternal(blankTransaction).getControllerObjectName();
+    public ObjectName beginConfig(boolean blankTransaction) {
+        // If we're closed or in the process of closing then all modules are destroyed or being destroyed
+        // so there's no point in trying to acquire the lock and beginning an actual transaction. Also we want
+        // to avoid trying to lock as it may block the shutdown process if there is an outstanding transaction
+        // attempting to be committed.
+        //
+        // We could throw an exception here to indicate this but that's not part of the original API contract
+        // and callers may not be expecting an unchecked exception. Therefore we return a special transaction
+        // handle that signifies a "no-op".
+        if(closed.get()) {
+            return NOOP_TX_NAME;
+        }
+
+        if(blankTransaction) {
+            try {
+                // For a "blank" transaction, we'll try to obtain the config lock but "blank" transactions
+                // are initiated via OSGi events so we don't want to block indefinitely or for a long period
+                // of time.
+                if(!configTransactionLock.tryLock(5, TimeUnit.SECONDS)) {
+                    LOG.debug("Timed out trying to obtain configTransactionLock");
+                    return NOOP_TX_NAME;
+                }
+            } catch(InterruptedException e) {
+                LOG.debug("Interrupted trying to obtain configTransactionLock", e);
+                Thread.currentThread().interrupt();
+                return NOOP_TX_NAME;
+            }
+        } else {
+            configTransactionLock.lock();
+        }
+
+        try {
+            return beginConfigSafe(blankTransaction).getControllerObjectName();
+        } finally {
+            configTransactionLock.unlock();
+        }
     }
 
-    private synchronized ConfigTransactionControllerInternal beginConfigInternal(boolean blankTransaction) {
+    @GuardedBy("configTransactionLock")
+    private ConfigTransactionControllerInternal beginConfigSafe(boolean blankTransaction) {
         versionCounter++;
         final String transactionName = "ConfigTransaction-" + version + "-" + versionCounter;
 
@@ -196,7 +239,22 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * {@inheritDoc}
      */
     @Override
-    public synchronized CommitStatus commitConfig(ObjectName transactionControllerON)
+    public CommitStatus commitConfig(ObjectName transactionControllerON)
+            throws ConflictingVersionException, ValidationException {
+        if(transactionControllerON == NOOP_TX_NAME || closed.get()) {
+            return new CommitStatus(Collections.emptyList(), Collections.emptyList(), Collections.emptyList());
+        }
+
+        configTransactionLock.lock();
+        try {
+            return commitConfigSafe(transactionControllerON);
+        } finally {
+            configTransactionLock.unlock();
+        }
+    }
+
+    @GuardedBy("configTransactionLock")
+    private CommitStatus commitConfigSafe(ObjectName transactionControllerON)
             throws ConflictingVersionException, ValidationException {
         final String transactionName = ObjectNameUtil
                 .getTransactionName(transactionControllerON);
@@ -237,6 +295,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
         }
     }
 
+    @GuardedBy("configTransactionLock")
     private CommitStatus secondPhaseCommit(ConfigTransactionControllerInternal configTransactionController,
                                            CommitInfo commitInfo, ConfigTransactionLookupRegistry txLookupRegistry) {
 
@@ -395,9 +454,11 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
         version = configTransactionController.getVersion();
 
         // switch readable Service Reference Registry
-        this.readableSRRegistry.close();
-        this.readableSRRegistry = ServiceReferenceRegistryImpl.createSRReadableRegistry(
-                configTransactionController.getWritableRegistry(), this, baseJMXRegistrator);
+        synchronized(readableSRRegistryLock) {
+            readableSRRegistry.close();
+            readableSRRegistry = ServiceReferenceRegistryImpl.createSRReadableRegistry(
+                    configTransactionController.getWritableRegistry(), this, baseJMXRegistrator);
+        }
 
         return new CommitStatus(newInstances, reusedInstances,
                 recreatedInstances);
@@ -407,7 +468,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * {@inheritDoc}
      */
     @Override
-    public synchronized List<ObjectName> getOpenConfigs() {
+    public List<ObjectName> getOpenConfigs() {
         Map<String, Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = transactionsHolder
                 .getCurrentTransactions();
         List<ObjectName> result = new ArrayList<>(transactions.size());
@@ -425,7 +486,11 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * here.
      */
     @Override
-    public synchronized void close() {
+    public void close() {
+        if(!closed.compareAndSet(false, true)) {
+            return;
+        }
+
         // abort transactions
         Map<String, Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = transactionsHolder
                 .getCurrentTransactions();
@@ -437,14 +502,15 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
                 configTransactionControllerEntry.getValue().close();
                 configTransactionController.abortConfig();
             } catch (RuntimeException e) {
-                LOG.warn("Ignoring exception while aborting {}",
-                        configTransactionController, e);
+                LOG.debug("Ignoring exception while aborting {}", configTransactionController, e);
             }
         }
 
         // destroy all live objects one after another in order of the dependency hierarchy, from top to bottom
-        List<DestroyedModule> destroyedModules = currentConfig
-                .getModulesToBeDestroyed();
+        List<DestroyedModule> destroyedModules = currentConfig.getModulesToBeDestroyed();
+
+        LOG.info("ConfigRegistry closing - destroying {} modules", destroyedModules.size());
+
         for (DestroyedModule destroyedModule : destroyedModules) {
             destroyedModule.close();
         }
@@ -454,6 +520,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
         MBeanServerFactory.releaseMBeanServer(registryMBeanServer);
         MBeanServerFactory.releaseMBeanServer(transactionsMBeanServer);
 
+        LOG.info("ConfigRegistry closed");
     }
 
     /**
@@ -554,38 +621,52 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
 
     // service reference functionality:
     @Override
-    public synchronized ObjectName lookupConfigBeanByServiceInterfaceName(String serviceInterfaceQName, String refName) {
-        return readableSRRegistry.lookupConfigBeanByServiceInterfaceName(serviceInterfaceQName, refName);
+    public ObjectName lookupConfigBeanByServiceInterfaceName(String serviceInterfaceQName, String refName) {
+        synchronized(readableSRRegistryLock) {
+            return readableSRRegistry.lookupConfigBeanByServiceInterfaceName(serviceInterfaceQName, refName);
+        }
     }
 
     @Override
-    public synchronized Map<String, Map<String, ObjectName>> getServiceMapping() {
-        return readableSRRegistry.getServiceMapping();
+    public Map<String, Map<String, ObjectName>> getServiceMapping() {
+        synchronized(readableSRRegistryLock) {
+            return readableSRRegistry.getServiceMapping();
+        }
     }
 
     @Override
-    public synchronized Map<String, ObjectName> lookupServiceReferencesByServiceInterfaceName(String serviceInterfaceQName) {
-        return readableSRRegistry.lookupServiceReferencesByServiceInterfaceName(serviceInterfaceQName);
+    public Map<String, ObjectName> lookupServiceReferencesByServiceInterfaceName(String serviceInterfaceQName) {
+        synchronized(readableSRRegistryLock) {
+            return readableSRRegistry.lookupServiceReferencesByServiceInterfaceName(serviceInterfaceQName);
+        }
     }
 
     @Override
-    public synchronized Set<String> lookupServiceInterfaceNames(ObjectName objectName) throws InstanceNotFoundException {
-        return readableSRRegistry.lookupServiceInterfaceNames(objectName);
+    public Set<String> lookupServiceInterfaceNames(ObjectName objectName) throws InstanceNotFoundException {
+        synchronized(readableSRRegistryLock) {
+            return readableSRRegistry.lookupServiceInterfaceNames(objectName);
+        }
     }
 
     @Override
-    public synchronized String getServiceInterfaceName(String namespace, String localName) {
-        return readableSRRegistry.getServiceInterfaceName(namespace, localName);
+    public String getServiceInterfaceName(String namespace, String localName) {
+        synchronized(readableSRRegistryLock) {
+            return readableSRRegistry.getServiceInterfaceName(namespace, localName);
+        }
     }
 
     @Override
     public void checkServiceReferenceExists(ObjectName objectName) throws InstanceNotFoundException {
-        readableSRRegistry.checkServiceReferenceExists(objectName);
+        synchronized(readableSRRegistryLock) {
+            readableSRRegistry.checkServiceReferenceExists(objectName);
+        }
     }
 
     @Override
     public ObjectName getServiceReference(String serviceInterfaceQName, String refName) throws InstanceNotFoundException {
-        return readableSRRegistry.getServiceReference(serviceInterfaceQName, refName);
+        synchronized(readableSRRegistryLock) {
+            return readableSRRegistry.getServiceReference(serviceInterfaceQName, refName);
+        }
     }
 
     @Override
@@ -605,9 +686,8 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
 /**
  * Holds currently running modules
  */
-@NotThreadSafe
 class ConfigHolder {
-    private final Map<ModuleIdentifier, ModuleInternalInfo> currentConfig = new HashMap<>();
+    private final ConcurrentMap<ModuleIdentifier, ModuleInternalInfo> currentConfig = new ConcurrentHashMap<>();
 
     /**
      * Add all modules to the internal map. Also add service instance to OSGi
@@ -625,8 +705,7 @@ class ConfigHolder {
     }
 
     private void add(ModuleInternalInfo configInfo) {
-        ModuleInternalInfo oldValue = currentConfig.put(configInfo.getIdentifier(),
-                configInfo);
+        ModuleInternalInfo oldValue = currentConfig.putIfAbsent(configInfo.getIdentifier(), configInfo);
         if (oldValue != null) {
             throw new IllegalStateException(
                     "Cannot overwrite module with same name:"
@@ -665,7 +744,6 @@ class ConfigHolder {
  * Holds Map<transactionName, transactionController> and purges it each time its
  * content is requested.
  */
-@NotThreadSafe
 class TransactionsHolder {
     /**
      * This map keeps transaction names and
@@ -673,16 +751,12 @@ class TransactionsHolder {
      * MBeanServer transforms mbeans into another representation. Map is cleaned
      * every time current transactions are requested.
      */
-    @GuardedBy("ConfigRegistryImpl.this")
-    private final Map<String /* transactionName */,
-            Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = new HashMap<>();
+    private final ConcurrentMap<String /* transactionName */,
+            Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = new ConcurrentHashMap<>();
 
-    /**
-     * Can only be called from within synchronized method.
-     */
     public void add(String transactionName,
                     ConfigTransactionControllerInternal transactionController, ConfigTransactionLookupRegistry txLookupRegistry) {
-        Object oldValue = transactions.put(transactionName,
+        Object oldValue = transactions.putIfAbsent(transactionName,
                 Maps.immutableEntry(transactionController, txLookupRegistry));
         if (oldValue != null) {
             throw new IllegalStateException(
@@ -691,8 +765,7 @@ class TransactionsHolder {
     }
 
     /**
-     * Purges closed transactions from transactions map. Can only be called from
-     * within synchronized method. Calling this method more than once within the
+     * Purges closed transactions from transactions map. Calling this method more than once within the
      * method can modify the resulting map that was obtained in previous calls.
      *
      * @return current view on transactions map.
index b945bf6..ebf6266 100644 (file)
@@ -437,7 +437,7 @@ class ConfigTransactionControllerImpl implements
     }
 
     @Override
-    public synchronized void abortConfig() {
+    public void abortConfig() {
         transactionStatus.checkNotCommitStarted();
         transactionStatus.checkNotAborted();
         internalAbort();
index 2e2bf96..fa46cae 100644 (file)
@@ -8,6 +8,9 @@
 package org.opendaylight.controller.config.manager.impl.osgi;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import javax.management.ObjectName;
 import org.opendaylight.controller.config.api.ConflictingVersionException;
 import org.opendaylight.controller.config.api.ValidationException;
@@ -30,12 +33,14 @@ public class BlankTransactionServiceTracker implements ServiceTrackerCustomizer<
     public static final int DEFAULT_MAX_ATTEMPTS = 10;
 
     private final BlankTransaction blankTransaction;
-    private int maxAttempts;
+    private final ExecutorService txExecutor;
+    private final int maxAttempts;
 
     public BlankTransactionServiceTracker(final ConfigRegistryImpl configRegistry) {
         this(new BlankTransaction() {
             @Override
-            public CommitStatus hit() throws ValidationException, ConflictingVersionException {
+            public CommitStatus hit()
+                    throws ValidationException, ConflictingVersionException {
                 ObjectName tx = configRegistry.beginConfig(true);
                 return configRegistry.commitConfig(tx);
             }
@@ -43,22 +48,29 @@ public class BlankTransactionServiceTracker implements ServiceTrackerCustomizer<
     }
 
     public BlankTransactionServiceTracker(final BlankTransaction blankTransaction) {
-        this(blankTransaction, DEFAULT_MAX_ATTEMPTS);
+        this(blankTransaction, DEFAULT_MAX_ATTEMPTS, Executors.newSingleThreadExecutor(new ThreadFactoryBuilder()
+                .setNameFormat("config-blank-txn-%d").build()));
     }
 
     @VisibleForTesting
-    BlankTransactionServiceTracker(final BlankTransaction blankTx, final int maxAttempts) {
+    BlankTransactionServiceTracker(final BlankTransaction blankTx, final int maxAttempts,
+            final ExecutorService txExecutor) {
         this.blankTransaction = blankTx;
         this.maxAttempts = maxAttempts;
+        this.txExecutor = txExecutor;
     }
 
     @Override
     public Object addingService(ServiceReference<ModuleFactory> moduleFactoryServiceReference) {
-        blankTransaction();
+        blankTransactionAsync();
         return null;
     }
 
-    synchronized void blankTransaction() {
+    private void blankTransactionAsync() {
+        txExecutor.execute(() -> { blankTransactionSync(); });
+    }
+
+    void blankTransactionSync() {
         // race condition check: config-persister might push new configuration while server is starting up.
         ConflictingVersionException lastException = null;
         for (int i = 0; i < maxAttempts; i++) {
@@ -73,25 +85,26 @@ public class BlankTransactionServiceTracker implements ServiceTrackerCustomizer<
                     Thread.sleep(1000);
                 } catch (InterruptedException interruptedException) {
                     Thread.currentThread().interrupt();
-                    throw new IllegalStateException(interruptedException);
+                    LOG.debug("blankTransactionSync was interrupted");
+                    return;
                 }
             } catch (ValidationException e) {
                 LOG.error("Validation exception while running blank transaction indicates programming error", e);
-                throw new RuntimeException("Validation exception while running blank transaction indicates programming error", e);
             }
         }
-        throw new RuntimeException("Maximal number of attempts reached and still cannot get optimistic lock from " +
-                "config manager",lastException);
+
+        LOG.error("Maximal number of attempts reached and still cannot get optimistic lock from config manager",
+                lastException);
     }
 
     @Override
     public void modifiedService(ServiceReference <ModuleFactory> moduleFactoryServiceReference, Object o) {
-        blankTransaction();
+        blankTransactionAsync();
     }
 
     @Override
     public void removedService(ServiceReference<ModuleFactory> moduleFactoryServiceReference, Object o) {
-        blankTransaction();
+        blankTransactionAsync();
     }
 
     @VisibleForTesting
index 50ab13e..38aeb9a 100644 (file)
@@ -31,19 +31,25 @@ import org.opendaylight.yangtools.yang.binding.YangModuleInfo;
 import org.osgi.framework.Bundle;
 import org.osgi.framework.BundleActivator;
 import org.osgi.framework.BundleContext;
+import org.osgi.framework.BundleEvent;
+import org.osgi.framework.SynchronousBundleListener;
 import org.osgi.util.tracker.BundleTracker;
 import org.osgi.util.tracker.ServiceTracker;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-public class ConfigManagerActivator implements BundleActivator {
+public class ConfigManagerActivator implements BundleActivator, SynchronousBundleListener {
 
     private static final Logger LOG = LoggerFactory.getLogger(ConfigManagerActivator.class);
 
+    private static final long SYSTEM_BUNDLE_ID = 0;
+
     private final MBeanServer configMBeanServer = ManagementFactory.getPlatformMBeanServer();
 
     private AutoCloseable autoCloseable;
 
+    private ConfigRegistryImpl configRegistry;
+
     @Override
     public void start(final BundleContext context) {
         try {
@@ -59,7 +65,7 @@ public class ConfigManagerActivator implements BundleActivator {
             // start config registry
             BundleContextBackedModuleFactoriesResolver bundleContextBackedModuleFactoriesResolver = new BundleContextBackedModuleFactoriesResolver(
                     context);
-            ConfigRegistryImpl configRegistry = new ConfigRegistryImpl(bundleContextBackedModuleFactoriesResolver, configMBeanServer,
+            configRegistry = new ConfigRegistryImpl(bundleContextBackedModuleFactoriesResolver, configMBeanServer,
                     bindingContextProvider);
 
             // track bundles containing factories
@@ -117,10 +123,12 @@ public class ConfigManagerActivator implements BundleActivator {
                     blankTransactionServiceTracker);
             serviceTracker.open();
 
-            List<AutoCloseable> list = Arrays.asList(bindingContextProvider, clsReg, configRegistry,
+            List<AutoCloseable> list = Arrays.asList(bindingContextProvider, clsReg,
                     wrap(moduleFactoryBundleTracker), moduleInfoBundleTracker,
                     configRegReg, configRegistryJMXRegistrator, configRegistryJMXRegistratorWithNotifications, wrap(serviceTracker), moduleInfoRegistryWrapper, notifyingConfigRegistry);
             autoCloseable = OsgiRegistrationUtil.aggregate(list);
+
+            context.addBundleListener(this);
         } catch(Exception e) {
             LOG.warn("Error starting config manager", e);
         } catch(Error e) {
@@ -133,6 +141,20 @@ public class ConfigManagerActivator implements BundleActivator {
 
     @Override
     public void stop(final BundleContext context) throws Exception {
+        context.removeBundleListener(this);
         autoCloseable.close();
     }
+
+    @Override
+    public void bundleChanged(BundleEvent event) {
+        if(configRegistry == null) {
+            return;
+        }
+
+        // If the system bundle (id 0) is stopping close the ConfigRegistry so it destroys all modules. On
+        // shutdown the system bundle is stopped first.
+        if(event.getBundle().getBundleId() == SYSTEM_BUNDLE_ID && event.getType() == BundleEvent.STOPPING) {
+            configRegistry.close();
+        }
+    }
 }
index cd72a73..15e6255 100644 (file)
@@ -30,7 +30,7 @@ import org.slf4j.LoggerFactory;
  * the services are unregistered automatically.
  * Code based on http://www.toedter.com/blog/?p=236
  */
-public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer<Object> {
+public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer<Boolean> {
     private final BlankTransactionServiceTracker blankTransactionServiceTracker;
     private static final Logger LOG = LoggerFactory.getLogger(ModuleFactoryBundleTracker.class);
 
@@ -39,7 +39,7 @@ public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer<Objec
     }
 
     @Override
-    public Object addingBundle(Bundle bundle, BundleEvent event) {
+    public Boolean addingBundle(Bundle bundle, BundleEvent event) {
         URL resource = bundle.getEntry("META-INF/services/" + ModuleFactory.class.getName());
         LOG.trace("Got addingBundle event of bundle {}, resource {}, event {}",
                 bundle, resource, event);
@@ -48,23 +48,28 @@ public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer<Objec
                 for (String factoryClassName : Resources.readLines(resource, Charsets.UTF_8)) {
                     registerFactory(factoryClassName, bundle);
                 }
+
+                return Boolean.TRUE;
             } catch (IOException e) {
                 LOG.error("Error while reading {}", resource, e);
                 throw new RuntimeException(e);
             }
         }
-        return bundle;
+
+        return Boolean.FALSE;
     }
 
     @Override
-    public void modifiedBundle(Bundle bundle, BundleEvent event, Object object) {
+    public void modifiedBundle(Bundle bundle, BundleEvent event, Boolean hasFactory) {
         // NOOP
     }
 
     @Override
-    public void removedBundle(Bundle bundle, BundleEvent event, Object object) {
-        // workaround for service tracker not getting removed service event
-        blankTransactionServiceTracker.blankTransaction();
+    public void removedBundle(Bundle bundle, BundleEvent event, Boolean hasFactory) {
+        if(hasFactory) {
+            // workaround for service tracker not getting removed service event
+            blankTransactionServiceTracker.blankTransactionSync();
+        }
     }
 
     @VisibleForTesting
index 4d342a6..47b6d10 100644 (file)
@@ -8,15 +8,12 @@
 
 package org.opendaylight.controller.config.manager.impl.osgi;
 
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertSame;
-import static org.junit.Assert.fail;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
-
+import com.google.common.util.concurrent.MoreExecutors;
 import java.util.Collections;
 import javax.management.ObjectName;
 import org.junit.Before;
@@ -31,7 +28,6 @@ import org.opendaylight.controller.config.spi.ModuleFactory;
 import org.osgi.framework.ServiceReference;
 
 public class BlankTransactionServiceTrackerTest {
-
     @Mock
     private BlankTransactionServiceTracker.BlankTransaction blankTx;
     private BlankTransactionServiceTracker tracker;
@@ -39,8 +35,9 @@ public class BlankTransactionServiceTrackerTest {
     @Before
     public void setUp() throws Exception {
         MockitoAnnotations.initMocks(this);
-        doReturn(new CommitStatus(Collections.<ObjectName>emptyList(), Collections.<ObjectName>emptyList(), Collections.<ObjectName>emptyList())).when(blankTx).hit();
-        tracker = new BlankTransactionServiceTracker(blankTx);
+        doReturn(new CommitStatus(Collections.<ObjectName>emptyList(), Collections.<ObjectName>emptyList(),
+                Collections.<ObjectName>emptyList())).when(blankTx).hit();
+        tracker = new BlankTransactionServiceTracker(blankTx, 10, MoreExecutors.newDirectExecutorService());
     }
 
     @Test
@@ -56,33 +53,21 @@ public class BlankTransactionServiceTrackerTest {
         IllegalArgumentException argumentException = new IllegalArgumentException();
         ValidationException validationException = ValidationException.createForSingleException(new ModuleIdentifier("m", "i"), argumentException);
         doThrow(validationException).when(blankTx).hit();
-        try {
-            tracker.addingService(getMockServiceReference());
-        } catch (Exception e) {
-            verify(blankTx, times(1)).hit();
-            assertNotNull(e.getCause());
-            assertSame(validationException, e.getCause());
-            return;
-        }
 
-        fail("Exception should have occurred for validation exception");
+        tracker.addingService(getMockServiceReference());
+        verify(blankTx, times(10)).hit();
     }
 
     @Test
     public void testConflictingException() throws Exception {
         int maxAttempts = 2;
-        tracker = new BlankTransactionServiceTracker(blankTx, maxAttempts);
+        tracker = new BlankTransactionServiceTracker(blankTx, maxAttempts, MoreExecutors.newDirectExecutorService());
 
         final ConflictingVersionException ex = new ConflictingVersionException();
         doThrow(ex).when(blankTx).hit();
-        try {
-            tracker.addingService(getMockServiceReference());
-        } catch (Exception e) {
-            verify(blankTx, times(maxAttempts)).hit();
-            return;
-        }
 
-        fail("Exception should have occurred for conflicting exception");
+        tracker.addingService(getMockServiceReference());
+        verify(blankTx, times(maxAttempts)).hit();
     }
 
     private ServiceReference<ModuleFactory> getMockServiceReference() {
index d5df534..66c43c2 100644 (file)
@@ -133,7 +133,7 @@ public class ConfigPersisterActivator implements BundleActivator {
                             LOG.warn("Unable to process configs as BundleContext is null");
                         }
                     } catch (InterruptedException e) {
-                        LOG.info("ConfigPusher thread stopped",e);
+                        LOG.info("ConfigPusher thread stopped");
                     }
                     LOG.info("Configuration Persister initialization completed.");
                 }