Fixed few sonar warnings.
[controller.git] / opendaylight / config / config-manager / src / main / java / org / opendaylight / controller / config / manager / impl / ConfigRegistryImpl.java
index e914162671e811682271d42bcbccbe4e89c47947..763ed66cb10d947f4ef5aa95d4a6178acf8d009e 100644 (file)
@@ -7,12 +7,36 @@
  */
 package org.opendaylight.controller.config.manager.impl;
 
+import com.google.common.base.Throwables;
 import com.google.common.collect.Maps;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.LinkedList;
+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.ThreadSafe;
+import javax.management.InstanceAlreadyExistsException;
+import javax.management.InstanceNotFoundException;
+import javax.management.MBeanServer;
+import javax.management.MBeanServerFactory;
+import javax.management.ObjectName;
 import org.opendaylight.controller.config.api.ConflictingVersionException;
 import org.opendaylight.controller.config.api.ModuleIdentifier;
 import org.opendaylight.controller.config.api.RuntimeBeanRegistratorAwareModule;
-import org.opendaylight.controller.config.api.ServiceReferenceWritableRegistry;
 import org.opendaylight.controller.config.api.ValidationException;
+import org.opendaylight.controller.config.api.annotations.ServiceInterfaceAnnotation;
 import org.opendaylight.controller.config.api.jmx.CommitStatus;
 import org.opendaylight.controller.config.api.jmx.ObjectNameUtil;
 import org.opendaylight.controller.config.manager.impl.dependencyresolver.DestroyedModule;
@@ -26,57 +50,36 @@ import org.opendaylight.controller.config.manager.impl.jmx.RootRuntimeBeanRegist
 import org.opendaylight.controller.config.manager.impl.jmx.TransactionJMXRegistrator;
 import org.opendaylight.controller.config.manager.impl.osgi.BeanToOsgiServiceManager;
 import org.opendaylight.controller.config.manager.impl.osgi.BeanToOsgiServiceManager.OsgiRegistration;
+import org.opendaylight.controller.config.manager.impl.osgi.mapping.BindingContextProvider;
 import org.opendaylight.controller.config.manager.impl.util.LookupBeansUtil;
 import org.opendaylight.controller.config.manager.impl.util.ModuleQNameUtil;
 import org.opendaylight.controller.config.spi.Module;
 import org.opendaylight.controller.config.spi.ModuleFactory;
-import org.opendaylight.yangtools.yang.data.impl.codec.CodecRegistry;
 import org.osgi.framework.BundleContext;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import javax.annotation.concurrent.GuardedBy;
-import javax.annotation.concurrent.NotThreadSafe;
-import javax.annotation.concurrent.ThreadSafe;
-import javax.management.InstanceAlreadyExistsException;
-import javax.management.InstanceNotFoundException;
-import javax.management.MBeanServer;
-import javax.management.MBeanServerFactory;
-import javax.management.ObjectName;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.LinkedList;
-import java.util.List;
-import java.util.Map;
-import java.util.Map.Entry;
-import java.util.Set;
-
 /**
  * Singleton that is responsible for creating and committing Config
  * Transactions. It is registered in Platform MBean Server.
  */
 @ThreadSafe
 public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBean {
-    private static final Logger logger = LoggerFactory.getLogger(ConfigRegistryImpl.class);
+    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 CodecRegistry codecRegistry;
+    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();
 
     /**
@@ -84,14 +87,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;
@@ -103,29 +104,37 @@ 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();
+    // switched in every 2ndPC
+    @GuardedBy("readableSRRegistryLock")
+    private CloseableServiceReferenceReadableRegistry readableSRRegistry =
+            ServiceReferenceRegistryImpl.createInitialSRLookupRegistry();
 
     // constructor
     public ConfigRegistryImpl(ModuleFactoriesResolver resolver,
-                              MBeanServer configMBeanServer, CodecRegistry codecRegistry) {
+                              MBeanServer configMBeanServer, BindingContextProvider bindingContextProvider) {
         this(resolver, configMBeanServer,
-                new BaseJMXRegistrator(configMBeanServer), codecRegistry);
+                new BaseJMXRegistrator(configMBeanServer), bindingContextProvider);
     }
 
     // constructor
     public ConfigRegistryImpl(ModuleFactoriesResolver resolver,
                               MBeanServer configMBeanServer,
-                              BaseJMXRegistrator baseJMXRegistrator, CodecRegistry codecRegistry) {
+                              BaseJMXRegistrator baseJMXRegistrator, BindingContextProvider bindingContextProvider) {
         this.resolver = resolver;
         this.beanToOsgiServiceManager = new BeanToOsgiServiceManager();
         this.configMBeanServer = configMBeanServer;
         this.baseJMXRegistrator = baseJMXRegistrator;
-        this.codecRegistry = codecRegistry;
+        this.bindingContextProvider = bindingContextProvider;
         this.registryMBeanServer = MBeanServerFactory
                 .createMBeanServer("ConfigRegistry" + configMBeanServer.getDefaultDomain());
         this.transactionsMBeanServer = MBeanServerFactory
@@ -136,7 +145,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * Create new {@link ConfigTransactionControllerImpl }
      */
     @Override
-    public synchronized ObjectName beginConfig() {
+    public ObjectName beginConfig() {
         return beginConfig(false);
     }
 
@@ -144,11 +153,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;
 
@@ -159,17 +203,28 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
             }
         };
 
