diff options
author | gjoranv <gjoranv@gmail.com> | 2017-05-29 10:47:25 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2017-05-29 10:47:25 +0200 |
commit | b4d6964ecc53280407f6a26b61ea4db556accded (patch) | |
tree | a28bbefaa94f1c204949f39725affc5eacba2734 /container-di | |
parent | 61e613620a5934273cd069b4d46635757feba8bc (diff) |
Revert "Revert "Gjoranv/component construction""
Diffstat (limited to 'container-di')
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 } |