diff options
author | Harald Musum <musum@yahoo-inc.com> | 2017-05-22 09:11:21 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2017-05-22 09:11:21 +0200 |
commit | 88c64ad827d23a3c2cab321712d21e7479892bb8 (patch) | |
tree | c52d2f280644e53baec00d8f6058bb71e5e29a0d /container-di | |
parent | ebbe253e649f38c5fe4dd7cfa8f891f6c0c5fdf4 (diff) |
Revert "Gjoranv/component construction"
Diffstat (limited to 'container-di')
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 } |