Run reconfigurer thread even when restartOnRedeploy is true

9 files changed, 61 insertions, 53 deletions

diff --git a/config/src/main/java/com/yahoo/config/subscription/ConfigSubscriber.java b/config/src/main/java/com/yahoo/config/subscription/ConfigSubscriber.java
index b7c45de9f5b..b0af69d2dbf 100644
--- a/config/src/main/java/com/yahoo/config/subscription/ConfigSubscriber.java
+++ b/config/src/main/java/com/yahoo/config/subscription/ConfigSubscriber.java
@@ -24,7 +24,6 @@ import com.yahoo.vespa.config.TimingValues;
  * {@link ConfigHandle} which {@link #subscribe(Class, String)} returned.
  *
  * @author vegardh
- * @since 5.1
  */
 public class ConfigSubscriber {
 
diff --git a/container-core/src/main/java/com/yahoo/container/Server.java b/container-core/src/main/java/com/yahoo/container/Server.java
index 207050a8d88..2eec2e36c47 100644
--- a/container-core/src/main/java/com/yahoo/container/Server.java
+++ b/container-core/src/main/java/com/yahoo/container/Server.java
@@ -19,7 +19,7 @@ public class Server {
     private ConfigSubscriber subscriber = new ConfigSubscriber();
 
     /** The OSGi container instance of this server */
-    private Container container=Container.get();
+    private Container container = Container.get();
 
     /** A short string which is different for all the qrserver instances on a given node. */
     private String localServerDiscriminator = "qrserver.0";
@@ -48,7 +48,6 @@ public class Server {
         return instance;
     }
 
-
     private void initRpcServer(Rpc rpcConfig) {
         if (rpcConfig.enabled()) {
             ContainerRpcAdaptor rpcAdaptor = container.getRpcAdaptor();
@@ -57,8 +56,7 @@ public class Server {
         }
     }
 
-    /** Ugly hack, see Container.resetInstance
-     **/
+    /** Ugly hack, see Container.resetInstance */
     static void resetInstance() {
         instance = new Server();
     }
diff --git a/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java b/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java
index 49cacb3a09b..41e3e43f156 100644
--- a/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java
+++ b/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java
@@ -86,7 +86,7 @@ public class HandlersConfigurerDi {
 
         container = new Container(subscriberFactory, configId, deconstructor, osgiWrapper);
         try {
-            runOnceAndEnsureRegistryHackRun(discInjector);
+            getNewConfigGraph(discInjector, false);
         } catch (InterruptedException e) {
             throw new RuntimeException("Interrupted while setting up handlers for the first time.");
         }
@@ -138,11 +138,18 @@ public class HandlersConfigurerDi {
         }
     }
 
-    public void runOnceAndEnsureRegistryHackRun(Injector discInjector) throws InterruptedException {
-        currentGraph = container.runOnce(currentGraph, createFallbackInjector(vespaContainer, discInjector));
-
-        RegistriesHack registriesHack = currentGraph.getInstance(RegistriesHack.class);
-        assert (registriesHack != null);
+    /**
+     * Wait for new config to arrive and produce the new graph
+     *
+     * @return true if this resulted in a new graph that should be applied to the currently running container
+     */
+    public boolean getNewConfigGraph(Injector discInjector, boolean restartOnRedeploy) throws InterruptedException {
+        ComponentGraph newGraph = container.getNewConfigGraph(currentGraph, createFallbackInjector(vespaContainer, discInjector), restartOnRedeploy);
+        if (newGraph == currentGraph) return false;
+        currentGraph = newGraph;
+
+        assert (currentGraph.getInstance(RegistriesHack.class) != null); // TODO: Remove, seems quite pointless?
+        return true;
     }
 
     @SuppressWarnings("deprecation")
diff --git a/container-core/src/main/java/com/yahoo/container/core/config/testutil/HandlersConfigurerTestWrapper.java b/container-core/src/main/java/com/yahoo/container/core/config/testutil/HandlersConfigurerTestWrapper.java
index 89c73e19fe3..008ca5c1d5a 100644
--- a/container-core/src/main/java/com/yahoo/container/core/config/testutil/HandlersConfigurerTestWrapper.java
+++ b/container-core/src/main/java/com/yahoo/container/core/config/testutil/HandlersConfigurerTestWrapper.java
@@ -112,7 +112,7 @@ public class HandlersConfigurerTestWrapper {
     public void reloadConfig() {
         configurer.reloadConfig(++lastGeneration);
         try {
-            configurer.runOnceAndEnsureRegistryHackRun(Guice.createInjector());
+            configurer.getNewConfigGraph(Guice.createInjector(), false);
         } catch (InterruptedException e) {
            throw new RuntimeException(e);
         }
diff --git a/container-di/src/main/scala/com/yahoo/container/di/Container.scala b/container-di/src/main/scala/com/yahoo/container/di/Container.scala
index 5ea6422ddad..0ef01e5aa3d 100644
--- a/container-di/src/main/scala/com/yahoo/container/di/Container.scala
+++ b/container-di/src/main/scala/com/yahoo/container/di/Container.scala
@@ -44,9 +44,9 @@ class Container(
   var leastGeneration = -1L
 
   @throws(classOf[InterruptedException])
-  def runOnce(
-               oldGraph: ComponentGraph = new ComponentGraph,
-               fallbackInjector: GuiceInjector = Guice.createInjector()): ComponentGraph = {
+  def getNewConfigGraph(oldGraph: ComponentGraph = new ComponentGraph,
+                        fallbackInjector: GuiceInjector = Guice.createInjector(),
+                        restartOnRedeploy: Boolean = false): ComponentGraph = {
 
     def deconstructObsoleteComponents(oldGraph: ComponentGraph, newGraph: ComponentGraph) {
       val oldComponents = new IdentityHashMap[AnyRef, AnyRef]()
@@ -56,7 +56,8 @@ class Container(
     }
 
     try {
-      val newGraph = createNewGraph(oldGraph, fallbackInjector)
+      val newGraph = getConfigAndCreateGraph(oldGraph, fallbackInjector)
+      if (restartOnRedeploy) return oldGraph // wait for restart to cause reconfig instead
       newGraph.reuseNodes(oldGraph)
       constructComponents(newGraph)
       deconstructObsoleteComponents(oldGraph, newGraph)
@@ -113,10 +114,11 @@ class Container(
     }
   }
 
-  final def createNewGraph(graph: ComponentGraph = new ComponentGraph,
-                           fallbackInjector: Injector): ComponentGraph = {
+  final def getConfigAndCreateGraph(graph: ComponentGraph = new ComponentGraph,
+                                    fallbackInjector: Injector): ComponentGraph = {
 
     val snapshot = configurer.getConfigs(graph.configKeys, leastGeneration)
+
     log.log(DEBUG,
             """createNewGraph:
               |graph.configKeys = %s
@@ -138,7 +140,7 @@ class Container(
               |previous generation: %d"""
               .format(getBootstrapGeneration, getComponentsGeneration, previousConfigGeneration).stripMargin)
           installBundles(configs)
-          createNewGraph(
+          getConfigAndCreateGraph(
             createComponentsGraph(configs, getBootstrapGeneration,fallbackInjector),
             fallbackInjector)
         case ComponentsConfigs(configs) =>
diff --git a/container-di/src/test/java/demo/ContainerTestBase.java b/container-di/src/test/java/demo/ContainerTestBase.java
index 426033ea101..d413a61759b 100644
--- a/container-di/src/test/java/demo/ContainerTestBase.java
+++ b/container-di/src/test/java/demo/ContainerTestBase.java
@@ -12,12 +12,9 @@ import com.yahoo.container.di.Osgi;
 import com.yahoo.container.di.componentgraph.core.ComponentGraph;
 import org.junit.Before;
 import org.osgi.framework.Bundle;
-import scala.collection.*;
-import scala.collection.immutable.*;
 import scala.collection.immutable.Set;
 
 import java.util.Collection;
-import java.util.List;
 
 /**
  * @author tonytv
@@ -62,7 +59,7 @@ public class ContainerTestBase extends ContainerTest {
                             throw new UnsupportedOperationException("getBundle not supported.");
                         }
                     });
-            componentGraph = container.runOnce(componentGraph, Guice.createInjector());
+            componentGraph = container.getNewConfigGraph(componentGraph, Guice.createInjector(), false);
         } catch (Exception e) {
             throw new RuntimeException(e);
         }
diff --git a/container-di/src/test/scala/com/yahoo/container/di/ContainerTest.scala b/container-di/src/test/scala/com/yahoo/container/di/ContainerTest.scala
index 8e7fbde3f5e..83645a8b35c 100644
--- a/container-di/src/test/scala/com/yahoo/container/di/ContainerTest.scala
+++ b/container-di/src/test/scala/com/yahoo/container/di/ContainerTest.scala
@@ -44,7 +44,7 @@ class ContainerTest {
 
     val container = newContainer(dirConfigSource)
 
-    val component = createComponentTakingConfig(container.runOnce())
+    val component = createComponentTakingConfig(container.getNewConfigGraph())
     assertThat(component.config.stringVal(), is("myString"))
 
     container.shutdownConfigurer()
@@ -57,7 +57,7 @@ class ContainerTest {
 
     val container = newContainer(dirConfigSource)
 
-    val componentGraph = container.runOnce()
+    val componentGraph = container.getNewConfigGraph()
     val component = createComponentTakingConfig(componentGraph)
 
     assertThat(component.config.stringVal(), is("original"))
@@ -66,7 +66,7 @@ class ContainerTest {
     dirConfigSource.writeConfig("test", """stringVal "reconfigured" """)
     container.reloadConfig(2)
 
-    val newComponentGraph = container.runOnce(componentGraph)
+    val newComponentGraph = container.getNewConfigGraph(componentGraph)
     val component2 = createComponentTakingConfig(newComponentGraph)
     assertThat(component2.config.stringVal(), is("reconfigured"))
 
@@ -80,7 +80,7 @@ class ContainerTest {
 
     val container = newContainer(dirConfigSource)
 
-    val graph = container.runOnce()
+    val graph = container.getNewConfigGraph()
     val component = createComponentTakingConfig(graph)
     assertThat(component.getId.toString, is("id1"))
 
@@ -89,7 +89,7 @@ class ContainerTest {
       ("id2", classOf[ComponentTakingConfig])))
 
     container.reloadConfig(2)
-    val newGraph = container.runOnce(graph)
+    val newGraph = container.getNewConfigGraph(graph)
 
     assertThat(ComponentGraph.getNode(newGraph, "id1"), notNullValue(classOf[Node]))
     assertThat(ComponentGraph.getNode(newGraph, "id2"), notNullValue(classOf[Node]))
@@ -107,12 +107,12 @@ class ContainerTest {
 
     val container = newContainer(dirConfigSource)
 
-    val oldGraph = container.runOnce()
+    val oldGraph = container.getNewConfigGraph()
     val componentToDestruct = oldGraph.getInstance(classOf[DestructableComponent])
 
     writeBootstrapConfigs("id2", classOf[DestructableComponent])
     container.reloadConfig(2)
-    container.runOnce(oldGraph)
+    container.getNewConfigGraph(oldGraph)
     assertTrue(componentToDestruct.deconstructed)
   }
 
@@ -123,7 +123,7 @@ class ContainerTest {
     val container = newContainer(dirConfigSource)
     var currentGraph: ComponentGraph = null
     try {
-      currentGraph = container.runOnce()
+      currentGraph = container.getNewConfigGraph()
       fail("Expected to log and die.")
     } catch {
       case _: Throwable => fail("Expected to log and die")
@@ -136,14 +136,14 @@ class ContainerTest {
 
     writeBootstrapConfigs(Array(simpleComponentEntry))
     val container = newContainer(dirConfigSource)
-    var currentGraph = container.runOnce()
+    var currentGraph = container.getNewConfigGraph()
 
     val simpleComponent = currentGraph.getInstance(classOf[SimpleComponent])
 
     writeBootstrapConfigs("thrower", classOf[ComponentThrowingExceptionInConstructor])
     container.reloadConfig(2)
     try {
-      currentGraph = container.runOnce(currentGraph)
+      currentGraph = container.getNewConfigGraph(currentGraph)
       fail("Expected exception")
     } catch {
       case _: ComponentConstructorException => // Expected, do nothing
@@ -156,7 +156,7 @@ class ContainerTest {
     dirConfigSource.writeConfig("test", """stringVal "myString" """)
     writeBootstrapConfigs(Array(simpleComponentEntry, componentTakingConfigEntry))
     container.reloadConfig(3)
-    currentGraph = container.runOnce(currentGraph)
+    currentGraph = container.getNewConfigGraph(currentGraph)
 
     assertEquals(3, currentGraph.generation)
     assertSame(simpleComponent, currentGraph.getInstance(classOf[SimpleComponent]))
@@ -169,7 +169,7 @@ class ContainerTest {
 
     writeBootstrapConfigs(Array(simpleComponentEntry))
     val container = newContainer(dirConfigSource)
-    var currentGraph = container.runOnce()
+    var currentGraph = container.getNewConfigGraph()
 
     val simpleComponent = currentGraph.getInstance(classOf[SimpleComponent])
 
@@ -177,7 +177,7 @@ class ContainerTest {
     dirConfigSource.writeConfig("test", """stringVal "myString" """)
     container.reloadConfig(2)
     try {
-      currentGraph = container.runOnce(currentGraph)
+      currentGraph = container.getNewConfigGraph(currentGraph)
       fail("Expected exception")
     } catch {
       case _: IllegalArgumentException => // Expected, do nothing
@@ -192,20 +192,20 @@ class ContainerTest {
     writeBootstrapConfigs("myId", classOf[ComponentTakingConfig])
 
     val container = newContainer(dirConfigSource)
-    var currentGraph = container.runOnce()
+    var currentGraph = container.getNewConfigGraph()
 
     writeBootstrapConfigs("thrower", classOf[ComponentThrowingExceptionForMissingConfig])
     container.reloadConfig(2)
 
     try {
-      currentGraph = container.runOnce(currentGraph)
+      currentGraph = container.getNewConfigGraph(currentGraph)
       fail("expected exception")
     } catch {
       case e: Exception =>
     }
 
     val newGraph = Future {
-      currentGraph = container.runOnce(currentGraph)
+      currentGraph = container.getNewConfigGraph(currentGraph)
       currentGraph
     }
 
@@ -230,7 +230,7 @@ class ContainerTest {
     dirConfigSource.writeConfig("jersey-injection", """inject[0]" """)
 
     val container = newContainer(dirConfigSource)
-    val componentGraph = container.runOnce()
+    val componentGraph = container.getNewConfigGraph()
 
     val restApiContext = componentGraph.getInstance(clazz)
     assertNotNull(restApiContext)
@@ -278,7 +278,7 @@ class ContainerTest {
     dirConfigSource.writeConfig("jersey-injection", injectionConfig)
 
     val container = newContainer(dirConfigSource)
-    val componentGraph = container.runOnce()
+    val componentGraph = container.getNewConfigGraph()
 
     val restApiContext = componentGraph.getInstance(restApiClass)
   }
@@ -328,12 +328,12 @@ class ContainerTest {
 
     val container = newContainer(dirConfigSource, deconstructor)
 
-    val oldGraph = container.runOnce()
+    val oldGraph = container.getNewConfigGraph()
     val destructableEntity = oldGraph.getInstance(classOf[DestructableEntity])
 
     writeBootstrapConfigs("id2", classOf[DestructableProvider])
     container.reloadConfig(2)
-    container.runOnce(oldGraph)
+    container.getNewConfigGraph(oldGraph)
 
     assertTrue(destructableEntity.deconstructed)
   }
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 1977b934253..d42afadfd9b 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
@@ -128,7 +128,7 @@ public final class ConfiguredApplication implements Application {
         configureComponents(builder.guiceModules().activate());
 
         intitializeAndActivateContainer(builder);
-        if (! qrConfig.restartOnDeploy()) startReconfigurerThread();
+        startReconfigurerThread();
         portWatcher = new Thread(this::watchPortChange);
         portWatcher.setDaemon(true);
         portWatcher.start();
@@ -199,8 +199,12 @@ public final class ConfiguredApplication implements Application {
             while ( ! Thread.interrupted()) {
                 try {
                     ContainerBuilder builder = createBuilderWithGuiceBindings();
-                    configurer.runOnceAndEnsureRegistryHackRun(builder.guiceModules().activate());
-                    intitializeAndActivateContainer(builder);
+
+                    // Block until new config:
+                    boolean gotNewConfigToApply = configurer.getNewConfigGraph(builder.guiceModules().activate(),
+                                                                               qrConfig.restartOnDeploy());
+                    if (gotNewConfigToApply)
+                        intitializeAndActivateContainer(builder);
                 } catch (ConfigInterruptedException | InterruptedException e) {
                     break;
                 } catch (Exception | LinkageError e) { // LinkageError: OSGi problems
diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/application/ContainerActivator.java b/jdisc_core/src/main/java/com/yahoo/jdisc/application/ContainerActivator.java
index 9d1b613e23c..59db453e2c4 100644
--- a/jdisc_core/src/main/java/com/yahoo/jdisc/application/ContainerActivator.java
+++ b/jdisc_core/src/main/java/com/yahoo/jdisc/application/ContainerActivator.java
@@ -13,27 +13,28 @@ import com.yahoo.jdisc.Container;
  * #newContainerBuilder()}, 2) configure the returned {@link ContainerBuilder}, and 3) pass the builder to the {@link
  * #activateContainer(ContainerBuilder)} method.</p>
  *
- * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen</a>
+ * @author Simon Thoresen
  */
 public interface ContainerActivator {
 
     /**
-     * <p>This method creates and returns a new {@link ContainerBuilder} object that has the necessary references to the
-     * application and its internal components.</p>
+     * This method creates and returns a new {@link ContainerBuilder} object that has the necessary references to the
+     * application and its internal components.
      *
      * @return The created builder.
      */
-    public ContainerBuilder newContainerBuilder();
+    ContainerBuilder newContainerBuilder();
 
     /**
-     * <p>Creates and activates a {@link Container} based on the provided {@link ContainerBuilder}. By providing a
+     * Creates and activates a {@link Container} based on the provided {@link ContainerBuilder}. By providing a
      * <em>null</em> argument, this method can be used to deactivate the current Container. The returned object can be
-     * used to schedule a cleanup task that is executed once the the deactivated Container has terminated.</p>
+     * used to schedule a cleanup task that is executed once the the deactivated Container has terminated.
      *
      * @param builder The builder to activate.
      * @return The previous container, if any.
      * @throws ApplicationNotReadyException If this method is called before {@link Application#start()} or after {@link
      *                                      Application#stop()}.
      */
-    public DeactivatedContainer activateContainer(ContainerBuilder builder);
+    DeactivatedContainer activateContainer(ContainerBuilder builder);
+
 }