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

2 files changed, 24 insertions, 23 deletions

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);
     }