Reapply "Gjoranv/allow duplicate bundles"

This reverts commit 2ef1e922a1d845b3cd79e9fb329925e7e9896919.

5 files changed, 57 insertions, 22 deletions

diff --git a/container-di/src/main/java/com/yahoo/container/di/ComponentDeconstructor.java b/container-di/src/main/java/com/yahoo/container/di/ComponentDeconstructor.java
index 09f72c9d86d..61497cf71bc 100644
--- a/container-di/src/main/java/com/yahoo/container/di/ComponentDeconstructor.java
+++ b/container-di/src/main/java/com/yahoo/container/di/ComponentDeconstructor.java
@@ -1,6 +1,8 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.di;
 
+import org.osgi.framework.Bundle;
+
 import java.util.Collection;
 
 /**
@@ -8,5 +10,7 @@ import java.util.Collection;
  * @author Tony Vaagenes
  */
 public interface ComponentDeconstructor {
-    void deconstruct(Collection<Object> components);
+
+    void deconstruct(Collection<Object> components, Collection<Bundle> bundles);
+
 }
diff --git a/container-di/src/main/java/com/yahoo/container/di/Container.java b/container-di/src/main/java/com/yahoo/container/di/Container.java
index f82fd22f40d..9a9245f4ba2 100644
--- a/container-di/src/main/java/com/yahoo/container/di/Container.java
+++ b/container-di/src/main/java/com/yahoo/container/di/Container.java
@@ -18,9 +18,13 @@ import com.yahoo.container.di.componentgraph.core.Node;
 import com.yahoo.container.di.config.RestApiContext;
 import com.yahoo.container.di.config.SubscriberFactory;
 import com.yahoo.vespa.config.ConfigKey;
+import org.osgi.framework.Bundle;
 
+import java.util.Collection;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.IdentityHashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.logging.Level;
@@ -65,19 +69,22 @@ public class Container {
         });
     }
 
