Keep old container alive until servers are switched

9 files changed, 62 insertions, 43 deletions

diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/application/DeactivatedContainer.java b/jdisc_core/src/main/java/com/yahoo/jdisc/application/DeactivatedContainer.java
index 88943617fef..af28d756fe6 100644
--- a/jdisc_core/src/main/java/com/yahoo/jdisc/application/DeactivatedContainer.java
+++ b/jdisc_core/src/main/java/com/yahoo/jdisc/application/DeactivatedContainer.java
@@ -7,13 +7,14 @@ import com.yahoo.jdisc.Response;
 import com.yahoo.jdisc.handler.ContentChannel;
 
 /**
- * <p>This interface represents a {@link Container} which has been deactivated. An instance of this class is returned by
- * the {@link ContainerActivator#activateContainer(ContainerBuilder)} method, and is used to schedule a cleanup task
- * that is executed once the the deactivated Container has terminated.</p>
+ * <p>This interface represents a {@link Container} which is in the process of being deactivated. Closing this
+ * releases the last non-request-response reference to the container, and enables its termination.
+ * An instance of this class is returned by the {@link ContainerActivator#activateContainer(ContainerBuilder)} method,
+ * and is used to schedule a cleanup task that is executed once the the deactivated Container has terminated. </p>
  *
  * @author Simon Thoresen Hult
  */
-public interface DeactivatedContainer {
+public interface DeactivatedContainer extends AutoCloseable {
 
     /**
      * <p>Returns the context object that was previously attached to the corresponding {@link ContainerBuilder} through
@@ -35,4 +36,11 @@ public interface DeactivatedContainer {
      */
     void notifyTermination(Runnable task);
 
+    /**
+     * <p>Close this DeactivatedContainer. This releases the last non-request-response reference to the container, and
+     * enables its termination.</p>
+     */
+    @Override
+    void close();
+
 }
diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/application/package-info.java b/jdisc_core/src/main/java/com/yahoo/jdisc/application/package-info.java
index 007f8242ee4..179dadbb040 100644
--- a/jdisc_core/src/main/java/com/yahoo/jdisc/application/package-info.java
+++ b/jdisc_core/src/main/java/com/yahoo/jdisc/application/package-info.java
@@ -34,22 +34,23 @@ MyApplication(ContainerActivator activator) {
  * com.yahoo.jdisc.application.DeactivatedContainer DeactivatedContainer} instance - an API that can be used by the
  * Application to asynchronously wait for Container termination in order to completely shut down components that are no
  * longer required. This activation pattern is used both for Application startup, runtime reconfigurations, as well as
- * for Application shutdown. It allows all jDISC Application to continously serve Requests during reconfiguration,
+ * for Application shutdown. It allows all jDISC Application to continuously serve Requests during reconfiguration,
  * causing no down time other than what the Application itself explicitly enforces.</p>
  *
 <pre>
 void reconfigureApplication() {
-   (...)
-   reconfiguredContainerBuilder.handlers().install(myRetainedClients);
-   reconfiguredContainerBuilder.servers().install(myRetainedServers);
-   myExpiredServers.close();
-   DeactivatedContainer deactivatedContainer = containerActivator.activateContainer(reconfiguredContainerBuilder);
-   deactivatedContainer.notifyTermination(new Runnable() {
-       void run() {
-           myExpiredClients.destroy();
-           myExpiredServers.destroy();
-       }
-   });
+    (...)
+    reconfiguredContainerBuilder.handlers().install(myRetainedClients);
+    reconfiguredContainerBuilder.servers().install(myRetainedServers);
+    myExpiredServers.close();
+    try (DeactivatedContainer deactivatedContainer = containerActivator.activateContainer(reconfiguredContainerBuilder)) {
+        deactivatedContainer.notifyTermination(new Runnable() {
+            void run() {
+                myExpiredClients.destroy();
+                myExpiredServers.destroy();
+            }
+        });
+    }
 }
 </pre>
  *
diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java
index e2d2da660c7..d3abfa77fca 100644
--- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java
+++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java
@@ -55,7 +55,7 @@ public class ActiveContainer extends AbstractResource implements CurrentContaine
             }
         });
         guiceInjector = builder.guiceModules().activate();
