From d75825d503b8566e66d45b0ef726a1d5834970d7 Mon Sep 17 00:00:00 2001 From: gjoranv Date: Tue, 5 Nov 2019 10:53:44 +0100 Subject: Reapply "Gjoranv/allow duplicate bundles" This reverts commit 2ef1e922a1d845b3cd79e9fb329925e7e9896919. --- .../yahoo/container/core/config/BundleLoader.java | 71 ++++++++++++++-------- .../core/config/HandlersConfigurerDi.java | 32 ++++++---- .../testutil/HandlersConfigurerTestWrapper.java | 8 +-- .../core/config/testutil/MockOsgiWrapper.java | 44 ++++++++++++++ .../src/main/java/com/yahoo/osgi/MockOsgi.java | 16 ++--- .../src/main/java/com/yahoo/osgi/Osgi.java | 10 ++- .../src/main/java/com/yahoo/osgi/OsgiImpl.java | 48 ++++++++++++--- .../src/main/java/com/yahoo/osgi/OsgiWrapper.java | 16 +++++ .../yahoo/container/di/ComponentDeconstructor.java | 6 +- .../java/com/yahoo/container/di/Container.java | 36 ++++++++--- .../src/main/java/com/yahoo/container/di/Osgi.java | 9 ++- .../java/com/yahoo/container/di/ContainerTest.java | 22 ++++--- .../com/yahoo/container/di/ContainerTestBase.java | 6 +- .../container/jdisc/DisableOsgiFramework.java | 11 ++++ .../container/jdisc/component/Deconstructor.java | 30 +++++++-- .../jdisc/component/DeconstructorTest.java | 7 ++- .../com/yahoo/jdisc/application/OsgiFramework.java | 18 ++++++ .../com/yahoo/jdisc/core/BundleCollisionHook.java | 3 + .../java/com/yahoo/jdisc/core/FelixFramework.java | 10 ++- .../yahoo/jdisc/test/NonWorkingOsgiFramework.java | 9 +++ .../container/impl/ClassLoaderOsgiFramework.java | 10 ++- .../standalone/StandaloneContainerActivator.java | 9 +++ 22 files changed, 334 insertions(+), 97 deletions(-) create mode 100644 container-core/src/main/java/com/yahoo/container/core/config/testutil/MockOsgiWrapper.java create mode 100644 container-core/src/main/java/com/yahoo/osgi/OsgiWrapper.java 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..4b8f21469d1 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 @@ -10,7 +10,6 @@ import org.osgi.framework.Bundle; import org.osgi.framework.wiring.BundleRevision; import java.io.File; -import java.util.Arrays; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; @@ -26,11 +25,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 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> reference2Bundles = new LinkedHashMap<>(); private final Logger log = Logger.getLogger(BundleLoader.class.getName()); @@ -38,7 +44,6 @@ public class BundleLoader { public BundleLoader(Osgi osgi) { this.osgi = osgi; - initialBundles = Arrays.asList(osgi.getBundles()); } private List obtainBundles(FileReference reference, FileAcquirer fileAcquirer) throws InterruptedException { @@ -46,17 +51,19 @@ public class BundleLoader { return osgi.install(file.getAbsolutePath()); } - /** Returns the number of bundles installed by this call. */ - private int install(List references) { + private void install(List references) { Set bundlesToInstall = new HashSet<>(references); + + // This is just an optimization, as installing a bundle with the same location id returns the already installed bundle. + // It's ok that fileRefs pending uninstall are removed from the map, because they are never in the new set of bundles.. bundlesToInstall.removeAll(reference2Bundles.keySet()); PredicateSplit 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 +104,7 @@ public class BundleLoader { } List bundles = osgi.install(file.getAbsolutePath()); + reference2Bundles.put(reference, bundles); } @@ -113,13 +121,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 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 +146,44 @@ public class BundleLoader { return (bundleRevision.getTypes() & BundleRevision.TYPE_FRAGMENT) != 0; } - /** Returns the number of uninstalled bundles */ - private int retainOnly(List newReferences) { - Set bundlesToRemove = new HashSet<>(Arrays.asList(osgi.getBundles())); + /** + * Returns the bundles to schedule for uninstall after their components have been deconstructed + * and removes the same bundles from the map of active bundles. + */ + private Set getBundlesToUninstall(List newReferences) { + Set 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()); + removeInactiveFileReferences(newReferences); + return bundlesToRemove; + } + + private void removeInactiveFileReferences(List newReferences) { + // Clean up the map of active bundles Set fileReferencesToRemove = new HashSet<>(reference2Bundles.keySet()); fileReferencesToRemove.removeAll(newReferences); - - for (FileReference fileReferenceToRemove : fileReferencesToRemove) { - reference2Bundles.remove(fileReferenceToRemove); - } - return bundlesToRemove.size(); + fileReferencesToRemove.forEach(reference2Bundles::remove); } - public synchronized int use(List 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 use(List newBundles) { + Set bundlesToUninstall = getBundlesToUninstall(newBundles); + osgi.allowDuplicateBundles(bundlesToUninstall); + log.info(() -> bundlesToUninstall.isEmpty() ? "Adding bundles to allowed duplicates: " + bundlesToUninstall : ""); + install(newBundles); 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..ef132694e10 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 @@ -9,6 +9,7 @@ import com.yahoo.component.ComponentSpecification; import com.yahoo.component.provider.ComponentRegistry; import com.yahoo.concurrent.ThreadFactoryFactory; import com.yahoo.config.FileReference; +import com.yahoo.container.core.config.testutil.MockOsgiWrapper; import com.yahoo.container.di.ComponentDeconstructor; import com.yahoo.container.di.Container; import com.yahoo.container.di.componentgraph.core.ComponentGraph; @@ -20,10 +21,9 @@ import com.yahoo.jdisc.application.OsgiFramework; import com.yahoo.jdisc.handler.RequestHandler; import com.yahoo.jdisc.service.ClientProvider; import com.yahoo.jdisc.service.ServerProvider; -import com.yahoo.language.Linguistics; -import com.yahoo.language.simple.SimpleLinguistics; import com.yahoo.log.LogLevel; import com.yahoo.osgi.OsgiImpl; +import com.yahoo.osgi.OsgiWrapper; import com.yahoo.statistics.Statistics; import org.osgi.framework.Bundle; import org.osgi.framework.wiring.BundleWiring; @@ -81,19 +81,30 @@ public class HandlersConfigurerDi { Injector discInjector, OsgiFramework osgiFramework) { - this.vespaContainer = vespaContainer; - osgiWrapper = new OsgiWrapper(osgiFramework, new BundleLoader(new OsgiImpl(osgiFramework))); + this(subscriberFactory, vespaContainer, configId, deconstructor, discInjector, + new ContainerAndDiOsgi(osgiFramework, new BundleLoader(new OsgiImpl(osgiFramework)))); + } + + // Only public for testing + public HandlersConfigurerDi(SubscriberFactory subscriberFactory, + com.yahoo.container.Container vespaContainer, + String configId, + ComponentDeconstructor deconstructor, + Injector discInjector, + OsgiWrapper osgiWrapper) { + this.vespaContainer = vespaContainer; + this.osgiWrapper = osgiWrapper; container = new Container(subscriberFactory, configId, deconstructor, osgiWrapper); getNewComponentGraph(discInjector, false); } - private static class OsgiWrapper extends OsgiImpl implements com.yahoo.container.di.Osgi { + private static class ContainerAndDiOsgi extends OsgiImpl implements OsgiWrapper { private final OsgiFramework osgiFramework; private final BundleLoader bundleLoader; - public OsgiWrapper(OsgiFramework osgiFramework, BundleLoader bundleLoader) { + public ContainerAndDiOsgi(OsgiFramework osgiFramework, BundleLoader bundleLoader) { super(osgiFramework); this.osgiFramework = osgiFramework; this.bundleLoader = bundleLoader; @@ -121,14 +132,9 @@ public class HandlersConfigurerDi { } @Override - public void useBundles(Collection bundles) { + public Set useBundles(Collection 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..9f49b016b68 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; @@ -90,7 +89,7 @@ public class HandlersConfigurerTestWrapper { public HandlersConfigurerTestWrapper(Container container, String configId) { createFiles(configId); - MockOsgi mockOsgi = new MockOsgi(); + MockOsgiWrapper mockOsgiWrapper = new MockOsgiWrapper(); ComponentDeconstructor testDeconstructor = getTestDeconstructor(); configurer = new HandlersConfigurerDi( new CloudSubscriberFactory(configSources), @@ -98,16 +97,17 @@ public class HandlersConfigurerTestWrapper { configId, testDeconstructor, guiceInjector(), - mockOsgi); + mockOsgiWrapper); this.container = container; } 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/container/core/config/testutil/MockOsgiWrapper.java b/container-core/src/main/java/com/yahoo/container/core/config/testutil/MockOsgiWrapper.java new file mode 100644 index 00000000000..a1f564f5682 --- /dev/null +++ b/container-core/src/main/java/com/yahoo/container/core/config/testutil/MockOsgiWrapper.java @@ -0,0 +1,44 @@ +package com.yahoo.container.core.config.testutil; + +import com.yahoo.component.ComponentSpecification; +import com.yahoo.osgi.OsgiWrapper; +import org.osgi.framework.Bundle; + +import java.util.Collection; +import java.util.List; + +import static java.util.Collections.emptyList; + +/** + * @author gjoranv + */ +public class MockOsgiWrapper implements OsgiWrapper { + + @Override + public List getInitialBundles() { + return emptyList(); + } + + @Override + public Bundle[] getBundles() { + return new Bundle[0]; + } + + @Override + public List getCurrentBundles() { + return emptyList(); + } + + @Override + public Bundle getBundle(ComponentSpecification bundleId) { + return null; + } + + @Override + public List install(String absolutePath) { + return emptyList(); + } + + @Override + public void allowDuplicateBundles(Collection 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..d809c493565 100644 --- a/container-core/src/main/java/com/yahoo/osgi/MockOsgi.java +++ b/container-core/src/main/java/com/yahoo/osgi/MockOsgi.java @@ -16,26 +16,28 @@ import java.util.List; public class MockOsgi extends NonWorkingOsgiFramework implements Osgi { @Override - public Bundle[] getBundles() { - return new Bundle[0]; + public List getInitialBundles() { + return Collections.emptyList(); } @Override - public Bundle getBundle(ComponentSpecification bundleId) { - return null; + public Bundle[] getBundles() { + return new Bundle[0]; } @Override - public List install(String absolutePath) { + public List getCurrentBundles() { return Collections.emptyList(); } @Override - public void uninstall(Bundle bundle) { + public Bundle getBundle(ComponentSpecification bundleId) { + return null; } @Override - public void refreshPackages() { + public List install(String absolutePath) { + return Collections.emptyList(); } } 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..8f0acf41f30 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,17 @@ import java.util.List; */ public interface Osgi { + List getInitialBundles(); + Bundle[] getBundles(); + /** Returns all bundles that have not been scheduled for uninstall. */ + List getCurrentBundles(); + Bundle getBundle(ComponentSpecification bundleId); List install(String absolutePath); - void uninstall(Bundle bundle); - - void refreshPackages(); + void allowDuplicateBundles(Collection bundles); } 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..ed93d15c975 100644 --- a/container-core/src/main/java/com/yahoo/osgi/OsgiImpl.java +++ b/container-core/src/main/java/com/yahoo/osgi/OsgiImpl.java @@ -7,19 +7,43 @@ import com.yahoo.container.bundle.BundleInstantiationSpecification; import com.yahoo.jdisc.application.OsgiFramework; import org.osgi.framework.Bundle; import org.osgi.framework.BundleException; +import org.osgi.framework.launch.Framework; +import java.util.Collection; import java.util.List; +import java.util.logging.Logger; /** * @author Tony Vaagenes * @author bratseth */ public class OsgiImpl implements Osgi { + private static final Logger log = Logger.getLogger(OsgiImpl.class.getName()); private final OsgiFramework jdiscOsgi; + // The initial bundles are never scheduled for uninstall + private final List 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; + + 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!"); + log.info("Using " + alwaysCurrentBundle + " to lookup current bundles."); + } + + @Override + public List getInitialBundles() { + return initialBundles; } @Override @@ -28,6 +52,10 @@ public class OsgiImpl implements Osgi { return bundles.toArray(new Bundle[bundles.size()]); } + @Override + public List getCurrentBundles() { + return jdiscOsgi.getBundles(alwaysCurrentBundle); + } public Class resolveClass(BundleInstantiationSpecification spec) { Bundle bundle = getBundle(spec.bundle); @@ -86,8 +114,9 @@ public class OsgiImpl implements Osgi { * @return the bundle match having the highest version, or null if there was no matches */ public Bundle getBundle(ComponentSpecification id) { + log.fine(() -> "Getting bundle for component " + id + ". Set of current bundles: " + getCurrentBundles()); 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,17 +151,16 @@ public class OsgiImpl implements Osgi { } @Override - public void uninstall(Bundle bundle) { - try { - bundle.uninstall(); - } catch (BundleException e) { - throw new RuntimeException(e); - } + public void allowDuplicateBundles(Collection bundles) { + jdiscOsgi.allowDuplicateBundles(bundles); } - @Override - public void refreshPackages() { - jdiscOsgi.refreshPackages(); + private static Bundle firstNonFrameworkBundle(List bundles) { + for (Bundle b : bundles) { + if (! (b instanceof Framework)) + return b; + } + return null; } } diff --git a/container-core/src/main/java/com/yahoo/osgi/OsgiWrapper.java b/container-core/src/main/java/com/yahoo/osgi/OsgiWrapper.java new file mode 100644 index 00000000000..58e19e52ee3 --- /dev/null +++ b/container-core/src/main/java/com/yahoo/osgi/OsgiWrapper.java @@ -0,0 +1,16 @@ +package com.yahoo.osgi; + +import com.yahoo.component.ComponentSpecification; +import org.osgi.framework.Bundle; + +/** + * @author gjoranv + */ +public interface OsgiWrapper extends Osgi, com.yahoo.container.di.Osgi { + + @Override + default Bundle getBundle(ComponentSpecification bundleId) { + return null; + } + +} 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 components); + + void deconstruct(Collection components, Collection 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 obsoleteBundles) { IdentityHashMap 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 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 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 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, ConfigInstance> configsIncludingBootstrapConfigs) { + private Set installBundles(Map, ConfigInstance> configsIncludingBootstrapConfigs) { BundlesConfig bundlesConfig = getConfig(bundlesConfigKey, configsIncludingBootstrapConfigs); - osgi.useBundles(bundlesConfig.bundle()); + return osgi.useBundles(bundlesConfig.bundle()); } private ComponentGraph createComponentsGraph(Map, 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 getConfig(ConfigKey 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 bundles) { + /** + * Returns the set of bundles that is not used by the current application generation, + * and therefore should be scheduled for uninstalling. + */ + default Set useBundles(Collection 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 components) { + public void deconstruct(Collection components, Collection 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 bundles) { + public Set useBundles(Collection bundles) { + return emptySet(); } @Override diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/DisableOsgiFramework.java b/container-disc/src/main/java/com/yahoo/container/jdisc/DisableOsgiFramework.java index 362fefa405d..b81923c5c0a 100644 --- a/container-disc/src/main/java/com/yahoo/container/jdisc/DisableOsgiFramework.java +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/DisableOsgiFramework.java @@ -6,6 +6,7 @@ import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; import org.osgi.framework.BundleException; +import java.util.Collection; import java.util.List; /** @@ -51,6 +52,16 @@ public final class DisableOsgiFramework implements OsgiFramework { throw newException(); } + @Override + public List getBundles(Bundle requestingBundle) { + throw newException(); + } + + @Override + public void allowDuplicateBundles(Collection bundles) { + throw newException(); + } + @Override public void start() throws BundleException { throw newException(); diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/component/Deconstructor.java b/container-disc/src/main/java/com/yahoo/container/jdisc/component/Deconstructor.java index 284e3a69b61..371f29e86a3 100644 --- a/container-disc/src/main/java/com/yahoo/container/jdisc/component/Deconstructor.java +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/component/Deconstructor.java @@ -7,6 +7,8 @@ import com.yahoo.container.di.ComponentDeconstructor; import com.yahoo.container.di.componentgraph.Provider; import com.yahoo.jdisc.SharedResource; import com.yahoo.log.LogLevel; +import org.osgi.framework.Bundle; +import org.osgi.framework.BundleException; import java.time.Duration; import java.util.ArrayList; @@ -17,6 +19,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.logging.Logger; +import static java.util.logging.Level.SEVERE; import static java.util.logging.Level.WARNING; /** @@ -37,7 +40,7 @@ public class Deconstructor implements ComponentDeconstructor { } @Override - public void deconstruct(Collection components) { + public void deconstruct(Collection components, Collection bundles) { Collection destructibleComponents = new ArrayList<>(); for (var component : components) { if (component instanceof AbstractComponent) { @@ -57,20 +60,24 @@ public class Deconstructor implements ComponentDeconstructor { } } if (! destructibleComponents.isEmpty()) - executor.schedule(new DestructComponentTask(destructibleComponents), delay.getSeconds(), TimeUnit.SECONDS); + executor.schedule(new DestructComponentTask(destructibleComponents, bundles), + delay.getSeconds(), TimeUnit.SECONDS); } private static class DestructComponentTask implements Runnable { private final Random random = new Random(System.nanoTime()); private final Collection components; + private final Collection bundles; - DestructComponentTask(Collection components) { + DestructComponentTask(Collection components, + Collection bundles) { this.components = components; + this.bundles = bundles; } /** - * Returns a random delay betweeen 0 and 10 minutes which will be different across identical containers invoking this at the same time. + * Returns a random delay between 0 and 10 minutes which will be different across identical containers invoking this at the same time. * Used to randomize restart to avoid simultaneous cluster restarts. */ private Duration getRandomizedShutdownDelay() { @@ -80,7 +87,6 @@ public class Deconstructor implements ComponentDeconstructor { @Override public void run() { - log.info("Starting deconstruction of " + components.size() + " components"); for (var component : components) { log.info("Starting deconstruction of component " + component); try { @@ -102,7 +108,19 @@ public class Deconstructor implements ComponentDeconstructor { log.log(WARNING, "Non-error not exception throwable thrown when deconstructing component " + component, e); } } - log.info("Finished deconstructing " + components.size() + " components"); + // It should now be safe to uninstall the old bundles. + for (var bundle : bundles) { + try { + log.info("Uninstalling bundle " + bundle); + bundle.uninstall(); + } catch (BundleException e) { + log.log(SEVERE, "Could not uninstall bundle " + bundle); + } + } + // NOTE: It could be tempting to refresh packages here, but if active bundles were using any of + // the removed ones, they would switch wiring in the middle of a generation's lifespan. + // This would happen if the dependent active bundle has not been rebuilt with a new version + // of its dependency(ies). } } diff --git a/container-disc/src/test/java/com/yahoo/container/jdisc/component/DeconstructorTest.java b/container-disc/src/test/java/com/yahoo/container/jdisc/component/DeconstructorTest.java index 395b1aa7c44..345f75f7eb6 100644 --- a/container-disc/src/test/java/com/yahoo/container/jdisc/component/DeconstructorTest.java +++ b/container-disc/src/test/java/com/yahoo/container/jdisc/component/DeconstructorTest.java @@ -10,6 +10,7 @@ import org.junit.Test; import java.util.Collections; +import static java.util.Collections.emptyList; import static java.util.Collections.singleton; import static org.junit.Assert.assertTrue; @@ -28,7 +29,7 @@ public class DeconstructorTest { public void require_abstract_component_destructed() throws InterruptedException { TestAbstractComponent abstractComponent = new TestAbstractComponent(); // Done by executor, so it takes some time even with a 0 delay. - deconstructor.deconstruct(singleton(abstractComponent)); + deconstructor.deconstruct(singleton(abstractComponent), emptyList()); int cnt = 0; while (! abstractComponent.destructed && (cnt++ < 12000)) { Thread.sleep(10); @@ -39,14 +40,14 @@ public class DeconstructorTest { @Test public void require_provider_destructed() { TestProvider provider = new TestProvider(); - deconstructor.deconstruct(singleton(provider)); + deconstructor.deconstruct(singleton(provider), emptyList()); assertTrue(provider.destructed); } @Test public void require_shared_resource_released() { TestSharedResource sharedResource = new TestSharedResource(); - deconstructor.deconstruct(singleton(sharedResource)); + deconstructor.deconstruct(singleton(sharedResource), emptyList()); assertTrue(sharedResource.released); } diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/application/OsgiFramework.java b/jdisc_core/src/main/java/com/yahoo/jdisc/application/OsgiFramework.java index f5346a21a4f..b1aceb81bc6 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/application/OsgiFramework.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/application/OsgiFramework.java @@ -5,6 +5,7 @@ import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; import org.osgi.framework.BundleException; +import java.util.Collection; import java.util.List; /** @@ -13,6 +14,7 @@ import java.util.List; * {@link BundleInstaller} since that provides common convenience methods. * * @author Simon Thoresen Hult + * @author gjoranv */ public interface OsgiFramework { @@ -58,6 +60,8 @@ public interface OsgiFramework { /** * Synchronously refresh all bundles currently loaded. Once this method returns, the * class loaders of all bundles will reflect on the current set of loaded bundles. + * + * NOTE: This method is no longer used by the Jdisc container framework, but kept for completeness. */ void refreshPackages(); @@ -80,6 +84,20 @@ public interface OsgiFramework { */ List bundles(); + /** + * Returns all installed bundles that are visible to the requesting bundle. Bundle visibility + * is controlled via implementations of {@link org.osgi.framework.hooks.bundle.FindHook}; + */ + List getBundles(Bundle requestingBundle); + + /** + * Allows this framework to install duplicates of the given collection of bundles. Duplicate detection + * is handled by the {@link com.yahoo.jdisc.core.BundleCollisionHook}. + * + * @param bundles The bundles to allow duplicates of + */ + void allowDuplicateBundles(Collection bundles); + /** * This method starts the framework instance. Before this method is called, any call to {@link * #installBundle(String)} or {@link #bundles()} will generate a {@link NullPointerException}. diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/BundleCollisionHook.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/BundleCollisionHook.java index 58ad5df9b0d..ae1c81195ce 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/BundleCollisionHook.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/BundleCollisionHook.java @@ -15,6 +15,7 @@ import java.util.HashSet; import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.logging.Logger; /** * A bundle {@link CollisionHook} that contains a set of bundles that are allowed to collide with @@ -26,6 +27,7 @@ import java.util.Set; * @author gjoranv */ public class BundleCollisionHook implements CollisionHook, EventHook, FindHook { + private static Logger log = Logger.getLogger(BundleCollisionHook.class.getName()); private ServiceRegistration registration; private Map allowedDuplicates = new HashMap<>(5); @@ -105,6 +107,7 @@ public class BundleCollisionHook implements CollisionHook, EventHook, FindHook { } } } + log.info("Hiding bundles from bundle '" + context.getBundle() + "': " + bundlesToHide); bundles.removeAll(bundlesToHide); } diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/FelixFramework.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/FelixFramework.java index 19a1707e97c..c14e513fb98 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/FelixFramework.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/FelixFramework.java @@ -117,6 +117,9 @@ public class FelixFramework implements OsgiFramework { return sb.toString(); } + /** + * NOTE: This method is no longer used by the Jdisc container framework, but kept for completeness. + */ @Override public void refreshPackages() { FrameworkWiring wiring = felix.adapt(FrameworkWiring.class); @@ -153,8 +156,13 @@ public class FelixFramework implements OsgiFramework { return Arrays.asList(felix.getBundleContext().getBundles()); } + @Override public List getBundles(Bundle requestingBundle) { - return Arrays.asList(requestingBundle.getBundleContext().getBundles()); + log.fine(() -> "All bundles: " + bundles()); + log.fine(() -> "Getting visible bundles for bundle " + requestingBundle); + List visibleBundles = Arrays.asList(requestingBundle.getBundleContext().getBundles()); + log.fine(() -> "Visible bundles: " + visibleBundles); + return visibleBundles; } public void allowDuplicateBundles(Collection bundles) { diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/test/NonWorkingOsgiFramework.java b/jdisc_core/src/main/java/com/yahoo/jdisc/test/NonWorkingOsgiFramework.java index 6b129e82a45..0f927aa97d3 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/test/NonWorkingOsgiFramework.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/test/NonWorkingOsgiFramework.java @@ -5,6 +5,7 @@ import com.yahoo.jdisc.application.OsgiFramework; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; +import java.util.Collection; import java.util.Collections; import java.util.List; @@ -38,6 +39,14 @@ public class NonWorkingOsgiFramework implements OsgiFramework { return Collections.emptyList(); } + @Override + public List getBundles(Bundle requestingBundle) { + return Collections.emptyList(); + } + + @Override + public void allowDuplicateBundles(Collection bundles) { } + @Override public void start() { diff --git a/standalone-container/src/main/java/com/yahoo/application/container/impl/ClassLoaderOsgiFramework.java b/standalone-container/src/main/java/com/yahoo/application/container/impl/ClassLoaderOsgiFramework.java index 08a70db7562..0d5b4c85264 100644 --- a/standalone-container/src/main/java/com/yahoo/application/container/impl/ClassLoaderOsgiFramework.java +++ b/standalone-container/src/main/java/com/yahoo/application/container/impl/ClassLoaderOsgiFramework.java @@ -63,7 +63,7 @@ public final class ClassLoaderOsgiFramework implements OsgiFramework { @Override public List installBundle(String bundleLocation) { - if (bundleLocation != null && bundleLocation.isEmpty() == false) { + if (bundleLocation != null && ! bundleLocation.isEmpty()) { try { URL url = new URL(bundleLocation); bundleLocations.add(url); @@ -105,6 +105,14 @@ public final class ClassLoaderOsgiFramework implements OsgiFramework { return bundleList; } + @Override + public List getBundles(Bundle requestingBundle) { + return bundleList; + } + + @Override + public void allowDuplicateBundles(Collection bundles) { } + @Override public void start() { } diff --git a/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneContainerActivator.java b/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneContainerActivator.java index 04c3396c95c..cfd5f753c4f 100644 --- a/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneContainerActivator.java +++ b/standalone-container/src/main/java/com/yahoo/container/standalone/StandaloneContainerActivator.java @@ -24,6 +24,7 @@ import java.net.InetSocketAddress; import java.nio.channels.ServerSocketChannel; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Hashtable; import java.util.List; @@ -141,6 +142,14 @@ public class StandaloneContainerActivator implements BundleActivator { return Collections.emptyList(); } + @Override + public List getBundles(Bundle requestingBundle) { + return Collections.emptyList(); + } + + @Override + public void allowDuplicateBundles(Collection bundles) { } + @Override public void start() { } -- cgit v1.2.3