Merge pull request #15826 from vespa-engine/gjoranv/wait-for-deconstruct-upon-shutdown

Gjoranv/wait for deconstruct upon shutdown

3 files changed, 91 insertions, 20 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 835326e0904..f266e3782ef 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
@@ -335,7 +335,7 @@ public final class ConfiguredApplication implements Application {
         return new HandlersConfigurerDi(subscriberFactory,
                                         Container.get(),
                                         configId,
-                                        new Deconstructor(true),
+                                        new Deconstructor(Deconstructor.Mode.RECONFIG),
                                         discInjector,
                                         osgiFramework);
     }
@@ -367,7 +367,7 @@ public final class ConfiguredApplication implements Application {
         }
 
         log.info("Stop: Shutting container down");
-        configurer.shutdown(new Deconstructor(false));
+        configurer.shutdown(new Deconstructor(Deconstructor.Mode.SHUTDOWN));
         slobrokConfigSubscriber.ifPresent(SlobrokConfigSubscriber::shutdown);
         Container.get().shutdown();
 
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..d6a241c30c9 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,19 +7,21 @@ 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;
 
 import java.time.Duration;
 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.Executors;
 import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import static java.util.logging.Level.FINE;
@@ -35,13 +37,31 @@ public class Deconstructor implements ComponentDeconstructor {
 
     private static final Logger log = Logger.getLogger(Deconstructor.class.getName());
 
-    final ScheduledExecutorService executor =
-            Executors.newScheduledThreadPool(1, ThreadFactoryFactory.getThreadFactory("component-deconstructor"));
+    private static final Duration RECONFIG_DECONSTRUCT_DELAY = Duration.ofSeconds(60);
+
+    // This must be smaller than the shutdownDeadlineExecutor delay in ConfiguredApplication
+    private static final Duration SHUTDOWN_DECONSTRUCT_TIMEOUT = Duration.ofSeconds(45);
+
+    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.
+    }
+
+    private final ScheduledExecutorService executor =
+            Executors.newScheduledThreadPool(2, ThreadFactoryFactory.getThreadFactory("component-deconstructor"));
 
+    private final Mode mode;
     private final Duration delay;
 
-    public Deconstructor(boolean delayDeconstruction) {
-        this.delay = delayDeconstruction ? Duration.ofSeconds(60) : Duration.ZERO;
+    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;
     }
 
     @Override
@@ -61,9 +81,27 @@ public class Deconstructor implements ComponentDeconstructor {
                 ((SharedResource) component).release();
             }
         }
-        if (! destructibleComponents.isEmpty() || ! bundles.isEmpty())
-            executor.schedule(new DestructComponentTask(destructibleComponents, bundles),
+        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);
+            }
+        }
+    }
+
+    private void waitFor(ScheduledFuture<?> task, Duration timeout) {
+        try {
+            log.info("Waiting up to " + timeout.toSeconds() + " seconds for all components to deconstruct.");
+            task.get(timeout.toMillis(), TimeUnit.MILLISECONDS);
+        } 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.");
+        }
     }
 
     private static class DestructComponentTask implements Runnable {
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..eef5b191e75 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,8 +9,10 @@ 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.concurrent.TimeUnit;
+import java.util.function.Supplier;
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singleton;
@@ -24,16 +26,24 @@ public class DeconstructorTest {
 
     @Before
     public void init() {
-        deconstructor = new Deconstructor(false);
+        deconstructor = new Deconstructor(Deconstructor.Mode.RECONFIG, Duration.ZERO);
+    }
+
+    @Test
+    public void deconstruct_is_synchronous_in_shutdown_mode() {
+        deconstructor = new Deconstructor(Deconstructor.Mode.SHUTDOWN);
+
+        var slowDeconstructComponent = new SlowDeconstructComponent();
+        deconstructor.deconstruct(List.of(slowDeconstructComponent), emptyList());
+        assertTrue(slowDeconstructComponent.destructed);
     }
 
     @Test
     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);
+
+        waitForDeconstructToComplete(() -> abstractComponent.destructed);
         assertTrue(abstractComponent.destructed);
     }
 
@@ -41,8 +51,8 @@ public class DeconstructorTest {
     public void require_provider_destructed() throws InterruptedException {
         TestProvider provider = new TestProvider();
         deconstructor.deconstruct(List.of(provider), emptyList());
-        deconstructor.executor.shutdown();
-        deconstructor.executor.awaitTermination(1, TimeUnit.MINUTES);
+
+        waitForDeconstructToComplete(() -> provider.destructed);
         assertTrue(provider.destructed);
     }
 
@@ -58,16 +68,38 @@ 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);
+
+        waitForDeconstructToComplete(() -> bundle.uninstalled);
         assertTrue(bundle.uninstalled);
     }
 
+    // Deconstruct is async in RECONFIG mode, so must wait even with a zero delay.
+    private void waitForDeconstructToComplete(Supplier<Boolean> destructed) throws InterruptedException {
+        var end = Instant.now().plusSeconds(30);
+        while (! destructed.get() && Instant.now().isBefore(end)) {
+            Thread.sleep(10);
+        }
+    }
+
     private static class TestAbstractComponent extends AbstractComponent {
         boolean destructed = false;
         @Override public void deconstruct() { destructed = true; }
     }
 
+    private static class SlowDeconstructComponent extends AbstractComponent {
+        boolean destructed = false;
+        @Override
+        public void deconstruct() {
+            // Add delay to verify that the Deconstructor waits until this is complete before returning.
+            try {
+                Thread.sleep(1000);
+            } catch (InterruptedException e) {
+                throw new RuntimeException("The delayed deconstruct was interrupted.");
+            }
+            destructed = true;
+        }
+    }
+
     private static class TestProvider implements Provider<Void> {
         volatile boolean destructed = false;
 
@@ -88,4 +120,5 @@ public class DeconstructorTest {
             uninstalled = true;
         }
     }
+
 }