-        Map<String, Map.Entry<ModuleFactory, BundleContext>> allCurrentFactories = Collections.unmodifiableMap(
+        Map<String, Map.Entry<ModuleFactory, BundleContext>> allCurrentFactories = new HashMap<>(
                 resolver.getAllFactories());
 
+        // add all factories that disappeared from SR but are still committed
+        for (ModuleInternalInfo moduleInternalInfo : currentConfig.getEntries()) {
+            String name = moduleInternalInfo.getModuleFactory().getImplementationName();
+            if (!allCurrentFactories.containsKey(name)) {
+                LOG.trace("Factory {} not found in SR, using reference from previous commit", name);
+                allCurrentFactories.put(name,
+                        Maps.immutableEntry(moduleInternalInfo.getModuleFactory(), moduleInternalInfo.getBundleContext()));
+            }
+        }
+        allCurrentFactories = Collections.unmodifiableMap(allCurrentFactories);
+
         // closed by transaction controller
         ConfigTransactionLookupRegistry txLookupRegistry = new ConfigTransactionLookupRegistry(new TransactionIdentifier(
                 transactionName), factory, allCurrentFactories);
-        ServiceReferenceWritableRegistry writableRegistry = ServiceReferenceRegistryImpl.createSRWritableRegistry(
+        SearchableServiceReferenceWritableRegistry writableRegistry = ServiceReferenceRegistryImpl.createSRWritableRegistry(
                 readableSRRegistry, txLookupRegistry, allCurrentFactories);
 
         ConfigTransactionControllerInternal transactionController = new ConfigTransactionControllerImpl(
-                txLookupRegistry, version, codecRegistry,
+                txLookupRegistry, version, bindingContextProvider,
                 versionCounter, allCurrentFactories, transactionsMBeanServer,
                 configMBeanServer, blankTransaction, writableRegistry);
         try {
@@ -184,14 +239,29 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
 
     /**
      * {@inheritDoc}
+     * @throws ConflictingVersionException
      */
     @Override
-    @SuppressWarnings("PMD.AvoidCatchingThrowable")
-    public synchronized CommitStatus commitConfig(ObjectName transactionControllerON)
+    public CommitStatus commitConfig(ObjectName transactionControllerON)
+            throws ValidationException, ConflictingVersionException {
+        if(NOOP_TX_NAME.equals(transactionControllerON) || 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);
-        logger.trace("About to commit {}. Current parentVersion: {}, versionCounter {}", transactionName, version, versionCounter);
+        LOG.trace("About to commit {}. Current parentVersion: {}, versionCounter {}", transactionName, version, versionCounter);
 
         // find ConfigTransactionController
         Map<String, Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = transactionsHolder.getCurrentTransactions();
@@ -216,20 +286,16 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
         // non recoverable from here:
         try {
             return secondPhaseCommit(configTransactionController, commitInfo, configTransactionControllerEntry.getValue());
-        } catch (Throwable t) { // some libs throw Errors: e.g.
+            // some libs throw Errors: e.g.
             // javax.xml.ws.spi.FactoryFinder$ConfigurationError
+        } catch (Throwable t) {
             isHealthy = false;
-            logger.error("Configuration Transaction failed on 2PC, server is unhealthy", t);
-            if (t instanceof RuntimeException) {
-                throw (RuntimeException) t;
-            } else if (t instanceof Error) {
-                throw (Error) t;
-            } else {
-                throw new RuntimeException(t);
-            }
+            LOG.error("Configuration Transaction failed on 2PC, server is unhealthy", t);
+            throw Throwables.propagate(t);
         }
     }
 
+    @GuardedBy("configTransactionLock")
     private CommitStatus secondPhaseCommit(ConfigTransactionControllerInternal configTransactionController,
                                            CommitInfo commitInfo, ConfigTransactionLookupRegistry txLookupRegistry) {
 
@@ -237,36 +303,44 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
         // (hopefully) runtime beans
         for (DestroyedModule toBeDestroyed : commitInfo
                 .getDestroyedFromPreviousTransactions()) {
-            toBeDestroyed.close(); // closes instance (which should close
+            // closes instance (which should close
             // runtime jmx registrator),
             // also closes osgi registration and ModuleJMXRegistrator
             // registration
+            toBeDestroyed.close();
             currentConfig.remove(toBeDestroyed.getIdentifier());
         }
 
         // set RuntimeBeanRegistrators on beans implementing
-        // RuntimeBeanRegistratorAwareModule, each module
-        // should have exactly one runtime jmx registrator.
+        // RuntimeBeanRegistratorAwareModule
         Map<ModuleIdentifier, RootRuntimeBeanRegistratorImpl> runtimeRegistrators = new HashMap<>();
         for (ModuleInternalTransactionalInfo entry : commitInfo.getCommitted()
                 .values()) {
-            RootRuntimeBeanRegistratorImpl runtimeBeanRegistrator;
-            if (entry.hasOldModule() == false) {
-                runtimeBeanRegistrator = baseJMXRegistrator
-                        .createRuntimeBeanRegistrator(entry.getIdentifier());
-            } else {
-                // reuse old JMX registrator
-                runtimeBeanRegistrator = entry.getOldInternalInfo()
-                        .getRuntimeBeanRegistrator();
-            }
             // set runtime jmx registrator if required
             Module module = entry.getProxiedModule();
+            RootRuntimeBeanRegistratorImpl runtimeBeanRegistrator = null;
+
             if (module instanceof RuntimeBeanRegistratorAwareModule) {
-                ((RuntimeBeanRegistratorAwareModule) module)
-                        .setRuntimeBeanRegistrator(runtimeBeanRegistrator);
+
+                if(entry.hasOldModule()) {
+
+                    if(module.canReuse(entry.getOldInternalInfo().getReadableModule().getModule())) {
+                        runtimeBeanRegistrator = entry.getOldInternalInfo().getRuntimeBeanRegistrator();
+                        ((RuntimeBeanRegistratorAwareModule) module).setRuntimeBeanRegistrator(runtimeBeanRegistrator);
+                    } else {
+                        runtimeBeanRegistrator = baseJMXRegistrator.createRuntimeBeanRegistrator(entry.getIdentifier());
+                        entry.getOldInternalInfo().getRuntimeBeanRegistrator().close();
+                        ((RuntimeBeanRegistratorAwareModule) module).setRuntimeBeanRegistrator(runtimeBeanRegistrator);
+                    }
+                } else {
+                    runtimeBeanRegistrator = baseJMXRegistrator.createRuntimeBeanRegistrator(entry.getIdentifier());
+                    ((RuntimeBeanRegistratorAwareModule) module).setRuntimeBeanRegistrator(runtimeBeanRegistrator);
+                }
             }
             // save it to info so it is accessible afterwards
-            runtimeRegistrators.put(entry.getIdentifier(), runtimeBeanRegistrator);
+            if(runtimeBeanRegistrator != null) {
+                runtimeRegistrators.put(entry.getIdentifier(), runtimeBeanRegistrator);
+            }
         }
 
         // can register runtime beans
@@ -283,7 +357,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
 
         int orderingIdx = 0;
         for (ModuleIdentifier moduleIdentifier : orderedModuleIdentifiers) {
-            logger.trace("Registering {}", moduleIdentifier);
+            LOG.trace("Registering {}", moduleIdentifier);
             ModuleInternalTransactionalInfo entry = commitInfo.getCommitted()
                     .get(moduleIdentifier);
             if (entry == null) {
@@ -331,6 +405,11 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
 
                 // close old module jmx registrator
                 oldInternalInfo.getModuleJMXRegistrator().close();
+
+                // We no longer need old internal info. Clear it out, so we do not create a serial leak evidenced
+                // by BUG-4514. The reason is that modules retain their resolver, which retains modules. If we retain
+                // the old module, we would have the complete reconfiguration history held in heap for no good reason.
+                entry.clearOldInternalInfo();
             } else {
                 // new instance:
                 // wrap in readable dynamic mbean
@@ -344,24 +423,20 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
 
             // register to JMX
             try {
-                newModuleJMXRegistrator.registerMBean(newReadableConfigBean,
-                        primaryReadOnlyON);
+                newModuleJMXRegistrator.registerMBean(newReadableConfigBean, primaryReadOnlyON);
             } catch (InstanceAlreadyExistsException e) {
-                throw new IllegalStateException(e);
+                throw new IllegalStateException("Possible code error, already registered:" + primaryReadOnlyON,e);
             }
 
-            // register to OSGi
+            // register services to OSGi
+            Map<ServiceInterfaceAnnotation, String /* service ref name */> annotationMapping = configTransactionController.getWritableRegistry().findServiceInterfaces(moduleIdentifier);
+            BundleContext bc = configTransactionController.getModuleFactoryBundleContext(
+                    entry.getModuleFactory().getImplementationName());
             if (osgiRegistration == null) {
-                ModuleFactory moduleFactory = entry.getModuleFactory();
-                if (moduleFactory != null) {
-                    BundleContext bc = configTransactionController.
-                            getModuleFactoryBundleContext(moduleFactory.getImplementationName());
-                    osgiRegistration = beanToOsgiServiceManager.registerToOsgi(realModule.getClass(),
-                            newReadableConfigBean.getInstance(), entry.getIdentifier(), bc);
-                } else {
-                    throw new NullPointerException(entry.getIdentifier().getFactoryName() + " ModuleFactory not found.");
-                }
-
+                osgiRegistration = beanToOsgiServiceManager.registerToOsgi(
+                        newReadableConfigBean.getInstance(), moduleIdentifier, bc, annotationMapping);
+            } else {
+                osgiRegistration.updateRegistrations(annotationMapping, bc, instance);
             }
 
             RootRuntimeBeanRegistratorImpl runtimeBeanRegistrator = runtimeRegistrators
@@ -369,7 +444,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
             ModuleInternalInfo newInfo = new ModuleInternalInfo(
                     entry.getIdentifier(), newReadableConfigBean, osgiRegistration,
                     runtimeBeanRegistrator, newModuleJMXRegistrator,
-                    orderingIdx, entry.isDefaultBean());
+                    orderingIdx, entry.isDefaultBean(), entry.getModuleFactory(), entry.getBundleContext());
 
             newConfigEntries.put(realModule, newInfo);
             orderingIdx++;
@@ -380,9 +455,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);
@@ -392,7 +469,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());
@@ -410,7 +487,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();
@@ -422,15 +503,15 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
                 configTransactionControllerEntry.getValue().close();
                 configTransactionController.abortConfig();
             } catch (RuntimeException e) {
-                logger.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
-        List<DestroyedModule> destroyedModules = currentConfig
-                .getModulesToBeDestroyed();
+        // destroy all live objects one after another in order of the dependency hierarchy, from top to bottom
+        List<DestroyedModule> destroyedModules = currentConfig.getModulesToBeDestroyed();
+
+        LOG.info("ConfigRegistry closing - destroying {} modules", destroyedModules.size());
+
         for (DestroyedModule destroyedModule : destroyedModules) {
             destroyedModule.close();
         }
@@ -440,6 +521,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
         MBeanServerFactory.releaseMBeanServer(registryMBeanServer);
         MBeanServerFactory.releaseMBeanServer(transactionsMBeanServer);
 
+        LOG.info("ConfigRegistry closed");
     }
 
     /**
@@ -519,14 +601,10 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
     @Override
     public Set<ObjectName> lookupRuntimeBeans(String moduleName,
                                               String instanceName) {
-        if (moduleName == null) {
-            moduleName = "*";
-        }
-        if (instanceName == null) {
-            instanceName = "*";
-        }
+        String finalModuleName = moduleName == null ? "*" : moduleName;
+        String finalInstanceName = instanceName == null ? "*" : instanceName;
         ObjectName namePattern = ObjectNameUtil.createRuntimeBeanPattern(
-                moduleName, instanceName);
+                finalModuleName, finalInstanceName);
         return baseJMXRegistrator.queryNames(namePattern, null);
     }
 
@@ -544,38 +622,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
@@ -595,16 +687,15 @@ 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
      * Service Registry.
      */
     public void addAll(Collection<ModuleInternalInfo> configInfos) {
-        if (currentConfig.size() > 0) {
+        if (!currentConfig.isEmpty()) {
             throw new IllegalStateException(
                     "Error - some config entries were not removed: "
                             + currentConfig);
@@ -615,8 +706,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:"
@@ -655,7 +745,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
@@ -663,16 +752,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(
@@ -681,8 +766,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.