Revert "Revert "Revert "Always deconstruct in reverse creation order, including Provider objects"""

13 files changed, 38 insertions, 80 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 4349a662025..5c7b6839d8a 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 unless already complete, then waits for it to terminate normally.
+        sessionShutdown.run(); // Shutdown aborts the session, 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 2dfa3e0d71d..5665075f1f2 100644
--- a/component/abi-spec.json
+++ b/component/abi-spec.json
@@ -2,8 +2,7 @@
   "com.yahoo.component.AbstractComponent": {
     "superClass": "java.lang.Object",
     "interfaces": [
-      "com.yahoo.component.Component",
-      "com.yahoo.component.Deconstructable"
+      "com.yahoo.component.Component"
     ],
     "attributes": [
       "public"
@@ -106,19 +105,6 @@
     ],
     "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 18a19f3e238..fcaa1e62d56 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, Deconstructable {
+public class AbstractComponent implements Component {
 
     private static final MethodCache deconstructMethods = new MethodCache("deconstruct");
 
@@ -129,7 +129,6 @@ public class AbstractComponent implements Component, Deconstructable {
      * <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
deleted file mode 100644
index 4da39cd3420..00000000000
--- a/component/src/main/java/com/yahoo/component/Deconstructable.java
+++ /dev/null
@@ -1,13 +0,0 @@
-// 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 02cc29cd07f..a9dda2ae224 100644
--- a/container-di/abi-spec.json
+++ b/container-di/abi-spec.json
@@ -1,16 +1,15 @@
 {
   "com.yahoo.container.di.componentgraph.Provider": {
     "superClass": "java.lang.Object",
-    "interfaces": [
-      "com.yahoo.component.Deconstructable"
-    ],
+    "interfaces": [],
     "attributes": [
       "public",
       "interface",
       "abstract"
     ],
     "methods": [
-      "public abstract java.lang.Object get()"
+      "public abstract java.lang.Object get()",
+      "public abstract void deconstruct()"
     ],
     "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 4e3881a6fe6..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
@@ -4,7 +4,6 @@ package com.yahoo.container.di;
 import org.osgi.framework.Bundle;
 
 import java.util.Collection;
-import java.util.List;
 
 /**
  * @author gjoranv
@@ -12,7 +11,6 @@ import java.util.List;
  */
 public interface ComponentDeconstructor {
 
-    /** Deconstructs the given components in order, then the given bundles. */
-    void deconstruct(List<Object> components, Collection<Bundle> bundles);
+    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 7fc2f2e55bc..af580767a17 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,7 +21,6 @@ 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;
@@ -160,16 +159,10 @@ public class Container {
     private void deconstructObsoleteComponents(ComponentGraph oldGraph,
                                                ComponentGraph newGraph,
                                                Collection<Bundle> 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);
+        IdentityHashMap<Object, Object> oldComponents = new IdentityHashMap<>();
+        oldGraph.allConstructedComponentsAndProviders().forEach(c -> oldComponents.put(c, null));
+        newGraph.allConstructedComponentsAndProviders().forEach(oldComponents::remove);
+        componentDeconstructor.deconstruct(oldComponents.keySet(), 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 3fd3195e5dd..a2d193654ea 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,8 +1,6 @@
 // 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,
@@ -19,8 +17,9 @@ import com.yahoo.component.Deconstructable;
  * @author Tony Vaagenes
  * @author gjoranv
  */
-public interface Provider<T> extends Deconstructable {
+public interface Provider<T> {
 
     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 fef2809f236..256c965052a 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,7 +166,6 @@ 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 d39e8a36aed..19f277ff8fb 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,7 +19,6 @@ 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;
@@ -381,7 +380,7 @@ public class ContainerTest extends ContainerTestBase {
 
     public static class TestDeconstructor implements ComponentDeconstructor {
         @Override
-        public void deconstruct(List<Object> components, Collection<Bundle> bundles) {
+        public void deconstruct(Collection<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 0bb60c8aca8..e60e8d407cd 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,13 +2,10 @@
 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;
@@ -35,7 +32,7 @@ public class Deconstructor implements ComponentDeconstructor {
 
     private static final Logger log = Logger.getLogger(Deconstructor.class.getName());
 
-    final ScheduledExecutorService executor =
+    private final ScheduledExecutorService executor =
             Executors.newScheduledThreadPool(1, ThreadFactoryFactory.getThreadFactory("component-deconstructor"));
 
     private final Duration delay;
@@ -45,8 +42,8 @@ public class Deconstructor implements ComponentDeconstructor {
     }
 
     @Override
-    public void deconstruct(List<Object> components, Collection<Bundle> bundles) {
-        Collection<Deconstructable> destructibleComponents = new ArrayList<>();
+    public void deconstruct(Collection<Object> components, Collection<Bundle> bundles) {
+        Collection<AbstractComponent> destructibleComponents = new ArrayList<>();
         for (var component : components) {
             if (component instanceof AbstractComponent) {
                 AbstractComponent abstractComponent = (AbstractComponent) component;
@@ -54,7 +51,10 @@ public class Deconstructor implements ComponentDeconstructor {
                     destructibleComponents.add(abstractComponent);
                 }
             } else if (component instanceof Provider) {
-                destructibleComponents.add((Deconstructable) component);
+                // 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);
             } 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<Deconstructable> components;
+        private final Collection<AbstractComponent> components;
         private final Collection<Bundle> bundles;
 
-        DestructComponentTask(Collection<Deconstructable> components, Collection<Bundle> bundles) {
+        DestructComponentTask(Collection<AbstractComponent> 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);
+                log.log(FINE, () -> "Starting deconstruction of component " + component);
                 try {
                     component.deconstruct();
-                    log.log(FINE, () -> "Finished deconstructing of " + component);
+                    log.log(FINE, () -> "Finished deconstructing of component " + 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 9ae05f5c193..efdc8f44c17 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,9 +9,6 @@ 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;
@@ -31,25 +28,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(List.of(abstractComponent), emptyList());
-        deconstructor.executor.shutdown();
-        deconstructor.executor.awaitTermination(1, TimeUnit.MINUTES);
+        deconstructor.deconstruct(singleton(abstractComponent), emptyList());
+        int cnt = 0;
+        while (! abstractComponent.destructed && (cnt++ < 12000)) {
+            Thread.sleep(10);
+        }
         assertTrue(abstractComponent.destructed);
     }
 
     @Test
-    public void require_provider_destructed() throws InterruptedException {
+    public void require_provider_destructed() {
         TestProvider provider = new TestProvider();
-        deconstructor.deconstruct(List.of(provider), emptyList());
-        deconstructor.executor.shutdown();
-        deconstructor.executor.awaitTermination(1, TimeUnit.MINUTES);
+        deconstructor.deconstruct(singleton(provider), emptyList());
         assertTrue(provider.destructed);
     }
 
     @Test
     public void require_shared_resource_released() {
         TestSharedResource sharedResource = new TestSharedResource();
-        deconstructor.deconstruct(List.of(sharedResource), emptyList());
+        deconstructor.deconstruct(singleton(sharedResource), emptyList());
         assertTrue(sharedResource.released);
     }
 
@@ -58,8 +55,10 @@ 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));
-        deconstructor.executor.shutdown();
-        deconstructor.executor.awaitTermination(1, TimeUnit.MINUTES);
+        int cnt = 0;
+        while (! bundle.uninstalled && (cnt++ < 12000)) {
+            Thread.sleep(10);
+        }
         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 9779aea0e4a..95fccd3fce8 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 Steinar Knutsen
+ * @author <a href="mailto:steinar@yahoo-inc.com">Steinar Knutsen</a>
  * @since 5.1.4
  * @see java.util.IdentityHashMap
  *