-    private void deconstructObsoleteComponents(ComponentGraph oldGraph, ComponentGraph newGraph) {
+    private void deconstructObsoleteComponents(ComponentGraph oldGraph,
+                                               ComponentGraph newGraph,
+                                               Collection<Bundle> obsoleteBundles) {
         IdentityHashMap<Object, Object> oldComponents = new IdentityHashMap<>();
         oldGraph.allConstructedComponentsAndProviders().forEach(c -> oldComponents.put(c, null));
         newGraph.allConstructedComponentsAndProviders().forEach(oldComponents::remove);
-        componentDeconstructor.deconstruct(oldComponents.keySet());
+        componentDeconstructor.deconstruct(oldComponents.keySet(), obsoleteBundles);
     }
 
     public ComponentGraph getNewComponentGraph(ComponentGraph oldGraph, Injector fallbackInjector, boolean restartOnRedeploy) {
         try {
-            ComponentGraph newGraph = getConfigAndCreateGraph(oldGraph, fallbackInjector, restartOnRedeploy);
+            Collection<Bundle> obsoleteBundles = new HashSet<>();
+            ComponentGraph newGraph = getConfigAndCreateGraph(oldGraph, fallbackInjector, restartOnRedeploy, obsoleteBundles);
             newGraph.reuseNodes(oldGraph);
             constructComponents(newGraph);
-            deconstructObsoleteComponents(oldGraph, newGraph);
+            deconstructObsoleteComponents(oldGraph, newGraph, obsoleteBundles);
             return newGraph;
         } catch (Throwable t) {
             // TODO: Wrap ComponentConstructorException in an Error when generation==0 (+ unit test that Error is thrown)
@@ -122,8 +129,13 @@ public class Container {
         }
     }
 
-    private ComponentGraph getConfigAndCreateGraph(ComponentGraph graph, Injector fallbackInjector, boolean restartOnRedeploy) {
+    private ComponentGraph getConfigAndCreateGraph(ComponentGraph graph,
+                                                   Injector fallbackInjector,
+                                                   boolean restartOnRedeploy,
+                                                   Collection<Bundle> obsoleteBundles) // NOTE: Return value
+    {
         ConfigSnapshot snapshot;
+
         while (true) {
             snapshot = configurer.getConfigs(graph.configKeys(), leastGeneration, restartOnRedeploy);
 
@@ -131,7 +143,6 @@ public class Container {
                     graph.configKeys(), graph.generation(), snapshot));
 
             if (snapshot instanceof BootstrapConfigs) {
-                // TODO: remove require when proven unnecessary
                 if (getBootstrapGeneration() <= previousConfigGeneration) {
                     throw new IllegalStateException(String.format(
                             "Got bootstrap configs out of sequence for old config generation %d.\n" + "Previous config generation is %d",
@@ -142,8 +153,12 @@ public class Container {
                                 "Got new bootstrap generation\n" + "bootstrap generation = %d\n" + "components generation: %d\n"
                                         + "previous generation: %d\n",
                                 getBootstrapGeneration(), getComponentsGeneration(), previousConfigGeneration));
-                installBundles(snapshot.configs());
+
+                Collection<Bundle> bundlesToRemove = installBundles(snapshot.configs());
+                obsoleteBundles.addAll(bundlesToRemove);
+
                 graph = createComponentsGraph(snapshot.configs(), getBootstrapGeneration(), fallbackInjector);
+
                 // Continues loop
 
             } else if (snapshot instanceof ConfigRetriever.ComponentsConfigs) {
@@ -184,9 +199,9 @@ public class Container {
         }
     }
 
-    private void installBundles(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> configsIncludingBootstrapConfigs) {
+    private Set<Bundle> installBundles(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> configsIncludingBootstrapConfigs) {
         BundlesConfig bundlesConfig = getConfig(bundlesConfigKey, configsIncludingBootstrapConfigs);
-        osgi.useBundles(bundlesConfig.bundle());
+        return osgi.useBundles(bundlesConfig.bundle());
     }
 
     private ComponentGraph createComponentsGraph(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> configsIncludingBootstrapConfigs,
@@ -244,7 +259,8 @@ public class Container {
     }
 
     private void deconstructAllComponents(ComponentGraph graph, ComponentDeconstructor deconstructor) {
-        deconstructor.deconstruct(graph.allConstructedComponentsAndProviders());
+        // This is only used for shutdown, so no need to uninstall any bundles.
+        deconstructor.deconstruct(graph.allConstructedComponentsAndProviders(), Collections.emptyList());
     }
 
     public static <T extends ConfigInstance> T getConfig(ConfigKey<T> key,
diff --git a/container-di/src/main/java/com/yahoo/container/di/Osgi.java b/container-di/src/main/java/com/yahoo/container/di/Osgi.java
index 7095180dfc5..ab7da7665b6 100644
--- a/container-di/src/main/java/com/yahoo/container/di/Osgi.java
+++ b/container-di/src/main/java/com/yahoo/container/di/Osgi.java
@@ -13,6 +13,8 @@ import java.util.Collections;
 import java.util.Set;
 import java.util.stream.Collectors;
 
+import static java.util.Collections.emptySet;
+
 /**
  * @author gjoranv
  * @author Tony Vaagenes
@@ -23,8 +25,13 @@ public interface Osgi {
         return new BundleClasses(new MockBundle(), Collections.emptySet());
     }
 
-    default void useBundles(Collection<FileReference> bundles) {
+    /**
+     * Returns the set of bundles that is not used by the current application generation,
+     * and therefore should be scheduled for uninstalling.
+     */
+    default Set<Bundle> useBundles(Collection<FileReference> bundles) {
         System.out.println("useBundles " + bundles.stream().map(Object::toString).collect(Collectors.joining(", ")));
+        return emptySet();
     }
 
     default Class<?> resolveClass(BundleInstantiationSpecification spec) {
diff --git a/container-di/src/test/java/com/yahoo/container/di/ContainerTest.java b/container-di/src/test/java/com/yahoo/container/di/ContainerTest.java
index 9c4891c7db2..90bda0ef8a8 100644
--- a/container-di/src/test/java/com/yahoo/container/di/ContainerTest.java
+++ b/container-di/src/test/java/com/yahoo/container/di/ContainerTest.java
@@ -14,6 +14,8 @@ import com.yahoo.container.di.componentgraph.core.Node;
 import com.yahoo.container.di.config.RestApiContext;
 import org.junit.Ignore;
 import org.junit.Test;
+import org.osgi.framework.Bundle;
+import org.osgi.framework.BundleException;
 
 import java.util.Collection;
 import java.util.concurrent.ExecutionException;
@@ -283,13 +285,16 @@ public class ContainerTest extends ContainerTestBase {
     public void providers_are_destructed() {
         writeBootstrapConfigs("id1", DestructableProvider.class);
 
-        ComponentDeconstructor deconstructor = components -> components.forEach(component -> {
-            if (component instanceof AbstractComponent) {
-                ((AbstractComponent) component).deconstruct();
-            } else if (component instanceof Provider) {
-                ((Provider<?>) component).deconstruct();
-            }
-        });
+        ComponentDeconstructor deconstructor = (components, bundles) -> {
+            components.forEach(component -> {
+                if (component instanceof AbstractComponent) {
+                    ((AbstractComponent) component).deconstruct();
+                } else if (component instanceof Provider) {
+                    ((Provider<?>) component).deconstruct();
+                }
+            });
+            if (! bundles.isEmpty()) throw new IllegalArgumentException("This test should not use bundles");
+        };
 
         Container container = newContainer(dirConfigSource, deconstructor);
 
@@ -373,13 +378,14 @@ public class ContainerTest extends ContainerTestBase {
 
     public static class TestDeconstructor implements ComponentDeconstructor {
         @Override
-        public void deconstruct(Collection<Object> components) {
+        public void deconstruct(Collection<Object> components, Collection<Bundle> bundles) {
             components.forEach(component -> {
                 if (component instanceof DestructableComponent) {
                     DestructableComponent vespaComponent = (DestructableComponent) component;
                     vespaComponent.deconstruct();
                 }
             });
+            if (! bundles.isEmpty()) throw new IllegalArgumentException("This test should not use bundles");
         }
     }
 
diff --git a/container-di/src/test/java/com/yahoo/container/di/ContainerTestBase.java b/container-di/src/test/java/com/yahoo/container/di/ContainerTestBase.java
index 79cb080dfa4..18236a6bde9 100644
--- a/container-di/src/test/java/com/yahoo/container/di/ContainerTestBase.java
+++ b/container-di/src/test/java/com/yahoo/container/di/ContainerTestBase.java
@@ -5,7 +5,6 @@ import com.google.inject.Guice;
 import com.yahoo.component.ComponentSpecification;
 import com.yahoo.config.FileReference;
 import com.yahoo.container.bundle.BundleInstantiationSpecification;
-import com.yahoo.container.di.CloudSubscriberFactory;
 import com.yahoo.container.di.ContainerTest.ComponentTakingConfig;
 import com.yahoo.container.di.componentgraph.core.ComponentGraph;
 import com.yahoo.container.di.osgi.BundleClasses;
@@ -16,6 +15,8 @@ import org.osgi.framework.Bundle;
 import java.util.Collection;
 import java.util.Set;
 
+import static java.util.Collections.emptySet;
+
 /**
  * @author Tony Vaagenes
  * @author gjoranv
@@ -60,7 +61,8 @@ public class ContainerTestBase {
                         }
 
                         @Override
-                        public void useBundles(Collection<FileReference> bundles) {
+                        public Set<Bundle> useBundles(Collection<FileReference> bundles) {
+                            return emptySet();
                         }
 
                         @Override