Revert "Gjoranv/component construction"

5 files changed, 68 insertions, 219 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 efa094cac11..a1b8167171a 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,14 +3,11 @@ package com.yahoo.container.di
 
 
 import config.Subscriber
-import java.util.logging.{Level, Logger}
-
+import java.util.logging.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
 
@@ -80,14 +77,7 @@ final class ConfigRetriever(bootstrapKeys: Set[ConfigKeyT],
       componentSubscriber.close()
 
       componentSubscriberKeys = 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
-      }
+      componentSubscriber = subscribe(keys)
     }
   }
 
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 2728885e3ac..28d99f89d73 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,25 +1,20 @@
 // 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 java.util.{IdentityHashMap, Random}
-import java.util.logging.{Level, Logger}
-
-import com.google.inject.{Guice, Injector}
-import com.yahoo.config._
-import com.yahoo.container.bundle.BundleInstantiationSpecification
-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.ConfigRetriever.{ComponentsConfigs, BootstrapConfigs}
+import com.yahoo.container.di.componentgraph.core.{JerseyNode, ComponentGraph, ComponentNode}
 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 Container._
 
 import scala.collection.JavaConversions._
 import scala.math.max
-import scala.concurrent.duration._
-import scala.language.postfixOps
+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.{BundlesConfig, ComponentsConfig}
 
 
 /**
@@ -48,67 +43,27 @@ 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 userException: ComponentConstructorException =>
-        invalidateGeneration(oldGraph.generation, userException)
-        throw userException
-      case t: Throwable =>
-        invalidateGeneration(oldGraph.generation, t)
-        throw t
+      case e : Throwable =>
+        invalidateGeneration()
+        throw e
     }
   }
 
-  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
-    }
+  private def invalidateGeneration() {
+    leastGeneration = max(configurer.getComponentsGeneration, configurer.getBootstrapGeneration) + 1
   }
 
   final def createNewGraph(graph: ComponentGraph = new ComponentGraph,
@@ -146,8 +101,9 @@ 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 477dfa40aad..16856930e54 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,22 +1,21 @@
 // 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.{Constructor, InvocationTargetException, Modifier, ParameterizedType, Type}
+import java.lang.reflect.{Modifier, ParameterizedType, Constructor, Type, InvocationTargetException}
 import java.util.logging.Logger
-
 import com.google.inject.Inject
-import com.yahoo.component.{AbstractComponent, ComponentId}
+
 import com.yahoo.config.ConfigInstance
-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 com.yahoo.component.{ComponentId, AbstractComponent}
+import com.yahoo.container.di.{ConfigKeyT, JavaAnnotation, createKey, makeClassCovariant, removeStackTrace, preserveStackTrace}
+import com.yahoo.container.di.componentgraph.Provider
 
-import scala.concurrent.ExecutionContext.Implicits.global
-import scala.concurrent.duration._
-import scala.concurrent.{Await, Future, TimeoutException}
-import scala.language.postfixOps
+import Node.equalEdges
+import ComponentNode._
+import java.lang.IllegalStateException
+import scala.Some
+import scala.Array
 
 /**
  * @author tonytv
@@ -95,36 +94,24 @@ class ComponentNode(componentId: ComponentId,
       case other => other
     }
 
-    val instanceFuture = Future {
-        try {
-          constructor.newInstance(actualArguments: _*)
-        } catch {
-          case e: InvocationTargetException =>
-            throw removeStackTrace(ErrorOrComponentConstructorException(cutStackTraceAtConstructor(e.getCause), s"Error constructing $idAndType"))
-        }
-    }
-
+    val instance =
     try {
-      val instance = Await.result(instanceFuture, ComponentConstructTimeout)
-      initId(instance)
+      constructor.newInstance(actualArguments: _*)
     } catch {
-      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)
+      case e: InvocationTargetException =>
+        throw removeStackTrace(constructThrowable(cutStackTraceAtConstructor(e.getCause), s"Error constructing $idAndType"))
     }
-  }
 
-  private def ErrorOrComponentConstructorException(cause: Throwable, message: String) : Throwable = {
+    initId(instance)
+  }
+  
+  private def constructThrowable(cause: Throwable, message: String) : Throwable = {
     if (cause != null && cause.isInstanceOf[Error]) // don't convert Errors to RuntimeExceptions
       new Error(message, cause)
     else
-      new ComponentConstructorException(message, cause)
+      new RuntimeException(message, cause)
   }
-
+  
   private def initId(component: AnyRef) = {
     def checkAndSetId(c: AbstractComponent) {
       if (c.hasInitializedId && c.getId != componentId )
@@ -186,9 +173,6 @@ 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]]]
 
@@ -196,7 +180,7 @@ object ComponentNode {
       publicConstructors filter {_.getAnnotation(classOf[Inject]) != null} match {
         case Array() => None
         case Array(single) => Some(single)
-        case _ => throwComponentConstructorException("Multiple constructors annotated with inject in class " + clazz.getName)
+        case _ => throwRuntimeExceptionRemoveStackTrace("Multiple constructors annotated with inject in class " + clazz.getName)
       }
     }
 
@@ -204,7 +188,7 @@ object ComponentNode {
       def isConfigInstance(clazz: Class[_]) = classOf[ConfigInstance].isAssignableFrom(clazz)
 
       publicConstructors match {
-        case Array() => throwComponentConstructorException("No public constructors in class " + clazz.getName)
+        case Array() => throwRuntimeExceptionRemoveStackTrace("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) +
@@ -218,12 +202,9 @@ object ComponentNode {
     constructorAnnotatedWithInject getOrElse constructorWithMostConfigParameters
   }
 
-  private def throwComponentConstructorException(message: String) =
-    throw removeStackTrace(new ComponentConstructorException(message))
+  private def throwRuntimeExceptionRemoveStackTrace(message: String) =
+    throw removeStackTrace(new RuntimeException(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 2ec3b77ed8a..9d30552b0aa 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 b7c3f94410b..b64de80e39f 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,35 +1,30 @@
 // 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.{SimpleComponent, SimpleComponent2}
+import com.yahoo.container.di.componentgraph.core.ComponentGraphTest.{SimpleComponent2, SimpleComponent}
 import com.yahoo.container.di.componentgraph.Provider
-import com.yahoo.container.di.componentgraph.core.{ComponentGraph, ComponentNode, Node}
-import org.junit.{After, Before, Ignore, Test}
+import com.yahoo.container.di.componentgraph.core.{Node, ComponentGraph}
+import org.junit.{Test, Before, After}
 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.{Await, Future, future}
+import scala.concurrent.{future, Await}
 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 = _
+  var dirConfigSource: DirConfigSource = null
 
   @Before def setup()  {
     dirConfigSource = new DirConfigSource("ContainerTest-")
@@ -93,8 +88,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()
   }
@@ -118,22 +113,8 @@ class ContainerTest {
     assertTrue(componentToDestruct.deconstructed)
   }
 
-  @Ignore  // because logAndDie is impossible(?) to verify programmatically
   @Test
-  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() {
+  def previous_graph_is_retained_when_new_graph_throws_exception() {
     val simpleComponentEntry = ComponentEntry("simpleComponent", classOf[SimpleComponent])
 
     writeBootstrapConfigs(Array(simpleComponentEntry))
@@ -142,76 +123,26 @@ class ContainerTest {
 
     val simpleComponent = currentGraph.getInstance(classOf[SimpleComponent])
 
-    writeBootstrapConfigs("thrower", classOf[ComponentThrowingExceptionInConstructor])
+    writeBootstrapConfigs("thrower", classOf[ComponentThrowingException])
+    dirConfigSource.writeConfig("test", """stringVal "myString" """)
     container.reloadConfig(2)
     try {
       currentGraph = container.runOnce(currentGraph)
       fail("Expected exception")
     } catch {
-      case _: ComponentConstructorException => // Expected, do nothing
-      case _: Throwable => fail("Expected ComponentConstructorException")
+      case e: Exception => e.printStackTrace()
     }
-    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])
@@ -219,7 +150,7 @@ class ContainerTest {
     val container = newContainer(dirConfigSource)
     var currentGraph = container.runOnce()
 
-    writeBootstrapConfigs("thrower", classOf[ComponentThrowingExceptionForMissingConfig])
+    writeBootstrapConfigs("thrower", classOf[ComponentThrowingException])
     container.reloadConfig(2)
 
     try {
@@ -229,7 +160,7 @@ class ContainerTest {
       case e: Exception =>
     }
 
-    val newGraph = Future {
+    val newGraph = future {
       currentGraph = container.runOnce(currentGraph)
       currentGraph
     }
@@ -285,7 +216,7 @@ class ContainerTest {
     val anotherComponentClass = classOf[SimpleComponent2]
     val anotherComponentId = "anotherComponent"
 
-    val componentsConfig: String =
+    val componentsConfig =
       ComponentEntry(injectedComponentId, injectedClass).asConfig(0) + "\n" +
         ComponentEntry(anotherComponentId, anotherComponentClass).asConfig(1) + "\n" +
         ComponentEntry("restApiContext", restApiClass).asConfig(2) + "\n" +
@@ -309,7 +240,7 @@ class ContainerTest {
   }
 
   case class ComponentEntry(componentId: String,  classId: Class[_]) {
-    def asConfig(position: Int): String = {
+    def asConfig(position: Int) = {
       <config>
       |components[{position}].id "{componentId}"
       |components[{position}].classId "{classId.getName}"
@@ -376,7 +307,7 @@ object ContainerTest {
     def get() = instance
 
     def deconstruct() {
-      require(! instance.deconstructed)
+      require(instance.deconstructed == false)
       instance.deconstructed = true
     }
   }
@@ -385,17 +316,8 @@ object ContainerTest {
     require(config != null)
   }
 
-  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 ComponentThrowingException(config:IntConfig) extends AbstractComponent {
+    throw new RuntimeException("This component can never be created")
   }
 
   class DestructableComponent extends AbstractComponent {
@@ -424,5 +346,5 @@ object ContainerTest {
     componentGraph.getInstance(classOf[ComponentTakingConfig])
   }
 
-  def convertMap[K, V](map: java.util.Map[K, V]): Map[K, V] = JavaConversions.mapAsScalaMap(map).toMap
+  def convertMap[K, V](map: java.util.Map[K, V]) = JavaConversions.mapAsScalaMap(map).toMap
 }