Revert "Revert "Gjoranv/component construction""

5 files changed, 219 insertions, 68 deletions

diff --git a/container-di/src/main/scala/com/yahoo/container/di/ConfigRetriever.scala b/container-di/src/main/scala/com/yahoo/container/di/ConfigRetriever.scala
index a1b8167171a..efa094cac11 100644
--- a/container-di/src/main/scala/com/yahoo/container/di/ConfigRetriever.scala
+++ b/container-di/src/main/scala/com/yahoo/container/di/ConfigRetriever.scala
@@ -3,11 +3,14 @@ package com.yahoo.container.di
 
 
 import config.Subscriber
-import java.util.logging.Logger
+import java.util.logging.{Level, Logger}
+
 import com.yahoo.log.LogLevel
 import ConfigRetriever._
+
 import annotation.tailrec
 import com.yahoo.config.ConfigInstance
+
 import scala.collection.JavaConversions._
 import com.yahoo.vespa.config.ConfigKey
 
@@ -77,7 +80,14 @@ final class ConfigRetriever(bootstrapKeys: Set[ConfigKeyT],
       componentSubscriber.close()
 
       componentSubscriberKeys = keys
-      componentSubscriber = subscribe(keys)
+      try {
+        componentSubscriber = subscribe(keys)
+      } catch {
+        case e: Throwable =>
+          log.log(Level.WARNING, s"Failed setting up subscriptions for component configs: ${e.getMessage}")
+          log.log(Level.WARNING, s"Config keys: $keys")
+          throw 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 28d99f89d73..2728885e3ac 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
@@ -1,20 +1,25 @@
 // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.di
 
-import com.yahoo.container.di.ConfigRetriever.{ComponentsConfigs, BootstrapConfigs}
-import com.yahoo.container.di.componentgraph.core.{JerseyNode, ComponentGraph, ComponentNode}
-import com.yahoo.container.di.config.{RestApiContext, SubscriberFactory}
-import Container._
+import java.util.{IdentityHashMap, Random}
+import java.util.logging.{Level, Logger}
 
-import scala.collection.JavaConversions._
-import scala.math.max
+import com.google.inject.{Guice, Injector}
 import com.yahoo.config._
-import com.yahoo.vespa.config.ConfigKey
-import java.util.IdentityHashMap
-import java.util.logging.Logger
 import com.yahoo.container.bundle.BundleInstantiationSpecification
-import com.google.inject.{Injector, Guice}
+import com.yahoo.container.di.ConfigRetriever.{BootstrapConfigs, ComponentsConfigs}
+import com.yahoo.container.di.Container._
+import com.yahoo.container.di.componentgraph.core.ComponentNode.ComponentConstructorException
+import com.yahoo.container.di.componentgraph.core.{ComponentGraph, ComponentNode, JerseyNode}
+import com.yahoo.container.di.config.{RestApiContext, SubscriberFactory}
 import com.yahoo.container.{BundlesConfig, ComponentsConfig}
+import com.yahoo.protect.Process
+import com.yahoo.vespa.config.ConfigKey
+
+import scala.collection.JavaConversions._
+import scala.math.max
+import scala.concurrent.duration._
+import scala.language.postfixOps
 
 
 /**
@@ -43,27 +48,67 @@ class Container(
 
     def deconstructObsoleteComponents(oldGraph: ComponentGraph, newGraph: ComponentGraph) {
       val oldComponents = new IdentityHashMap[AnyRef, AnyRef]()
-      oldGraph.allComponentsAndProviders foreach(oldComponents.put(_, null))
-      newGraph.allComponentsAndProviders foreach(oldComponents.remove(_))
-      oldComponents.keySet foreach(componentDeconstructor.deconstruct(_))
+      oldGraph.allComponentsAndProviders foreach (oldComponents.put(_, null))
+      newGraph.allComponentsAndProviders foreach (oldComponents.remove(_))
+      oldComponents.keySet foreach (componentDeconstructor.deconstruct(_))
     }
 
     try {
-      //TODO: wrap user exceptions.
       val newGraph = createNewGraph(oldGraph, fallbackInjector)
       newGraph.reuseNodes(oldGraph)
       constructComponents(newGraph)
       deconstructObsoleteComponents(oldGraph, newGraph)
       newGraph
     } catch {
-      case e : Throwable =>
-        invalidateGeneration()
-        throw e
+      case userException: ComponentConstructorException =>
+        invalidateGeneration(oldGraph.generation, userException)
+        throw userException
+      case t: Throwable =>
+        invalidateGeneration(oldGraph.generation, t)
+        throw t
     }
   }
 
-  private def invalidateGeneration() {
-    leastGeneration = max(configurer.getComponentsGeneration, configurer.getBootstrapGeneration) + 1
+  private def invalidateGeneration(generation: Long, cause: Throwable) {
+    val maxWaitToExit = 60 seconds
+
+    def newGraphErrorMessage(generation: Long, cause: Throwable): String = {
+      val failedFirstMessage = "Failed to set up first component graph"
+      val failedNewMessage = "Failed to set up new component graph"
+      val constructMessage = "due to error when constructing one of the components"
+      val exitMessage = s"Exiting within $maxWaitToExit."
+      val retainMessage = "Retaining previous component generation."
+      generation match {
+        case 0 =>
+          cause match {
+            case _: ComponentConstructorException => s"$failedFirstMessage $constructMessage. $exitMessage"
+            case _ => s"$failedFirstMessage. $exitMessage"
+          }
+        case _ =>
+          cause match {
+            case _: ComponentConstructorException => s"$failedNewMessage $constructMessage. $retainMessage"
+            case _ => s"$failedNewMessage. $retainMessage"
+          }
+      }
+    }
+
+    def logAndDie(message: String, cause: Throwable): Unit = {
+      log.log(Level.SEVERE, message, cause)
+      try {
+        Thread.sleep((new Random(System.nanoTime).nextDouble * maxWaitToExit.toMillis).toLong)
+      } catch {
+        case _: InterruptedException => // Do nothing
+      }
+      Process.logAndDie("Exited for reason (repeated from above):", cause)
+    }
+
+    val message = newGraphErrorMessage(generation, cause)
+    generation match {
+      case 0 => logAndDie(message, cause)
+      case _ =>
+        log.log(Level.WARNING, message, cause)
+        leastGeneration = max(configurer.getComponentsGeneration, configurer.getBootstrapGeneration) + 1
+    }
   }
 
   final def createNewGraph(graph: ComponentGraph = new ComponentGraph,
@@ -101,9 +146,8 @@ class Container(
     configurer.getComponentsGeneration
   }
 
-  private def createAndConfigureComponentsGraph[T](
-                                                    componentsConfigs: Map[ConfigKeyT, ConfigInstance],
-                                                    fallbackInjector: Injector): ComponentGraph = {
+  private def createAndConfigureComponentsGraph[T](componentsConfigs: Map[ConfigKeyT, ConfigInstance],
+                                                   fallbackInjector: Injector): ComponentGraph = {
 
     val componentGraph = createComponentsGraph(componentsConfigs, getComponentsGeneration, fallbackInjector)
     componentGraph.setAvailableConfigs(componentsConfigs)
diff --git a/container-di/src/main/scala/com/yahoo/container/di/componentgraph/core/ComponentNode.scala b/container-di/src/main/scala/com/yahoo/container/di/componentgraph/core/ComponentNode.scala
index 16856930e54..477dfa40aad 100644
--- a/container-di/src/main/scala/com/yahoo/container/di/componentgraph/core/ComponentNode.scala
+++ b/container-di/src/main/scala/com/yahoo/container/di/componentgraph/core/ComponentNode.scala
@@ -1,21 +1,22 @@
 // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.di.componentgraph.core
 
-import java.lang.reflect.{Modifier, ParameterizedType, Constructor, Type, InvocationTargetException}
+import java.lang.reflect.{Constructor, InvocationTargetException, Modifier, ParameterizedType, Type}
 import java.util.logging.Logger
-import com.google.inject.Inject
 
+import com.google.inject.Inject
+import com.yahoo.component.{AbstractComponent, ComponentId}
 import com.yahoo.config.ConfigInstance
-import com.yahoo.vespa.config.ConfigKey
-import com.yahoo.component.{ComponentId, AbstractComponent}
-import com.yahoo.container.di.{ConfigKeyT, JavaAnnotation, createKey, makeClassCovariant, removeStackTrace, preserveStackTrace}
 import com.yahoo.container.di.componentgraph.Provider
+import com.yahoo.container.di.componentgraph.core.ComponentNode._
+import com.yahoo.container.di.componentgraph.core.Node.equalEdges
+import com.yahoo.container.di.{ConfigKeyT, JavaAnnotation, createKey, makeClassCovariant, preserveStackTrace, removeStackTrace}
+import com.yahoo.vespa.config.ConfigKey
 
-import Node.equalEdges
-import ComponentNode._
-import java.lang.IllegalStateException
-import scala.Some
-import scala.Array
+import scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent.duration._
+import scala.concurrent.{Await, Future, TimeoutException}
+import scala.language.postfixOps
 
 /**
  * @author tonytv
@@ -94,24 +95,36 @@ class ComponentNode(componentId: ComponentId,
       case other => other
     }
 
-    val instance =
+    val instanceFuture = Future {
+        try {
+          constructor.newInstance(actualArguments: _*)
+        } catch {
+          case e: InvocationTargetException =>
+            throw removeStackTrace(ErrorOrComponentConstructorException(cutStackTraceAtConstructor(e.getCause), s"Error constructing $idAndType"))
+        }
+    }
+
     try {
-      constructor.newInstance(actualArguments: _*)
+      val instance = Await.result(instanceFuture, ComponentConstructTimeout)
+      initId(instance)
     } catch {
-      case e: InvocationTargetException =>
-        throw removeStackTrace(constructThrowable(cutStackTraceAtConstructor(e.getCause), s"Error constructing $idAndType"))
+      case constructorException: ComponentConstructorException =>
+        throw constructorException
+      case _:TimeoutException =>
+        throw new ComponentConstructorException(s"Timed out after $ComponentConstructTimeout while constructing component $idAndType.")
+      case e: InterruptedException =>
+        Thread.currentThread().interrupt()
+        throw new RuntimeException(s"Interrupted while constructing component $idAndType", e)
     }
-
-    initId(instance)
   }
-  
-  private def constructThrowable(cause: Throwable, message: String) : Throwable = {
+
+  private def ErrorOrComponentConstructorException(cause: Throwable, message: String) : Throwable = {
     if (cause != null && cause.isInstanceOf[Error]) // don't convert Errors to RuntimeExceptions
       new Error(message, cause)
     else
-      new RuntimeException(message, cause)
+      new ComponentConstructorException(message, cause)
   }
-  
+
   private def initId(component: AnyRef) = {
     def checkAndSetId(c: AbstractComponent) {
       if (c.hasInitializedId && c.getId != componentId )
@@ -173,6 +186,9 @@ class ComponentNode(componentId: ComponentId,
 object ComponentNode {
   val log = Logger.getLogger(classOf[ComponentNode].getName)
 
+  // XXX: var for testing only. Do not reset in production code!
+  var ComponentConstructTimeout: FiniteDuration = 60 seconds
+
   private def bestConstructor(clazz: Class[AnyRef]) = {
     val publicConstructors = clazz.getConstructors.asInstanceOf[Array[Constructor[AnyRef]]]
 
@@ -180,7 +196,7 @@ object ComponentNode {
       publicConstructors filter {_.getAnnotation(classOf[Inject]) != null} match {
         case Array() => None
         case Array(single) => Some(single)
-        case _ => throwRuntimeExceptionRemoveStackTrace("Multiple constructors annotated with inject in class " + clazz.getName)
+        case _ => throwComponentConstructorException("Multiple constructors annotated with inject in class " + clazz.getName)
       }
     }
 
@@ -188,7 +204,7 @@ object ComponentNode {
       def isConfigInstance(clazz: Class[_]) = classOf[ConfigInstance].isAssignableFrom(clazz)
 
       publicConstructors match {
-        case Array() => throwRuntimeExceptionRemoveStackTrace("No public constructors in class " + clazz.getName)
+        case Array() => throwComponentConstructorException("No public constructors in class " + clazz.getName)
         case Array(single) => single
         case _ =>
           log.warning("Multiple public constructors found in class %s, there should only be one. ".format(clazz.getName) +
@@ -202,9 +218,12 @@ object ComponentNode {
     constructorAnnotatedWithInject getOrElse constructorWithMostConfigParameters
   }
 
-  private def throwRuntimeExceptionRemoveStackTrace(message: String) =
-    throw removeStackTrace(new RuntimeException(message))
+  private def throwComponentConstructorException(message: String) =
+    throw removeStackTrace(new ComponentConstructorException(message))
 
+  class ComponentConstructorException(message: String, cause: Throwable) extends RuntimeException(message, cause) {
+    def this(message: String) = this(message, null)
+  }
 
   def isAbstract(clazz: Class[_ <: AnyRef]) = Modifier.isAbstract(clazz.getModifiers)
 }
diff --git a/container-di/src/main/scala/com/yahoo/container/di/componentgraph/core/Node.scala b/container-di/src/main/scala/com/yahoo/container/di/componentgraph/core/Node.scala
index 9d30552b0aa..2ec3b77ed8a 100644
--- a/container-di/src/main/scala/com/yahoo/container/di/componentgraph/core/Node.scala
+++ b/container-di/src/main/scala/com/yahoo/container/di/componentgraph/core/Node.scala
@@ -72,7 +72,7 @@ abstract class Node(val componentId: ComponentId) {
     val className = instanceType.getName
 
     if (className == componentId.getName) s"'$componentId'"
-    else s"'$componentId of type '$className'"
+    else s"'$componentId' of type '$className'"
   }
 
 }
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 b64de80e39f..b7c3f94410b 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
@@ -1,30 +1,35 @@
 // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.di
 
-import com.yahoo.container.di.componentgraph.core.ComponentGraphTest.{SimpleComponent2, SimpleComponent}
+import com.yahoo.container.di.componentgraph.core.ComponentGraphTest.{SimpleComponent, SimpleComponent2}
 import com.yahoo.container.di.componentgraph.Provider
-import com.yahoo.container.di.componentgraph.core.{Node, ComponentGraph}
-import org.junit.{Test, Before, After}
+import com.yahoo.container.di.componentgraph.core.{ComponentGraph, ComponentNode, Node}
+import org.junit.{After, Before, Ignore, Test}
 import org.junit.Assert._
 import org.hamcrest.CoreMatchers._
 import com.yahoo.config.test.TestConfig
 import com.yahoo.component.AbstractComponent
 import ContainerTest._
+
 import scala.collection.JavaConversions
 import com.yahoo.config.di.IntConfig
-import scala.concurrent.{future, Await}
+
+import scala.concurrent.{Await, Future, future}
 import scala.concurrent.duration._
 import scala.concurrent.ExecutionContext.Implicits.global
 import scala.util.Try
 import com.yahoo.container.di.config.RestApiContext
 import com.yahoo.container.bundle.MockBundle
+import com.yahoo.container.di.componentgraph.core.ComponentNode.ComponentConstructorException
+
+import scala.language.postfixOps
 
 /**
  * @author tonytv
  * @author gjoranv
  */
 class ContainerTest {
-  var dirConfigSource: DirConfigSource = null
+  var dirConfigSource: DirConfigSource = _
 
   @Before def setup()  {
     dirConfigSource = new DirConfigSource("ContainerTest-")
@@ -88,8 +93,8 @@ class ContainerTest {
     container.reloadConfig(2)
     val newGraph = container.runOnce(graph)
 
-    assertThat(ComponentGraph.getNode(newGraph, "id1"), notNullValue(classOf[Node]));
-    assertThat(ComponentGraph.getNode(newGraph, "id2"), notNullValue(classOf[Node]));
+    assertThat(ComponentGraph.getNode(newGraph, "id1"), notNullValue(classOf[Node]))
+    assertThat(ComponentGraph.getNode(newGraph, "id2"), notNullValue(classOf[Node]))
 
     container.shutdownConfigurer()
   }
@@ -113,8 +118,22 @@ class ContainerTest {
     assertTrue(componentToDestruct.deconstructed)
   }
 
+  @Ignore  // because logAndDie is impossible(?) to verify programmatically
   @Test
-  def previous_graph_is_retained_when_new_graph_throws_exception() {
+  def manually_verify_what_happens_when_first_graph_contains_component_that_throws_exception_in_ctor() {
+    writeBootstrapConfigs("thrower", classOf[ComponentThrowingExceptionInConstructor])
+    val container = newContainer(dirConfigSource)
+    var currentGraph: ComponentGraph = null
+    try {
+      currentGraph = container.runOnce()
+      fail("Expected to log and die.")
+    } catch {
+      case _: Throwable => fail("Expected to log and die")
+    }
+  }
+
+  @Test
+  def previous_graph_is_retained_when_new_graph_contains_component_that_throws_exception_in_ctor() {
     val simpleComponentEntry = ComponentEntry("simpleComponent", classOf[SimpleComponent])
 
     writeBootstrapConfigs(Array(simpleComponentEntry))
@@ -123,26 +142,76 @@ class ContainerTest {
 
     val simpleComponent = currentGraph.getInstance(classOf[SimpleComponent])
 
-    writeBootstrapConfigs("thrower", classOf[ComponentThrowingException])
-    dirConfigSource.writeConfig("test", """stringVal "myString" """)
+    writeBootstrapConfigs("thrower", classOf[ComponentThrowingExceptionInConstructor])
     container.reloadConfig(2)
     try {
       currentGraph = container.runOnce(currentGraph)
       fail("Expected exception")
     } catch {
-      case e: Exception => e.printStackTrace()
+      case _: ComponentConstructorException => // Expected, do nothing
+      case _: Throwable => fail("Expected ComponentConstructorException")
     }
+    assertEquals(1, currentGraph.generation)
 
+    // Also verify that next reconfig is successful
     val componentTakingConfigEntry = ComponentEntry("componentTakingConfig", classOf[ComponentTakingConfig])
+    dirConfigSource.writeConfig("test", """stringVal "myString" """)
     writeBootstrapConfigs(Array(simpleComponentEntry, componentTakingConfigEntry))
     container.reloadConfig(3)
     currentGraph = container.runOnce(currentGraph)
 
+    assertEquals(3, currentGraph.generation)
     assertSame(simpleComponent, currentGraph.getInstance(classOf[SimpleComponent]))
     assertNotNull(currentGraph.getInstance(classOf[ComponentTakingConfig]))
   }
 
   @Test
+  def previous_graph_is_retained_when_new_graph_contains_component_that_times_out_in_ctor() {
+    ComponentNode.ComponentConstructTimeout = 1 second
+    val simpleComponentEntry = ComponentEntry("simpleComponent", classOf[SimpleComponent])
+
+    writeBootstrapConfigs(Array(simpleComponentEntry))
+    val container = newContainer(dirConfigSource)
+    var currentGraph = container.runOnce()
+
+    val simpleComponent = currentGraph.getInstance(classOf[SimpleComponent])
+
+    writeBootstrapConfigs("sleeper", classOf[ComponentThatSleepsInConstructor])
+    container.reloadConfig(2)
+    try {
+      currentGraph = container.runOnce(currentGraph)
+      fail("Expected exception")
+    } catch {
+      case e: ComponentConstructorException => assertThat(e.getMessage, containsString("Timed out"))
+      case _: Throwable => fail("Expected ComponentConstructorException")
+    }
+    assertEquals(1, currentGraph.generation)
+  }
+
+  @Test
+  def previous_graph_is_retained_when_new_graph_throws_exception_for_missing_config() {
+    val simpleComponentEntry = ComponentEntry("simpleComponent", classOf[SimpleComponent])
+
+    writeBootstrapConfigs(Array(simpleComponentEntry))
+    val container = newContainer(dirConfigSource)
+    var currentGraph = container.runOnce()
+
+    val simpleComponent = currentGraph.getInstance(classOf[SimpleComponent])
+
+    writeBootstrapConfigs("thrower", classOf[ComponentThrowingExceptionForMissingConfig])
+    dirConfigSource.writeConfig("test", """stringVal "myString" """)
+    container.reloadConfig(2)
+    try {
+      currentGraph = container.runOnce(currentGraph)
+      fail("Expected exception")
+    } catch {
+      case _: IllegalArgumentException => // Expected, do nothing
+      case _: Throwable => fail("Expected IllegalArgumentException")
+    }
+    assertEquals(1, currentGraph.generation)
+  }
+
+  @Test
   def runOnce_hangs_waiting_for_valid_config_after_invalid_config() {
     dirConfigSource.writeConfig("test", """stringVal "original" """)
     writeBootstrapConfigs("myId", classOf[ComponentTakingConfig])
@@ -150,7 +219,7 @@ class ContainerTest {
     val container = newContainer(dirConfigSource)
     var currentGraph = container.runOnce()
 
-    writeBootstrapConfigs("thrower", classOf[ComponentThrowingException])
+    writeBootstrapConfigs("thrower", classOf[ComponentThrowingExceptionForMissingConfig])
     container.reloadConfig(2)
 
     try {
@@ -160,7 +229,7 @@ class ContainerTest {
       case e: Exception =>
     }
 
-    val newGraph = future {
+    val newGraph = Future {
       currentGraph = container.runOnce(currentGraph)
       currentGraph
     }
@@ -216,7 +285,7 @@ class ContainerTest {
     val anotherComponentClass = classOf[SimpleComponent2]
     val anotherComponentId = "anotherComponent"
 
-    val componentsConfig =
+    val componentsConfig: String =
       ComponentEntry(injectedComponentId, injectedClass).asConfig(0) + "\n" +
         ComponentEntry(anotherComponentId, anotherComponentClass).asConfig(1) + "\n" +
         ComponentEntry("restApiContext", restApiClass).asConfig(2) + "\n" +
@@ -240,7 +309,7 @@ class ContainerTest {
   }
 
   case class ComponentEntry(componentId: String,  classId: Class[_]) {
-    def asConfig(position: Int) = {
+    def asConfig(position: Int): String = {
       <config>
       |components[{position}].id "{componentId}"
       |components[{position}].classId "{classId.getName}"
@@ -307,7 +376,7 @@ object ContainerTest {
     def get() = instance
 
     def deconstruct() {
-      require(instance.deconstructed == false)
+      require(! instance.deconstructed)
       instance.deconstructed = true
     }
   }
@@ -316,8 +385,17 @@ object ContainerTest {
     require(config != null)
   }
 
-  class ComponentThrowingException(config:IntConfig) extends AbstractComponent {
-    throw new RuntimeException("This component can never be created")
+  class ComponentThrowingExceptionInConstructor() {
+    throw new RuntimeException("This component fails upon construction.")
+  }
+
+  class ComponentThrowingExceptionForMissingConfig(intConfig: IntConfig) extends AbstractComponent {
+    fail("This component should never be created. Only used for tests where 'int' config is missing.")
+  }
+
+  class ComponentThatSleepsInConstructor() {
+    Thread.sleep(3 * ComponentNode.ComponentConstructTimeout.toMillis)
+    fail("The timeout mechanism for constructing components is broken.")
   }
 
   class DestructableComponent extends AbstractComponent {
@@ -346,5 +424,5 @@ object ContainerTest {
     componentGraph.getInstance(classOf[ComponentTakingConfig])
   }
 
-  def convertMap[K, V](map: java.util.Map[K, V]) = JavaConversions.mapAsScalaMap(map).toMap
+  def convertMap[K, V](map: java.util.Map[K, V]): Map[K, V] = JavaConversions.mapAsScalaMap(map).toMap
 }