Bug 6969 - Memory leak during bundle tree restart 84/48284/11
authorAlexis de Talhouët <adetalhouet@inocybe.com>
Tue, 15 Nov 2016 18:55:33 +0000 (13:55 -0500)
committerTom Pantelis <tpanteli@brocade.com>
Thu, 17 Nov 2016 13:34:00 +0000 (13:34 +0000)
There are two root causes for that memory leak:

-  Retention on BlueprintContainer when desotrying them before "quiescing"
them:
Restarting a set of bundles requires cooperation between them to avoid
having retention on services due to on-going call, processing.
As OSGi APIs don't provide such functionality, the Quiesce APIs where
created, ensuring all calls to be finished before actually shutting down
the bundle. Once the set of bundles are quiesced, e.g. shut down, they
can safely be destroyed.

- Retention on the TopoProcessingProviderImpl due to unreleased resources:
https://git.opendaylight.org/gerrit/#/c/48283/

Change-Id: I0af4a27ac7c87f1cc158313f0497158733a045a5
Signed-off-by: Alexis de Talhouët <adetalhouet@inocybe.com>
opendaylight/blueprint/src/main/java/org/opendaylight/controller/blueprint/BlueprintBundleTracker.java
opendaylight/blueprint/src/main/java/org/opendaylight/controller/blueprint/BlueprintContainerRestartServiceImpl.java

index 6c267ac..404d78a 100644 (file)
@@ -18,6 +18,7 @@ import java.util.Hashtable;
 import java.util.List;
 import org.apache.aries.blueprint.NamespaceHandler;
 import org.apache.aries.blueprint.services.BlueprintExtenderService;
+import org.apache.aries.quiesce.participant.QuiesceParticipant;
 import org.apache.aries.util.AriesFrameworkUtil;
 import org.opendaylight.controller.blueprint.ext.OpendaylightNamespaceHandler;
 import org.opendaylight.controller.config.api.ConfigSystemService;
@@ -54,10 +55,12 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus
     private static final String BLUEPRINT_FLE_PATTERN = "*.xml";
     private static final long SYSTEM_BUNDLE_ID = 0;
 
-    private ServiceTracker<BlueprintExtenderService, BlueprintExtenderService> serviceTracker;
+    private ServiceTracker<BlueprintExtenderService, BlueprintExtenderService> blueprintExtenderServiceTracker;
+    private ServiceTracker<QuiesceParticipant, QuiesceParticipant> quiesceParticipantTracker;
     private BundleTracker<Bundle> bundleTracker;
     private BundleContext bundleContext;
     private volatile BlueprintExtenderService blueprintExtenderService;
+    private volatile QuiesceParticipant quiesceParticipant;
     private volatile ServiceRegistration<?> blueprintContainerRestartReg;
     private volatile BlueprintContainerRestartServiceImpl restartService;
     private volatile boolean shuttingDown;