-        termination = new ContainerTermination(builder.appContext());
+        termination = new ContainerTermination(builder.appContext(), refer());
     }
 
     @Override
diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationLoader.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationLoader.java
index 0ca32197d5e..d6618d50bc3 100644
--- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationLoader.java
+++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationLoader.java
@@ -186,7 +186,7 @@ public class ApplicationLoader implements BootstrapLoader, ContainerActivator, C
         synchronized (appLock) {
             application = null;
         }
-        activateContainer(null);
+        try (DeactivatedContainer deactivated = activateContainer(null)) { }
         synchronized (appLock) {
             this.applicationInUseTracker = null;
         }
diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ContainerTermination.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ContainerTermination.java
index 4c7c26358d6..7177c337fdd 100644
--- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ContainerTermination.java
+++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ContainerTermination.java
@@ -1,6 +1,7 @@
 // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.jdisc.core;
 
+import com.yahoo.jdisc.ResourceReference;
 import com.yahoo.jdisc.application.DeactivatedContainer;
 
 /**
@@ -10,11 +11,14 @@ public class ContainerTermination implements DeactivatedContainer, Runnable {
 
     private final Object lock = new Object();
     private final Object appContext;
+    private final ResourceReference containerReference;
     private Runnable task;
     private boolean done;
+    private boolean closed;
 
-    public ContainerTermination(Object appContext) {
+    public ContainerTermination(Object appContext, ResourceReference containerReference) {
         this.appContext = appContext;
+        this.containerReference = containerReference;
     }
 
     @Override
@@ -38,6 +42,15 @@ public class ContainerTermination implements DeactivatedContainer, Runnable {
     }
 
     @Override
+    public void close() {
+        synchronized (lock) {
+            if (closed) return;
+            closed = true;
+            containerReference.close();
+        }
+    }
+
+    @Override
     public void run() {
         Runnable task;
         synchronized (lock) {
diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/test/TestDriver.java b/jdisc_core/src/main/java/com/yahoo/jdisc/test/TestDriver.java
index e3bc8aa3b42..d5b6265a1e2 100644
--- a/jdisc_core/src/main/java/com/yahoo/jdisc/test/TestDriver.java
+++ b/jdisc_core/src/main/java/com/yahoo/jdisc/test/TestDriver.java
@@ -73,9 +73,10 @@ public class TestDriver implements ContainerActivator, CurrentContainer {
         return loader.newContainerBuilder();
     }
 
+    /** Returns the deactivated container, with its container reference already released. */
     @Override
     public DeactivatedContainer activateContainer(ContainerBuilder builder) {
-        return loader.activateContainer(builder);
+        try (DeactivatedContainer deactivated = loader.activateContainer(buionf
     }
 
     @Override
diff --git a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ApplicationLoaderTestCase.java b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ApplicationLoaderTestCase.java
index 34acf3d4f93..ca5eefb58fc 100644
--- a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ApplicationLoaderTestCase.java
+++ b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ApplicationLoaderTestCase.java
@@ -22,6 +22,7 @@ import org.osgi.framework.BundleContext;
 import java.nio.ByteBuffer;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ThreadFactory;
@@ -31,6 +32,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -72,23 +74,14 @@ public class ApplicationLoaderTestCase {
     void requireThatApplicationStartExceptionUnsetsAndDestroysApplication() throws Exception {
         MyApplication app = MyApplication.newStartException();
         ApplicationLoader loader = new ApplicationLoader(new NonWorkingOsgiFramework(),
-                Arrays.asList(new MyApplicationModule(app)));
+                                                         List.of(new MyApplicationModule(app)));
         loader.init(null, false);
-        try {
-            loader.start();
-            fail();
-        } catch (MyException e) {
-
-        }
+        assertThrows(MyException.class, loader::start);
         assertNull(loader.application());
         assertFalse(app.stop.await(100, TimeUnit.MILLISECONDS));
         assertTrue(app.destroy.await(600, TimeUnit.SECONDS));
-        try {
-            loader.activateContainer(loader.newContainerBuilder());
-            fail();
-        } catch (ApplicationNotReadyException e) {
-
-        }
+        assertThrows(ApplicationNotReadyException.class,
+                     () -> loader.activateContainer(loader.newContainerBuilder()));
         loader.stop();
         loader.destroy();
     }
@@ -97,14 +90,10 @@ public class ApplicationLoaderTestCase {
     void requireThatApplicationStopExceptionDestroysApplication() throws Exception {
         MyApplication app = MyApplication.newStopException();
         ApplicationLoader loader = new ApplicationLoader(new NonWorkingOsgiFramework(),
-                Arrays.asList(new MyApplicationModule(app)));
+                                                         List.of(new MyApplicationModule(app)));
         loader.init(null, false);
         loader.start();
-        try {
-            loader.stop();
-        } catch (MyException e) {
-
-        }
+        loader.stop();
         assertTrue(app.destroy.await(600, TimeUnit.SECONDS));
         loader.destroy();
     }
diff --git a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerTerminationTestCase.java b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerTerminationTestCase.java
index f91c7ccc3b0..e24c63487d9 100644
--- a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerTerminationTestCase.java
+++ b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerTerminationTestCase.java
@@ -18,8 +18,12 @@ public class ContainerTerminationTestCase {
     @Test
     void requireThatAccessorsWork() {
         Object obj = new Object();
-        ContainerTermination termination = new ContainerTermination(obj);
+        MyTask task = new MyTask();
+        ContainerTermination termination = new ContainerTermination(obj, task::run);
         assertSame(obj, termination.appContext());
+        assertFalse(task.done);
+        termination.close();
+        assertTrue(task.done);
     }
 
     @Test
@@ -36,7 +40,7 @@ public class ContainerTerminationTestCase {
 
     @Test
     void requireThatEarlyTerminationIsNotified() {
-        ContainerTermination termination = new ContainerTermination(null);
+        ContainerTermination termination = new ContainerTermination(null, null);
         termination.run();
         MyTask task = new MyTask();
         termination.notifyTermination(task);
@@ -45,7 +49,7 @@ public class ContainerTerminationTestCase {
 
     @Test
     void requireThatLaterTerminationIsNotified() {
-        ContainerTermination termination = new ContainerTermination(null);
+        ContainerTermination termination = new ContainerTermination(null, null);
         MyTask task = new MyTask();
         termination.notifyTermination(task);
         assertFalse(task.done);
@@ -55,7 +59,7 @@ public class ContainerTerminationTestCase {
 
     @Test
     void requireThatNotifyCanOnlyBeCalledOnce() {
-        ContainerTermination termination = new ContainerTermination(null);
+        ContainerTermination termination = new ContainerTermination(null, null);
         termination.notifyTermination(new MyTask());
         try {
             termination.notifyTermination(new MyTask());
diff --git a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerWatchdogTest.java b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerWatchdogTest.java
index 2909c11a6c6..6d178567b75 100644
--- a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerWatchdogTest.java
+++ b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerWatchdogTest.java
@@ -42,6 +42,9 @@ public class ContainerWatchdogTest {
         assertEquals(1, runMonitorStepAndGetStaleContainerCount(watchdog, metric));
 
         containerWithoutRetainedResources.release();
+        assertEquals(1, runMonitorStepAndGetStaleContainerCount(watchdog, metric));
+
+        containerWithoutRetainedResources.shutdown().close();
         assertEquals(0, runMonitorStepAndGetStaleContainerCount(watchdog, metric));
     }