Wrap component construction in a future, and throw upon timeout.

- Throw ComponentConstructorException for same behaviour as when
  a component ctor throws an exception.
- Add test
- Remove unnecessarily duplicated test code.

2 files changed, 59 insertions, 19 deletions

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 eb0511f3427..5c42626614e 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
@@ -13,6 +13,11 @@ 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 scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent.duration._
+import scala.concurrent.{Await, Future, TimeoutException}
+import scala.language.postfixOps
+
 /**
  * @author tonytv
  * @author gjoranv
@@ -90,17 +95,29 @@ 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(ErrorOrComponentConstructorException(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 _: InterruptedException =>
+        Thread.currentThread().interrupt()
+        throw new RuntimeException(s"Interrupted while constructing component $idAndType")
     }
-
-    initId(instance)
   }
-  
+
   private def ErrorOrComponentConstructorException(cause: Throwable, message: String) : Throwable = {
     if (cause != null && cause.isInstanceOf[Error]) // don't convert Errors to RuntimeExceptions
       new Error(message, cause)
@@ -169,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]]]
 
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 f31973c5cea..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
@@ -3,7 +3,7 @@ package com.yahoo.container.di
 
 import com.yahoo.container.di.componentgraph.core.ComponentGraphTest.{SimpleComponent, SimpleComponent2}
 import com.yahoo.container.di.componentgraph.Provider
-import com.yahoo.container.di.componentgraph.core.{ComponentGraph, Node}
+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._
@@ -148,11 +148,12 @@ class ContainerTest {
       currentGraph = container.runOnce(currentGraph)
       fail("Expected exception")
     } catch {
-      case _:ComponentConstructorException => // Expected, do nothing
+      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))
@@ -165,6 +166,29 @@ class ContainerTest {
   }
 
   @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])
 
@@ -181,19 +205,10 @@ class ContainerTest {
       currentGraph = container.runOnce(currentGraph)
       fail("Expected exception")
     } catch {
-      case _:IllegalArgumentException => // Expected, do nothing
+      case _: IllegalArgumentException => // Expected, do nothing
       case _: Throwable => fail("Expected IllegalArgumentException")
     }
     assertEquals(1, currentGraph.generation)
-
-    val componentTakingConfigEntry = ComponentEntry("componentTakingConfig", classOf[ComponentTakingConfig])
-    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
@@ -378,6 +393,11 @@ object ContainerTest {
     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 {
     var deconstructed = false
     override def deconstruct() {