diff options
author | Jon Marius Venstad <jonmv@users.noreply.github.com> | 2020-12-07 15:52:52 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-12-07 15:52:52 +0100 |
commit | f561afbb3620b796d7321cde8c11d75a6bc6b2cf (patch) | |
tree | 69b7afbf913d23cb65909065b092d7e77e70a837 | |
parent | 339996348953bc9661c9cef12af7e944f9f66bc8 (diff) |
Revert "Revert "Always deconstruct in reverse creation order, including Provider objects""
13 files changed, 80 insertions, 38 deletions
diff --git a/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/Reindexer.java b/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/Reindexer.java index 5c7b6839d8a..4349a662025 100644 --- a/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/Reindexer.java +++ b/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/Reindexer.java @@ -169,7 +169,7 @@ public class Reindexer { // Wait until done; or until termination is forced, in which we shut down the visitor session immediately. phaser.arriveAndAwaitAdvance(); // Synchronize with visitor completion. - sessionShutdown.run(); // Shutdown aborts the session, then waits for it to terminate normally. + sessionShutdown.run(); // Shutdown aborts the session unless already complete, then waits for it to terminate normally. switch (control.getResult().getCode()) { default: diff --git a/component/abi-spec.json b/component/abi-spec.json index 5665075f1f2..2dfa3e0d71d 100644 --- a/component/abi-spec.json +++ b/component/abi-spec.json @@ -2,7 +2,8 @@ "com.yahoo.component.AbstractComponent": { "superClass": "java.lang.Object", "interfaces": [ - "com.yahoo.component.Component" + "com.yahoo.component.Component", + "com.yahoo.component.Deconstructable" ], "attributes": [ "public" @@ -105,6 +106,19 @@ ], "fields": [] }, + "com.yahoo.component.Deconstructable": { + "superClass": "java.lang.Object", + "interfaces": [], + "attributes": [ + "public", + "interface", + "abstract" + ], + "methods": [ + "public abstract void deconstruct()" + ], + "fields": [] + }, "com.yahoo.component.Version": { "superClass": "java.lang.Object", "interfaces": [ diff --git a/component/src/main/java/com/yahoo/component/AbstractComponent.java b/component/src/main/java/com/yahoo/component/AbstractComponent.java index fcaa1e62d56..18a19f3e238 100644 --- a/component/src/main/java/com/yahoo/component/AbstractComponent.java +++ b/component/src/main/java/com/yahoo/component/AbstractComponent.java @@ -11,7 +11,7 @@ import java.lang.reflect.Method; * * @author bratseth */ -public class AbstractComponent implements Component { +public class AbstractComponent implements Component, Deconstructable { private static final MethodCache deconstructMethods = new MethodCache("deconstruct"); @@ -129,6 +129,7 @@ public class AbstractComponent implements Component { * <p> * This default implementation does nothing. */ + @Override public void deconstruct() { } /** diff --git a/component/src/main/java/com/yahoo/component/Deconstructable.java b/component/src/main/java/com/yahoo/component/Deconstructable.java new file mode 100644 index 00000000000..4da39cd3420 --- /dev/null +++ b/component/src/main/java/com/yahoo/component/Deconstructable.java @@ -0,0 +1,13 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.component; + +/** + * A component in the component graph that should be deconstructed, to release resources. + * + * @author jonmv + */ +public interface Deconstructable { + + void deconstruct(); + +} diff --git a/container-di/abi-spec.json b/container-di/abi-spec.json index a9dda2ae224..02cc29cd07f 100644 --- a/container-di/abi-spec.json +++ b/container-di/abi-spec.json @@ -1,15 +1,16 @@ { "com.yahoo.container.di.componentgraph.Provider": { "superClass": "java.lang.Object", - "interfaces": [], + "interfaces": [ + "com.yahoo.component.Deconstructable" + ], "attributes": [ "public", "interface", "abstract" ], "methods": [ - "public abstract java.lang.Object get()", - "public abstract void deconstruct()" + "public abstract java.lang.Object get()" ], "fields": [] } 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 61497cf71bc..4e3881a6fe6 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 @@ -4,6 +4,7 @@ package com.yahoo.container.di; import org.osgi.framework.Bundle; import java.util.Collection; +import java.util.List; /** * @author gjoranv @@ -11,6 +12,7 @@ import java.util.Collection; */ public interface ComponentDeconstructor { - void deconstruct(Collection<Object> components, Collection<Bundle> bundles); + /** Deconstructs the given components in order, then the given bundles. */ + void deconstruct(List<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 af580767a17..7fc2f2e55bc 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 @@ -21,6 +21,7 @@ import com.yahoo.container.di.config.SubscriberFactory; import com.yahoo.vespa.config.ConfigKey; import org.osgi.framework.Bundle; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashSet; @@ -159,10 +160,16 @@ public class Container { 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(), obsoleteBundles); + Map<Object, ?> newComponents = new IdentityHashMap<>(newGraph.size()); + for (Object component : newGraph.allConstructedComponentsAndProviders()) + newComponents.put(component, null); + + List<Object> obsoleteComponents = new ArrayList<>(); + for (Object component : oldGraph.allConstructedComponentsAndProviders()) + if ( ! newComponents.containsKey(component)) + obsoleteComponents.add(component); + + componentDeconstructor.deconstruct(obsoleteComponents, obsoleteBundles); } private Set<Bundle> installApplicationBundles(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> configsIncludingBootstrapConfigs) { diff --git a/container-di/src/main/java/com/yahoo/container/di/componentgraph/Provider.java b/container-di/src/main/java/com/yahoo/container/di/componentgraph/Provider.java index a2d193654ea..3fd3195e5dd 100644 --- a/container-di/src/main/java/com/yahoo/container/di/componentgraph/Provider.java +++ b/container-di/src/main/java/com/yahoo/container/di/componentgraph/Provider.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.componentgraph; +import com.yahoo.component.Deconstructable; + /** * <p>Provides a component of the parameter type T. * If (and only if) dependency injection does not have a component of type T, @@ -17,9 +19,8 @@ package com.yahoo.container.di.componentgraph; * @author Tony Vaagenes * @author gjoranv */ -public interface Provider<T> { +public interface Provider<T> extends Deconstructable { T get(); - void deconstruct(); } diff --git a/container-di/src/main/java/com/yahoo/container/di/componentgraph/core/ComponentGraph.java b/container-di/src/main/java/com/yahoo/container/di/componentgraph/core/ComponentGraph.java index 256c965052a..fef2809f236 100644 --- a/container-di/src/main/java/com/yahoo/container/di/componentgraph/core/ComponentGraph.java +++ b/container-di/src/main/java/com/yahoo/container/di/componentgraph/core/ComponentGraph.java @@ -166,6 +166,7 @@ public class ComponentGraph { } } + /** All constructed components and providers of this, in reverse creation order, i.e., suited for ordered deconstruction. */ public List<Object> allConstructedComponentsAndProviders() { List<Node> orderedNodes = topologicalSort(nodes()); Collections.reverse(orderedNodes); 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 19f277ff8fb..d39e8a36aed 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 @@ -19,6 +19,7 @@ import org.osgi.framework.Bundle; import org.osgi.framework.BundleException; import java.util.Collection; +import java.util.List; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -380,7 +381,7 @@ public class ContainerTest extends ContainerTestBase { public static class TestDeconstructor implements ComponentDeconstructor { @Override - public void deconstruct(Collection<Object> components, Collection<Bundle> bundles) { + public void deconstruct(List<Object> components, Collection<Bundle> bundles) { components.forEach(component -> { if (component instanceof DestructableComponent) { DestructableComponent vespaComponent = (DestructableComponent) component; 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 e60e8d407cd..0bb60c8aca8 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 @@ -2,10 +2,13 @@ package com.yahoo.container.jdisc.component; import com.yahoo.component.AbstractComponent; +import com.yahoo.component.Deconstructable; import com.yahoo.concurrent.ThreadFactoryFactory; import com.yahoo.container.di.ComponentDeconstructor; import com.yahoo.container.di.componentgraph.Provider; import com.yahoo.jdisc.SharedResource; + +import java.util.List; import java.util.logging.Level; import org.osgi.framework.Bundle; import org.osgi.framework.BundleException; @@ -32,7 +35,7 @@ public class Deconstructor implements ComponentDeconstructor { private static final Logger log = Logger.getLogger(Deconstructor.class.getName()); - private final ScheduledExecutorService executor = + final ScheduledExecutorService executor = Executors.newScheduledThreadPool(1, ThreadFactoryFactory.getThreadFactory("component-deconstructor")); private final Duration delay; @@ -42,8 +45,8 @@ public class Deconstructor implements ComponentDeconstructor { } @Override - public void deconstruct(Collection<Object> components, Collection<Bundle> bundles) { - Collection<AbstractComponent> destructibleComponents = new ArrayList<>(); + public void deconstruct(List<Object> components, Collection<Bundle> bundles) { + Collection<Deconstructable> destructibleComponents = new ArrayList<>(); for (var component : components) { if (component instanceof AbstractComponent) { AbstractComponent abstractComponent = (AbstractComponent) component; @@ -51,10 +54,7 @@ public class Deconstructor implements ComponentDeconstructor { destructibleComponents.add(abstractComponent); } } else if (component instanceof Provider) { - // TODO Providers should most likely be deconstructed similarly to AbstractComponent - log.log(FINE, () -> "Starting deconstruction of provider " + component); - ((Provider<?>) component).deconstruct(); - log.log(FINE, () -> "Finished deconstruction of provider " + component); + destructibleComponents.add((Deconstructable) component); } else if (component instanceof SharedResource) { log.log(FINE, () -> "Releasing container reference to resource " + component); // No need to delay release, as jdisc does ref-counting @@ -69,10 +69,10 @@ public class Deconstructor implements ComponentDeconstructor { private static class DestructComponentTask implements Runnable { private final Random random = new Random(System.nanoTime()); - private final Collection<AbstractComponent> components; + private final Collection<Deconstructable> components; private final Collection<Bundle> bundles; - DestructComponentTask(Collection<AbstractComponent> components, Collection<Bundle> bundles) { + DestructComponentTask(Collection<Deconstructable> components, Collection<Bundle> bundles) { this.components = components; this.bundles = bundles; } @@ -89,10 +89,10 @@ public class Deconstructor implements ComponentDeconstructor { @Override public void run() { for (var component : components) { - log.log(FINE, () -> "Starting deconstruction of component " + component); + log.log(FINE, () -> "Starting deconstruction of " + component); try { component.deconstruct(); - log.log(FINE, () -> "Finished deconstructing of component " + component); + log.log(FINE, () -> "Finished deconstructing of " + component); } catch (Exception | NoClassDefFoundError e) { // May get class not found due to it being already unloaded log.log(WARNING, "Exception thrown when deconstructing component " + component, e); } catch (Error e) { 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 efdc8f44c17..9ae05f5c193 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 @@ -9,6 +9,9 @@ import com.yahoo.jdisc.SharedResource; import org.junit.Before; import org.junit.Test; +import java.util.List; +import java.util.concurrent.TimeUnit; + import static java.util.Collections.emptyList; import static java.util.Collections.singleton; import static org.junit.Assert.assertTrue; @@ -28,25 +31,25 @@ 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), emptyList()); - int cnt = 0; - while (! abstractComponent.destructed && (cnt++ < 12000)) { - Thread.sleep(10); - } + deconstructor.deconstruct(List.of(abstractComponent), emptyList()); + deconstructor.executor.shutdown(); + deconstructor.executor.awaitTermination(1, TimeUnit.MINUTES); assertTrue(abstractComponent.destructed); } @Test - public void require_provider_destructed() { + public void require_provider_destructed() throws InterruptedException { TestProvider provider = new TestProvider(); - deconstructor.deconstruct(singleton(provider), emptyList()); + deconstructor.deconstruct(List.of(provider), emptyList()); + deconstructor.executor.shutdown(); + deconstructor.executor.awaitTermination(1, TimeUnit.MINUTES); assertTrue(provider.destructed); } @Test public void require_shared_resource_released() { TestSharedResource sharedResource = new TestSharedResource(); - deconstructor.deconstruct(singleton(sharedResource), emptyList()); + deconstructor.deconstruct(List.of(sharedResource), emptyList()); assertTrue(sharedResource.released); } @@ -55,10 +58,8 @@ public class DeconstructorTest { var bundle = new UninstallableMockBundle(); // Done by executor, so it takes some time even with a 0 delay. deconstructor.deconstruct(emptyList(), singleton(bundle)); - int cnt = 0; - while (! bundle.uninstalled && (cnt++ < 12000)) { - Thread.sleep(10); - } + deconstructor.executor.shutdown(); + deconstructor.executor.awaitTermination(1, TimeUnit.MINUTES); assertTrue(bundle.uninstalled); } diff --git a/vespajlib/src/main/java/com/yahoo/collections/TinyIdentitySet.java b/vespajlib/src/main/java/com/yahoo/collections/TinyIdentitySet.java index 95fccd3fce8..9779aea0e4a 100644 --- a/vespajlib/src/main/java/com/yahoo/collections/TinyIdentitySet.java +++ b/vespajlib/src/main/java/com/yahoo/collections/TinyIdentitySet.java @@ -18,7 +18,7 @@ import java.util.Set; * in IdentityHashMap, where the key set is often used as an identity set. * </p> * - * @author <a href="mailto:steinar@yahoo-inc.com">Steinar Knutsen</a> + * @author Steinar Knutsen * @since 5.1.4 * @see java.util.IdentityHashMap * |