Redesign ComponentDeconstructor to start deconstruction immediately

There is no need to wait with deconstruction after new generation
as the container knows when an old graph can be safely GCed.
Add shutdown() that waits for all previous graphs to complete deconstruction.

3 files changed, 24 insertions, 45 deletions

diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java b/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java
index cf81b39db06..d686bb9a3dd 100644
--- a/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java
+++ b/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java
@@ -351,7 +351,7 @@ public final class ConfiguredApplication implements Application {
         return new HandlersConfigurerDi(subscriberFactory,
                                         Container.get(),
                                         configId,
-                                        new Deconstructor(Deconstructor.Mode.RECONFIG),
+                                        new Deconstructor(),
                                         discInjector,
                                         osgiFramework);
     }
@@ -385,7 +385,7 @@ public final class ConfiguredApplication implements Application {
         CountDownLatch latch = new CountDownLatch(1);
         activator.activateContainer(null)
                 .notifyTermination(() -> {
-                    configurer.shutdown(new Deconstructor(Deconstructor.Mode.SHUTDOWN));
+                    configurer.shutdown();
                     slobrokConfigSubscriber.ifPresent(SlobrokConfigSubscriber::shutdown);
                     Container.get().shutdown();
                     unregisterInSlobrok();
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 e224601498b..ee592c2e8d1 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,7 @@ import com.yahoo.concurrent.ThreadFactoryFactory;
 import com.yahoo.container.di.ComponentDeconstructor;
 import com.yahoo.container.di.componentgraph.Provider;
 import com.yahoo.jdisc.SharedResource;
+import com.yahoo.yolean.UncheckedInterruptedException;
 import org.osgi.framework.Bundle;
 import org.osgi.framework.BundleException;
 
@@ -15,12 +16,9 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Random;
-import java.util.concurrent.ExecutionException;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.ScheduledFuture;
-import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -37,32 +35,17 @@ public class Deconstructor implements ComponentDeconstructor {
 
     private static final Logger log = Logger.getLogger(Deconstructor.class.getName());
 
-    private static final Duration RECONFIG_DECONSTRUCT_DELAY = Duration.ofSeconds(60);
+    private final ExecutorService executor =
+            Executors.newFixedThreadPool(1, ThreadFactoryFactory.getThreadFactory("component-deconstructor"));
 
     // This must be smaller than the shutdownDeadlineExecutor delay in ConfiguredApplication
-    private static final Duration SHUTDOWN_DECONSTRUCT_TIMEOUT = Duration.ofSeconds(45);
+    private final Duration shutdownTimeout;
 
-    public enum Mode {
-        RECONFIG,  // Delay deconstruction to allow old components to finish processing in-flight requests.
-        SHUTDOWN   // The container is shutting down. Start deconstructing immediately, and wait until all components
-                   // are deconstructed, to prevent shutting down while deconstruct is in progress.
+    public Deconstructor(Duration shutdownTimeout) {
+        this.shutdownTimeout = shutdownTimeout;
     }
 
-    private final ScheduledExecutorService executor =
-            new ScheduledThreadPoolExecutor(1, ThreadFactoryFactory.getThreadFactory("component-deconstructor"));
-
-    private final Mode mode;
-    private final Duration delay;
-
-    public Deconstructor(Mode mode) {
-        this(mode, (mode == Mode.RECONFIG) ? RECONFIG_DECONSTRUCT_DELAY : Duration.ZERO);
-    }
-
-    // For testing only
-    Deconstructor(Mode mode, Duration reconfigDeconstructDelay) {
-        this.mode = mode;
-        this.delay = reconfigDeconstructDelay;
-    }
+    public Deconstructor() { this(Duration.ofSeconds(45)); }
 
     @Override
     public void deconstruct(List<Object> components, Collection<Bundle> bundles) {
@@ -81,25 +64,21 @@ public class Deconstructor implements ComponentDeconstructor {
             }
         }
         if (!destructibleComponents.isEmpty() || !bundles.isEmpty()) {
-            var task = executor.schedule(new DestructComponentTask(destructibleComponents, bundles),
-                              delay.getSeconds(), TimeUnit.SECONDS);
-            if (mode.equals(Mode.SHUTDOWN)) {
-                waitFor(task, SHUTDOWN_DECONSTRUCT_TIMEOUT);
-            }
+            executor.execute(new DestructComponentTask(destructibleComponents, bundles));
         }
     }
 
-    private void waitFor(ScheduledFuture<?> task, Duration timeout) {
+    @Override
+    public void shutdown() {
+        executor.shutdown();
         try {
-            log.info("Waiting up to " + timeout.toSeconds() + " seconds for all components to deconstruct.");
-            task.get(timeout.toMillis(), TimeUnit.MILLISECONDS);
+            log.info("Waiting up to " + shutdownTimeout.toSeconds() + " seconds for all previous components graphs to deconstruct.");
+            if (!executor.awaitTermination(shutdownTimeout.getSeconds(), TimeUnit.SECONDS)) {
+                log.warning("Waiting for deconstruction timed out.");
+            }
         } catch (InterruptedException e) {
             log.info("Interrupted while waiting for component deconstruction to finish.");
-            Thread.currentThread().interrupt();
-        } catch (ExecutionException e) {
-            log.warning("Component deconstruction threw an exception: " + e.getMessage());
-        } catch (TimeoutException e) {
-            log.warning("Component deconstruction timed out.");
+            throw new UncheckedInterruptedException(e, true);
         }
     }
 
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 0a7d3f45e82..579275deb0f 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,7 +9,6 @@ import com.yahoo.jdisc.SharedResource;
 import org.junit.Before;
 import org.junit.Test;
 
-import java.time.Duration;
 import java.time.Instant;
 import java.util.List;
 import java.util.function.Supplier;
@@ -26,15 +25,16 @@ public class DeconstructorTest {
 
     @Before
     public void init() {
-        deconstructor = new Deconstructor(Deconstructor.Mode.RECONFIG, Duration.ZERO);
+        deconstructor = new Deconstructor();
     }
 
     @Test
-    public void deconstruct_is_synchronous_in_shutdown_mode() {
-        deconstructor = new Deconstructor(Deconstructor.Mode.SHUTDOWN);
+    public void deconstructor_waits_for_completion_on_shutdown() {
+        deconstructor = new Deconstructor();
 
         var slowDeconstructComponent = new SlowDeconstructComponent();
         deconstructor.deconstruct(List.of(slowDeconstructComponent), emptyList());
+        deconstructor.shutdown();
         assertTrue(slowDeconstructComponent.destructed);
     }