@@ -71,6 +74,8 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus
     public void start(BundleContext context) {
         LOG.info("Starting {}", getClass().getSimpleName());
 
+        restartService = new BlueprintContainerRestartServiceImpl();
+
         bundleContext = context;
 
         registerBlueprintEventHandler(context);
@@ -79,7 +84,7 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus
 
         bundleTracker = new BundleTracker<>(context, Bundle.ACTIVE, this);
 
-        serviceTracker = new ServiceTracker<>(context, BlueprintExtenderService.class.getName(),
+        blueprintExtenderServiceTracker = new ServiceTracker<>(context, BlueprintExtenderService.class.getName(),
                 new ServiceTrackerCustomizer<BlueprintExtenderService, BlueprintExtenderService>() {
                     @Override
                     public BlueprintExtenderService addingService(
@@ -91,7 +96,8 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus
 
                         LOG.debug("Got BlueprintExtenderService");
 
-                        restartService = new BlueprintContainerRestartServiceImpl(blueprintExtenderService);
+                        restartService.setBlueprintExtenderService(blueprintExtenderService);
+
                         blueprintContainerRestartReg = context.registerService(
                                 BlueprintContainerRestartService.class.getName(), restartService, new Hashtable<>());
 
@@ -108,7 +114,33 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus
                             BlueprintExtenderService service) {
                     }
                 });
-        serviceTracker.open();
+        blueprintExtenderServiceTracker.open();
+
+        quiesceParticipantTracker = new ServiceTracker<>(context, QuiesceParticipant.class.getName(),
+                new ServiceTrackerCustomizer<QuiesceParticipant, QuiesceParticipant>() {
+                    @Override
+                    public QuiesceParticipant addingService(
+                            ServiceReference<QuiesceParticipant> reference) {
+                        quiesceParticipant = reference.getBundle().getBundleContext().getService(reference);
+
+                        LOG.debug("Got QuiesceParticipant");
+
+                        restartService.setQuiesceParticipant(quiesceParticipant);
+
+                        return quiesceParticipant;
+                    }
+
+                    @Override
+                    public void modifiedService(ServiceReference<QuiesceParticipant> reference,
+                                                QuiesceParticipant service) {
+                    }
+
+                    @Override
+                    public void removedService(ServiceReference<QuiesceParticipant> reference,
+                                               QuiesceParticipant service) {
+                    }
+                });
+        quiesceParticipantTracker.open();
     }
 
     private void registerNamespaceHandler(BundleContext context) {
@@ -131,7 +163,8 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus
     @Override
     public void stop(BundleContext context) {
         bundleTracker.close();
-        serviceTracker.close();
+        blueprintExtenderServiceTracker.close();
+        quiesceParticipantTracker.close();
 
         AriesFrameworkUtil.safeUnregisterService(eventHandlerReg);
         AriesFrameworkUtil.safeUnregisterService(namespaceReg);
@@ -160,7 +193,7 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus
     }
 
     /**
-     * Implemented from BundleActivator.
+     * Implemented from BundleTrackerCustomizer.
      */
     @Override
     public void modifiedBundle(Bundle bundle, BundleEvent event, Bundle object) {
@@ -180,7 +213,7 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus
     }
 
     /**
-     * Implemented from BundleActivator.
+     * Implemented from BundleTrackerCustomizer.
      */
     @Override
     public void removedBundle(Bundle bundle, BundleEvent event, Bundle object) {
index 6a464b7..e58c956 100644 (file)
@@ -8,11 +8,16 @@
 package org.opendaylight.controller.blueprint;
 
 import com.google.common.base.Optional;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.Lists;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
 import java.lang.management.ManagementFactory;
 import java.util.AbstractMap.SimpleEntry;
+import java.util.ArrayDeque;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Deque;
 import java.util.Dictionary;
 import java.util.Hashtable;
 import java.util.LinkedHashSet;
@@ -28,6 +33,7 @@ import javax.management.InstanceNotFoundException;
 import javax.management.ObjectName;
 import javax.xml.parsers.ParserConfigurationException;
 import org.apache.aries.blueprint.services.BlueprintExtenderService;
+import org.apache.aries.quiesce.participant.QuiesceParticipant;
 import org.apache.aries.util.AriesFrameworkUtil;
 import org.opendaylight.controller.config.api.ConfigRegistry;
 import org.opendaylight.controller.config.api.ConflictingVersionException;
@@ -68,12 +74,18 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo
 
     private final ExecutorService restartExecutor = Executors.newSingleThreadExecutor(new ThreadFactoryBuilder()
             .setDaemon(true).setNameFormat("BlueprintContainerRestartService").build());
-    private final BlueprintExtenderService blueprintExtenderService;
 
-    BlueprintContainerRestartServiceImpl(BlueprintExtenderService blueprintExtenderService) {
+    private BlueprintExtenderService blueprintExtenderService;
+    private QuiesceParticipant quiesceParticipant;
+
+    void setBlueprintExtenderService(final BlueprintExtenderService blueprintExtenderService) {
         this.blueprintExtenderService = blueprintExtenderService;
     }
 
+    void setQuiesceParticipant(final QuiesceParticipant quiesceParticipant) {
+        this.quiesceParticipant = quiesceParticipant;
+    }
+
     public void restartContainer(final Bundle bundle, final List<Object> paths) {
         if (restartExecutor.isShutdown()) {
             return;
@@ -99,6 +111,9 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo
     }
 
     private void restartContainerAndDependentsInternal(Bundle forBundle) {
+        Preconditions.checkNotNull(blueprintExtenderService);
+        Preconditions.checkNotNull(quiesceParticipant);
+
         // We use a LinkedHashSet to preserve insertion order as we walk the service usage hierarchy.
         Set<Bundle> containerBundlesSet = new LinkedHashSet<>();
         List<Entry<String, ModuleIdentifier>> configModules = new ArrayList<>();
@@ -109,11 +124,6 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo
         LOG.info("Restarting blueprint containers for bundle {} and its dependent bundles {}", forBundle,
                 containerBundles.subList(1, containerBundles.size()));
 
-        // Destroy the containers in reverse order with 'forBundle' last, ie bottom-up in the service tree.
-        for (Bundle bundle: Lists.reverse(containerBundles)) {
-            blueprintExtenderService.destroyContainer(bundle, blueprintExtenderService.getContainer(bundle));
-        }
-
         // The blueprint containers are created asynchronously so we register a handler for blueprint events
         // that are sent when a container is complete, successful or not. The CountDownLatch tells when all
         // containers are complete. This is done to ensure all blueprint containers are finished before we
@@ -126,14 +136,11 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo
             }
         });
 
-        // Restart the containers top-down starting with 'forBundle'.
-        for (Bundle bundle: containerBundles) {
-            List<Object> paths = BlueprintBundleTracker.findBlueprintPaths(bundle);
+        final Runnable createContainerCallback = () -> createContainers(containerBundles);
 
-            LOG.info("Restarting blueprint container for bundle {} with paths {}", bundle, paths);
+        // Destroy the container down-top recursively and once done, restart the container top-down
+        destroyContainers(new ArrayDeque<>(Lists.reverse(containerBundles)), createContainerCallback);
 
-            blueprintExtenderService.createContainer(bundle, paths);
-        }
 
         try {
             containerCreationComplete.await(5, TimeUnit.MINUTES);
@@ -148,6 +155,62 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo
         restartConfigModules(forBundle.getBundleContext(), configModules);
     }
 
+    /**
+     * Recursively quiesce and destroy the bundles one by one in order to maintain synchronicity and ordering.
+     * @param remainingBundlesToDestroy the list of remaining bundles to destroy.
+     * @param createContainerCallback a {@link Runnable} to {@code run()} when the recursive function is completed.
+     */
+    private void destroyContainers(final Deque<Bundle> remainingBundlesToDestroy,
+            final Runnable createContainerCallback) {
+
+        final Bundle nextBundle;
+        synchronized (remainingBundlesToDestroy) {
+            if (remainingBundlesToDestroy.isEmpty()) {
+                LOG.debug("All blueprint containers were quiesced and destroyed");
+                createContainerCallback.run();
+                return;
+            }
+
+            nextBundle = remainingBundlesToDestroy.poll();
+        }
+
+        // The Quiesce capability is a like a soft-stop, clean-stop. In the case of the Blueprint extender, in flight
+        // service calls are allowed to finish; they're counted in and counted out, and no new calls are allowed. When
+        // there are no in flight service calls, the bundle is told to stop. The Blueprint bundle itself doesn't know
+        // this is happening which is a key design point. In the case of Blueprint, the extender ensures no new Entity
+        // Managers(EMs) are created. Then when all those EMs are closed the quiesce operation reports that it is
+        // finished.
+        // To properly restart the blueprint containers, first we have to quiesce the list of bundles, and once done, it
+        // is safe to destroy their BlueprintContainer, so no reference is retained.
+        //
+        // Mail - thread explaining Quiesce API:
+        //      https://www.mail-archive.com/dev@aries.apache.org/msg08403.html
+
+        // Quiesced the bundle to unregister the associated BlueprintContainer
+        quiesceParticipant.quiesce(bundlesQuiesced -> {
+
+            // Destroy the container once Quiesced
+            Arrays.stream(bundlesQuiesced).forEach(quiescedBundle -> {
+                LOG.debug("Quiesced bundle {}", quiescedBundle);
+                blueprintExtenderService.destroyContainer(
+                        quiescedBundle, blueprintExtenderService.getContainer(quiescedBundle));
+            });
+
+            destroyContainers(remainingBundlesToDestroy, createContainerCallback);
+
+        }, Collections.singletonList(nextBundle));
+    }
+
+    private void createContainers(List<Bundle> containerBundles) {
+        containerBundles.forEach(bundle -> {
+            List<Object> paths = BlueprintBundleTracker.findBlueprintPaths(bundle);
+
+            LOG.info("Restarting blueprint container for bundle {} with paths {}", bundle, paths);
+
+            blueprintExtenderService.createContainer(bundle, paths);
+        });
+    }
+
     private void restartConfigModules(BundleContext bundleContext, List<Entry<String,
             ModuleIdentifier>> configModules) {
         if (configModules.isEmpty()) {
@@ -193,7 +256,7 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo
         Config configMapping = configFacade.getConfigMapping();
 
         ConfigRegistry configRegistryClient = new ConfigRegistryJMXClient(ManagementFactory.getPlatformMBeanServer());
-        for (Entry<String, ModuleIdentifier> entry: configModules) {
+        for (Entry<String, ModuleIdentifier> entry : configModules) {
             String moduleNamespace = entry.getKey();
             ModuleIdentifier moduleId = entry.getValue();
             try {
@@ -229,6 +292,7 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo
      *
      * @param bundle the bundle to traverse
      * @param containerBundles the current set of bundles containing blueprint containers
+     * @param configModules the current set of bundles containing config modules
      */
     private void findDependentContainersRecursively(Bundle bundle, Set<Bundle> containerBundles,
             List<Entry<String, ModuleIdentifier>> configModules) {