Support safe component deconstruction in jdisc container.

- Add allowDuplicates to all Osgi classes
- Uninstall bundles in Deconstructor
- We no longer refresh bundles because we uninstall old bundles at
  a later point than the new bundles are installed. Hence, the
  user must version app bundles that are dependencies used by
  other app bundles.

6 files changed, 110 insertions, 34 deletions

diff --git a/container-core/src/main/java/com/yahoo/container/core/config/BundleLoader.java b/container-core/src/main/java/com/yahoo/container/core/config/BundleLoader.java
index 2b3a272fadc..c796b773232 100644
--- a/container-core/src/main/java/com/yahoo/container/core/config/BundleLoader.java
+++ b/container-core/src/main/java/com/yahoo/container/core/config/BundleLoader.java
@@ -26,11 +26,18 @@ import static com.yahoo.container.core.BundleLoaderProperties.DISK_BUNDLE_PREFIX
  * Manages the set of installed 3rd-party component bundles.
  *
  * @author Tony Vaagenes
+ * @author gjoranv
  */
 public class BundleLoader {
 
-    private final List<Bundle> initialBundles;
-
+    /* Map of file refs of active bundles (not scheduled for uninstall) to a list of all bundles that were installed
+     * (pre-install directive) by the bundle pointed to by the file ref (including itself).
+     *
+     * Used to:
+     * 1. Avoid installing already installed bundles. Just an optimization, installing the same bundle location is a NOP
+     * 2. Start bundles (all are started every time)
+     * 3. Calculate the set of bundles to uninstall
+     */
     private final Map<FileReference, List<Bundle>> reference2Bundles = new LinkedHashMap<>();
 
     private final Logger log = Logger.getLogger(BundleLoader.class.getName());
@@ -38,7 +45,6 @@ public class BundleLoader {
 
     public BundleLoader(Osgi osgi) {
         this.osgi = osgi;
-        initialBundles = Arrays.asList(osgi.getBundles());
     }
 
     private List<Bundle> obtainBundles(FileReference reference, FileAcquirer fileAcquirer) throws InterruptedException {
@@ -47,16 +53,19 @@ public class BundleLoader {
     }
 
     /** Returns the number of bundles installed by this call. */
-    private int install(List<FileReference> references) {
+    private void install(List<FileReference> references) {
         Set<FileReference> bundlesToInstall = new HashSet<>(references);
+
+        // This is just an optimization, as installing a bundle with the same location id returns the already installed bundle.
+        // It has no effect that pendingUninstall fileRefs are removed from the map, as they are NOT in the new set of bundles..
         bundlesToInstall.removeAll(reference2Bundles.keySet());
 
         PredicateSplit<FileReference> bundlesToInstall_isDisk = partition(bundlesToInstall, BundleLoader::isDiskBundle);
         installBundlesFromDisk(bundlesToInstall_isDisk.trueValues);
         installBundlesFromFileDistribution(bundlesToInstall_isDisk.falseValues);
 
+        // TODO: Remove. Bundles are also started in use()
         startBundles();
-        return bundlesToInstall.size();
     }
 
     private static boolean isDiskBundle(FileReference fileReference) {
@@ -97,6 +106,7 @@ public class BundleLoader {
         }
 
         List<Bundle> bundles = osgi.install(file.getAbsolutePath());
+
         reference2Bundles.put(reference, bundles);
     }
 
@@ -113,13 +123,16 @@ public class BundleLoader {
         }
     }
 
-    // All bundles must have been started first to ensure correct package resolution.
+    /**
+     * Resolves and starts (calls the Bundles BundleActivator) all bundles. Bundle resolution must take place
+     * after all bundles are installed to ensure that the framework can resolve dependencies between bundles.
+     */
     private void startBundles() {
         for (List<Bundle> bundles : reference2Bundles.values()) {
             for (Bundle bundle : bundles) {
                 try {
                     if ( ! isFragment(bundle))
-                        bundle.start();
+                        bundle.start();  // NOP for already ACTIVE bundles
                 } catch(Exception e) {
                     throw new RuntimeException("Could not start bundle '" + bundle.getSymbolicName() + "'", e);
                 }
@@ -135,38 +148,39 @@ public class BundleLoader {
         return (bundleRevision.getTypes() & BundleRevision.TYPE_FRAGMENT) != 0;
     }
 
-    /** Returns the number of uninstalled bundles */
-    private int retainOnly(List<FileReference> newReferences) {
-        Set<Bundle> bundlesToRemove = new HashSet<>(Arrays.asList(osgi.getBundles()));
+    /**
+     * Returns the bundles to schedule for uninstall after their components have been deconstructed.
+     */
+    private Set<Bundle> retainOnly(List<FileReference> newReferences) {
+        Set<Bundle> bundlesToRemove = new HashSet<>(osgi.getCurrentBundles());
 
         for (FileReference fileReferenceToKeep: newReferences) {
             if (reference2Bundles.containsKey(fileReferenceToKeep))
                 bundlesToRemove.removeAll(reference2Bundles.get(fileReferenceToKeep));
         }
 
-        bundlesToRemove.removeAll(initialBundles);
-        for (Bundle bundle : bundlesToRemove) {
-            log.info("Removing bundle '" + bundle.toString() + "'");
-            osgi.uninstall(bundle);
-        }
+        bundlesToRemove.removeAll(osgi.getInitialBundles());
 
+        // Clean up the map of active bundles
         Set<FileReference> fileReferencesToRemove = new HashSet<>(reference2Bundles.keySet());
         fileReferencesToRemove.removeAll(newReferences);
+        fileReferencesToRemove.forEach(reference2Bundles::remove);
 
-        for (FileReference fileReferenceToRemove : fileReferencesToRemove) {
-            reference2Bundles.remove(fileReferenceToRemove);
-        }
-        return bundlesToRemove.size();
+        return bundlesToRemove;
     }
 
-    public synchronized int use(List<FileReference> bundles) {
-        int removedBundles = retainOnly(bundles);
-        int installedBundles = install(bundles);
+    /**
+     * Installs the given set of bundles and returns the set of bundles that is no longer used
+     * by the application, and should therefore be scheduled for uninstall.
+     */
+    public synchronized Set<Bundle> use(List<FileReference> bundles) {
+        Set<Bundle> bundlesToUninstall = retainOnly(bundles);
+        osgi.allowDuplicateBundles(bundlesToUninstall);
+        install(bundles);
         startBundles();
 
-        log.info(removedBundles + " bundles were removed, and " + installedBundles + " bundles were installed.");
         log.info(installedBundlesMessage());
-        return removedBundles + installedBundles;
+        return bundlesToUninstall;
     }
 
     private String installedBundlesMessage() {
diff --git a/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java b/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java
index dd8b2605820..2ca76535b34 100644
--- a/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java
+++ b/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java
@@ -30,6 +30,7 @@ import org.osgi.framework.wiring.BundleWiring;
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.List;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
 import java.util.logging.Logger;
@@ -121,14 +122,9 @@ public class HandlersConfigurerDi {
         }
 
         @Override
-        public void useBundles(Collection<FileReference> bundles) {
+        public Set<Bundle> useBundles(Collection<FileReference> bundles) {
             log.info("Installing bundles from the latest application");
-
-            int bundlesRemovedOrInstalled = bundleLoader.use(new ArrayList<>(bundles));
-
-            if (bundlesRemovedOrInstalled > 0) {
-                refreshPackages();
-            }
+            return bundleLoader.use(new ArrayList<>(bundles));
         }
     }
 
diff --git a/container-core/src/main/java/com/yahoo/container/core/config/testutil/HandlersConfigurerTestWrapper.java b/container-core/src/main/java/com/yahoo/container/core/config/testutil/HandlersConfigurerTestWrapper.java
index 684a45aeac1..bc06aad76ec 100644
--- a/container-core/src/main/java/com/yahoo/container/core/config/testutil/HandlersConfigurerTestWrapper.java
+++ b/container-core/src/main/java/com/yahoo/container/core/config/testutil/HandlersConfigurerTestWrapper.java
@@ -19,7 +19,6 @@ import com.yahoo.osgi.MockOsgi;
 
 import java.io.File;
 import java.io.IOException;
-import java.util.Collection;
 import java.util.LinkedHashSet;
 import java.util.Random;
 import java.util.Set;
@@ -103,11 +102,12 @@ public class HandlersConfigurerTestWrapper {
     }
 
     private ComponentDeconstructor getTestDeconstructor() {
-        return components -> components.forEach(component -> {
+        return (components, bundles) -> components.forEach(component -> {
             if (component instanceof AbstractComponent) {
                 AbstractComponent abstractComponent = (AbstractComponent) component;
                 if (abstractComponent.isDeconstructable()) abstractComponent.deconstruct();
             }
+            if (! bundles.isEmpty()) throw new IllegalArgumentException("This test should not use bundles");
         });
     }
 
diff --git a/container-core/src/main/java/com/yahoo/osgi/MockOsgi.java b/container-core/src/main/java/com/yahoo/osgi/MockOsgi.java
index 45ad02d2cef..5cfcde7a8a3 100644
--- a/container-core/src/main/java/com/yahoo/osgi/MockOsgi.java
+++ b/container-core/src/main/java/com/yahoo/osgi/MockOsgi.java
@@ -16,11 +16,21 @@ import java.util.List;
 public class MockOsgi extends NonWorkingOsgiFramework implements Osgi {
 
     @Override
+    public List<Bundle> getInitialBundles() {
+        return Collections.emptyList();
+    }
+
+    @Override
     public Bundle[] getBundles() {
         return new Bundle[0];
     }
 
     @Override
+    public List<Bundle> getCurrentBundles() {
+        return Collections.emptyList();
+    }
+
+    @Override
     public Bundle getBundle(ComponentSpecification bundleId) {
         return null;
     }
diff --git a/container-core/src/main/java/com/yahoo/osgi/Osgi.java b/container-core/src/main/java/com/yahoo/osgi/Osgi.java
index c94eaf43deb..fa7e88dbfe6 100644
--- a/container-core/src/main/java/com/yahoo/osgi/Osgi.java
+++ b/container-core/src/main/java/com/yahoo/osgi/Osgi.java
@@ -4,6 +4,7 @@ package com.yahoo.osgi;
 import com.yahoo.component.ComponentSpecification;
 import org.osgi.framework.Bundle;
 
+import java.util.Collection;
 import java.util.List;
 
 /**
@@ -11,14 +12,23 @@ import java.util.List;
  */
 public interface Osgi {
 
+    List<Bundle> getInitialBundles();
+
     Bundle[] getBundles();
 
+    /** Returns all bundles that have not been scheduled for uninstall. */
+    List<Bundle> getCurrentBundles();
+
     Bundle getBundle(ComponentSpecification bundleId);
 
     List<Bundle> install(String absolutePath);
 
+    void allowDuplicateBundles(Collection<Bundle> bundles);
+
+    // TODO: remove
     void uninstall(Bundle bundle);
 
+    // TODO: remove
     void refreshPackages();
 
 }
diff --git a/container-core/src/main/java/com/yahoo/osgi/OsgiImpl.java b/container-core/src/main/java/com/yahoo/osgi/OsgiImpl.java
index 8b2f20a1c13..9a74aeda8cf 100644
--- a/container-core/src/main/java/com/yahoo/osgi/OsgiImpl.java
+++ b/container-core/src/main/java/com/yahoo/osgi/OsgiImpl.java
@@ -5,9 +5,13 @@ import com.yahoo.component.ComponentSpecification;
 import com.yahoo.component.Version;
 import com.yahoo.container.bundle.BundleInstantiationSpecification;
 import com.yahoo.jdisc.application.OsgiFramework;
+import com.yahoo.jdisc.test.NonWorkingOsgiFramework;
 import org.osgi.framework.Bundle;
 import org.osgi.framework.BundleException;
+import org.osgi.framework.launch.Framework;
 
+import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 
 /**
@@ -18,8 +22,33 @@ public class OsgiImpl implements Osgi {
 
     private final OsgiFramework jdiscOsgi;
 
+    // The initial bundles are never scheduled for uninstall
+    private final List<Bundle> initialBundles;
+
+    // An initial bundle that is not the framework, and can hence be used to look up current bundles
+    private final Bundle alwaysCurrentBundle;
+
     public OsgiImpl(OsgiFramework jdiscOsgi) {
         this.jdiscOsgi = jdiscOsgi;
+
+        if (jdiscOsgi instanceof NonWorkingOsgiFramework) {
+            initialBundles = Collections.emptyList();
+            alwaysCurrentBundle = null;
+        } else {
+
+            this.initialBundles = jdiscOsgi.bundles();
+            if (initialBundles.isEmpty())
+                throw new IllegalStateException("No initial bundles!");
+
+            alwaysCurrentBundle = firstNonFrameworkBundle(initialBundles);
+            if (alwaysCurrentBundle == null)
+                throw new IllegalStateException("The initial bundles only contained the framework bundle!");
+        }
+    }
+
+    @Override
+    public List<Bundle> getInitialBundles() {
+        return initialBundles;
     }
 
     @Override
@@ -28,6 +57,10 @@ public class OsgiImpl implements Osgi {
         return bundles.toArray(new Bundle[bundles.size()]);
     }
 
+    @Override
+    public List<Bundle> getCurrentBundles() {
+        return jdiscOsgi.getBundles(alwaysCurrentBundle);
+    }
 
     public Class<Object> resolveClass(BundleInstantiationSpecification spec) {
         Bundle bundle = getBundle(spec.bundle);
@@ -87,7 +120,7 @@ public class OsgiImpl implements Osgi {
      */
     public Bundle getBundle(ComponentSpecification id) {
         Bundle highestMatch = null;
-        for (Bundle bundle : getBundles()) {
+        for (Bundle bundle : getCurrentBundles()) {
             assert bundle.getSymbolicName() != null : "ensureHasBundleSymbolicName not called during installation";
 
             if ( ! bundle.getSymbolicName().equals(id.getName())) continue;
@@ -122,6 +155,11 @@ public class OsgiImpl implements Osgi {
     }
 
     @Override
+    public void allowDuplicateBundles(Collection<Bundle> bundles) {
+        jdiscOsgi.allowDuplicateBundles(bundles);
+    }
+
+    @Override
     public void uninstall(Bundle bundle) {
         try {
             bundle.uninstall();
@@ -135,4 +173,12 @@ public class OsgiImpl implements Osgi {
         jdiscOsgi.refreshPackages();
     }
 
+    private static Bundle firstNonFrameworkBundle(List<Bundle> bundles) {
+        for (Bundle b : bundles) {
+            if (! (b instanceof Framework))
+                return b;
+        }
+        return null;
+    }
+
 }