From 4bc200ee0c8f3e48fa34d2e741b4691a723c0398 Mon Sep 17 00:00:00 2001 From: gjoranv Date: Fri, 19 May 2017 00:51:34 +0200 Subject: 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. --- .../di/componentgraph/core/ComponentNode.scala | 34 +++++++++++++---- .../com/yahoo/container/di/ContainerTest.scala | 44 ++++++++++++++++------ 2 files changed, 59 insertions(+), 19 deletions(-) (limited to 'container-di') 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)) @@ -164,6 +165,29 @@ class ContainerTest { 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]) @@ -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() { -- cgit v1.2.3