From a6499246ce47e223255d91179fb6370f421da4d2 Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Tue, 31 Oct 2017 15:24:18 +0100 Subject: [PATCH] SONAR-9973 ComponentContainer#stopComponent now never fail and ensures all stoppable and/or disposable components in the container have their stop and/or dispose methods called --- .../engine/MigrationContainerImpl.java | 5 +- .../task/container/TaskContainerImpl.java | 3 +- .../core/platform/ComponentContainer.java | 39 ++----- .../StopSafeReflectionLifecycleStrategy.java | 60 ++++++++++ .../core/platform/ComponentContainerTest.java | 106 ++++++++++++------ .../org/sonar/batch/bootstrapper/Batch.java | 10 +- 6 files changed, 150 insertions(+), 73 deletions(-) create mode 100644 sonar-core/src/main/java/org/sonar/core/platform/StopSafeReflectionLifecycleStrategy.java diff --git a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/engine/MigrationContainerImpl.java b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/engine/MigrationContainerImpl.java index 96c2fe978cf..bce29650b9a 100644 --- a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/engine/MigrationContainerImpl.java +++ b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/engine/MigrationContainerImpl.java @@ -28,6 +28,7 @@ import org.picocontainer.lifecycle.ReflectionLifecycleStrategy; import org.picocontainer.monitors.NullComponentMonitor; import org.sonar.api.config.PropertyDefinitions; import org.sonar.core.platform.ComponentContainer; +import org.sonar.core.platform.StopSafeReflectionLifecycleStrategy; import static java.util.Objects.requireNonNull; @@ -49,7 +50,7 @@ public class MigrationContainerImpl extends ComponentContainer implements Migrat */ private static MutablePicoContainer createContainer(ComponentContainer parent) { ComponentMonitor componentMonitor = new NullComponentMonitor(); - ReflectionLifecycleStrategy lifecycleStrategy = new ReflectionLifecycleStrategy(componentMonitor, "start", "stop", "close") { + ReflectionLifecycleStrategy lifecycleStrategy = new StopSafeReflectionLifecycleStrategy(componentMonitor) { @Override public boolean isLazy(ComponentAdapter adapter) { return true; @@ -60,7 +61,7 @@ public class MigrationContainerImpl extends ComponentContainer implements Migrat @Override public void cleanup() { - stopComponents(true); + stopComponents(); } @Override diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/task/container/TaskContainerImpl.java b/server/sonar-server/src/main/java/org/sonar/server/computation/task/container/TaskContainerImpl.java index cd65f1c886d..27888a6f9be 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/task/container/TaskContainerImpl.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/task/container/TaskContainerImpl.java @@ -32,6 +32,7 @@ import org.sonar.api.utils.log.Loggers; import org.sonar.core.platform.ComponentContainer; import org.sonar.core.platform.ContainerPopulator; import org.sonar.core.platform.Module; +import org.sonar.core.platform.StopSafeReflectionLifecycleStrategy; import static java.util.Objects.requireNonNull; @@ -62,7 +63,7 @@ public class TaskContainerImpl extends ComponentContainer implements TaskContain */ private static MutablePicoContainer createContainer(ComponentContainer parent) { ComponentMonitor componentMonitor = new NullComponentMonitor(); - ReflectionLifecycleStrategy lifecycleStrategy = new ReflectionLifecycleStrategy(componentMonitor, "start", "stop", "close") { + ReflectionLifecycleStrategy lifecycleStrategy = new StopSafeReflectionLifecycleStrategy(componentMonitor) { @Override public boolean isLazy(ComponentAdapter adapter) { return adapter.getComponentImplementation().getAnnotation(EagerStart.class) == null; diff --git a/sonar-core/src/main/java/org/sonar/core/platform/ComponentContainer.java b/sonar-core/src/main/java/org/sonar/core/platform/ComponentContainer.java index f383b8f40fa..cd33521c5c9 100644 --- a/sonar-core/src/main/java/org/sonar/core/platform/ComponentContainer.java +++ b/sonar-core/src/main/java/org/sonar/core/platform/ComponentContainer.java @@ -35,14 +35,11 @@ import org.picocontainer.LifecycleStrategy; import org.picocontainer.MutablePicoContainer; import org.picocontainer.PicoContainer; import org.picocontainer.behaviors.OptInCaching; -import org.picocontainer.lifecycle.ReflectionLifecycleStrategy; import org.picocontainer.monitors.NullComponentMonitor; import org.sonar.api.batch.ScannerSide; import org.sonar.api.ce.ComputeEngineSide; import org.sonar.api.config.PropertyDefinitions; import org.sonar.api.server.ServerSide; -import org.sonar.api.utils.log.Loggers; -import org.sonar.api.utils.log.Profiler; import static com.google.common.collect.ImmutableList.copyOf; import static java.util.Objects.requireNonNull; @@ -54,12 +51,8 @@ public class ComponentContainer implements ContainerPopulator.Container { public static final int COMPONENTS_IN_EMPTY_COMPONENT_CONTAINER = 2; private static final class ExtendedDefaultPicoContainer extends DefaultPicoContainer { - private ExtendedDefaultPicoContainer(ComponentFactory componentFactory, LifecycleStrategy lifecycleStrategy, PicoContainer parent) { - super(componentFactory, lifecycleStrategy, parent); - } - - private ExtendedDefaultPicoContainer(final ComponentFactory componentFactory, final LifecycleStrategy lifecycleStrategy, final PicoContainer parent, - final ComponentMonitor componentMonitor) { + private ExtendedDefaultPicoContainer(ComponentFactory componentFactory, LifecycleStrategy lifecycleStrategy, PicoContainer parent, + ComponentMonitor componentMonitor) { super(componentFactory, lifecycleStrategy, parent, componentMonitor); } @@ -124,12 +117,10 @@ public class ComponentContainer implements ContainerPopulator.Container { } public void execute() { - boolean threw = true; try { startComponents(); - threw = false; } finally { - stopComponents(threw); + stopComponents(); } } @@ -167,21 +158,12 @@ public class ComponentContainer implements ContainerPopulator.Container { * a component twice is not authorized. */ public ComponentContainer stopComponents() { - return stopComponents(false); - } - - public ComponentContainer stopComponents(boolean swallowException) { - stopChildren(); try { + stopChildren(); if (pico.getLifecycleState().isStarted()) { pico.stop(); } pico.dispose(); - - } catch (RuntimeException e) { - if (!swallowException) { - throw PicoUtils.propagate(e); - } } finally { if (parent != null) { parent.removeChild(this); @@ -312,16 +294,8 @@ public class ComponentContainer implements ContainerPopulator.Container { } public static MutablePicoContainer createPicoContainer() { - ReflectionLifecycleStrategy lifecycleStrategy = new ReflectionLifecycleStrategy(new NullComponentMonitor(), "start", "stop", "close") { - @Override - public void start(Object component) { - Profiler profiler = Profiler.createIfTrace(Loggers.get(ComponentContainer.class)); - profiler.start(); - super.start(component); - profiler.stopTrace(component.getClass().getCanonicalName() + " started"); - } - }; - return new ExtendedDefaultPicoContainer(new OptInCaching(), lifecycleStrategy, null); + NullComponentMonitor componentMonitor = new NullComponentMonitor(); + return new ExtendedDefaultPicoContainer(new OptInCaching(), new StopSafeReflectionLifecycleStrategy(componentMonitor), null, componentMonitor); } public ComponentContainer getParent() { @@ -339,4 +313,5 @@ public class ComponentContainer implements ContainerPopulator.Container { public int size() { return pico.getComponentAdapters().size(); } + } diff --git a/sonar-core/src/main/java/org/sonar/core/platform/StopSafeReflectionLifecycleStrategy.java b/sonar-core/src/main/java/org/sonar/core/platform/StopSafeReflectionLifecycleStrategy.java new file mode 100644 index 00000000000..5abf16e3e17 --- /dev/null +++ b/sonar-core/src/main/java/org/sonar/core/platform/StopSafeReflectionLifecycleStrategy.java @@ -0,0 +1,60 @@ +/* + * SonarQube + * Copyright (C) 2009-2017 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.core.platform; + +import org.picocontainer.ComponentMonitor; +import org.picocontainer.lifecycle.ReflectionLifecycleStrategy; +import org.sonar.api.utils.log.Loggers; + +/** + * A {@link ReflectionLifecycleStrategy} which: + *
  • + * + * + *
    • + */ +public class StopSafeReflectionLifecycleStrategy extends ReflectionLifecycleStrategy { + public StopSafeReflectionLifecycleStrategy(ComponentMonitor componentMonitor) { + super(componentMonitor, "start", "stop", "close"); + } + + @Override + public void stop(Object component) { + try { + super.stop(component); + } catch (RuntimeException | Error e) { + Loggers.get(StopSafeReflectionLifecycleStrategy.class) + .warn("Stopping of component {} failed", component.getClass().getCanonicalName(), e); + } + } + + @Override + public void dispose(Object component) { + try { + super.dispose(component); + } catch (RuntimeException | Error e) { + Loggers.get(StopSafeReflectionLifecycleStrategy.class) + .warn("Dispose of component {} failed", component.getClass().getCanonicalName(), e); + } + } +} diff --git a/sonar-core/src/test/java/org/sonar/core/platform/ComponentContainerTest.java b/sonar-core/src/test/java/org/sonar/core/platform/ComponentContainerTest.java index a877d1787ac..9232399b25f 100644 --- a/sonar-core/src/test/java/org/sonar/core/platform/ComponentContainerTest.java +++ b/sonar-core/src/test/java/org/sonar/core/platform/ComponentContainerTest.java @@ -19,7 +19,9 @@ */ package org.sonar.core.platform; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -48,22 +50,22 @@ public class ComponentContainerTest { @Test public void should_start_and_stop() { ComponentContainer container = spy(new ComponentContainer()); - container.addSingleton(StartableComponent.class); + container.addSingleton(StartableStoppableComponent.class); container.startComponents(); - assertThat(container.getComponentByType(StartableComponent.class).started).isTrue(); - assertThat(container.getComponentByType(StartableComponent.class).stopped).isFalse(); + assertThat(container.getComponentByType(StartableStoppableComponent.class).started).isTrue(); + assertThat(container.getComponentByType(StartableStoppableComponent.class).stopped).isFalse(); verify(container).doBeforeStart(); verify(container).doAfterStart(); container.stopComponents(); - assertThat(container.getComponentByType(StartableComponent.class).stopped).isTrue(); + assertThat(container.getComponentByType(StartableStoppableComponent.class).stopped).isTrue(); } @Test public void should_start_and_stop_hierarchy_of_containers() { - StartableComponent parentComponent = new StartableComponent(); - final StartableComponent childComponent = new StartableComponent(); + StartableStoppableComponent parentComponent = new StartableStoppableComponent(); + final StartableStoppableComponent childComponent = new StartableStoppableComponent(); ComponentContainer parentContainer = new ComponentContainer() { @Override public void doAfterStart() { @@ -82,8 +84,8 @@ public class ComponentContainerTest { @Test public void should_stop_hierarchy_of_containers_on_failure() { - StartableComponent parentComponent = new StartableComponent(); - final StartableComponent childComponent1 = new StartableComponent(); + StartableStoppableComponent parentComponent = new StartableStoppableComponent(); + final StartableStoppableComponent childComponent1 = new StartableStoppableComponent(); final UnstartableComponent childComponent2 = new UnstartableComponent(); ComponentContainer parentContainer = new ComponentContainer() { @Override @@ -112,15 +114,15 @@ public class ComponentContainerTest { parent.startComponents(); ComponentContainer child = parent.createChild(); - child.addSingleton(StartableComponent.class); + child.addSingleton(StartableStoppableComponent.class); child.startComponents(); assertThat(child.getParent()).isSameAs(parent); assertThat(parent.getChildren()).containsOnly(child); assertThat(child.getComponentByType(ComponentContainer.class)).isSameAs(child); assertThat(parent.getComponentByType(ComponentContainer.class)).isSameAs(parent); - assertThat(child.getComponentByType(StartableComponent.class)).isNotNull(); - assertThat(parent.getComponentByType(StartableComponent.class)).isNull(); + assertThat(child.getComponentByType(StartableStoppableComponent.class)).isNotNull(); + assertThat(parent.getComponentByType(StartableStoppableComponent.class)).isNull(); parent.stopComponents(); } @@ -154,17 +156,16 @@ public class ComponentContainerTest { assertThat(parent.getChildren()).isEmpty(); } - @Test public void shouldForwardStartAndStopToDescendants() { ComponentContainer grandParent = new ComponentContainer(); ComponentContainer parent = grandParent.createChild(); ComponentContainer child = parent.createChild(); - child.addSingleton(StartableComponent.class); + child.addSingleton(StartableStoppableComponent.class); grandParent.startComponents(); - StartableComponent component = child.getComponentByType(StartableComponent.class); + StartableStoppableComponent component = child.getComponentByType(StartableStoppableComponent.class); assertTrue(component.started); parent.stopComponents(); @@ -254,7 +255,7 @@ public class ComponentContainerTest { @Test public void test_start_failure() { ComponentContainer container = new ComponentContainer(); - StartableComponent startable = new StartableComponent(); + StartableStoppableComponent startable = new StartableStoppableComponent(); container.add(startable, UnstartableComponent.class); try { @@ -269,26 +270,38 @@ public class ComponentContainerTest { } @Test - public void test_stop_failure() { + public void stop_container_does_not_fail_and_all_stoppable_components_are_stopped_even_if_one_or_more_stop_method_call_fail() { ComponentContainer container = new ComponentContainer(); - StartableComponent startable = new StartableComponent(); - container.add(startable, UnstoppableComponent.class); + container.add(FailingStopWithISEComponent.class, FailingStopWithISEComponent2.class, FailingStopWithErrorComponent.class, FailingStopWithErrorComponent2.class); + container.startComponents(); + StartableStoppableComponent[] components = { + container.getComponentByType(FailingStopWithISEComponent.class), + container.getComponentByType(FailingStopWithISEComponent2.class), + container.getComponentByType(FailingStopWithErrorComponent.class), + container.getComponentByType(FailingStopWithErrorComponent2.class) + }; - try { - container.execute(); - fail(); - } catch (Exception e) { - assertThat(startable.started).isTrue(); + container.stopComponents(); + + Arrays.stream(components).forEach(startableComponent -> assertThat(startableComponent.stopped).isTrue()); + } + + @Test + public void stop_container_stops_all_stoppable_components_even_in_case_of_OOM_in_any_stop_method() { + ComponentContainer container = new ComponentContainer(); + container.add(FailingStopWithOOMComponent.class, FailingStopWithOOMComponent2.class); + container.startComponents(); + StartableStoppableComponent[] components = { + container.getComponentByType(FailingStopWithOOMComponent.class), + container.getComponentByType(FailingStopWithOOMComponent2.class) + }; - // container should stop the components that have already been started - // ... but that's not the case - } } @Test public void stop_exception_should_not_hide_start_exception() { ComponentContainer container = new ComponentContainer(); - container.add(UnstartableComponent.class, UnstoppableComponent.class); + container.add(UnstartableComponent.class, FailingStopWithISEComponent.class); thrown.expect(IllegalStateException.class); thrown.expectMessage("Fail to start"); @@ -298,7 +311,7 @@ public class ComponentContainerTest { @Test public void should_execute_components() { ComponentContainer container = new ComponentContainer(); - StartableComponent component = new StartableComponent(); + StartableStoppableComponent component = new StartableStoppableComponent(); container.add(component); container.execute(); @@ -338,7 +351,7 @@ public class ComponentContainerTest { assertThat(component.isClosedAfterStop).isTrue(); } - public static class StartableComponent { + public static class StartableStoppableComponent { public boolean started = false; public boolean stopped = false; @@ -361,15 +374,44 @@ public class ComponentContainerTest { } } - public static class UnstoppableComponent { - public void start() { + public static class FailingStopWithISEComponent extends StartableStoppableComponent { + public void stop() { + super.stop(); + throw new IllegalStateException("Faking IllegalStateException thrown by stop method of " + getClass().getSimpleName()); } + } + + public static class FailingStopWithISEComponent2 extends FailingStopWithErrorComponent { + } + public static class FailingStopWithErrorComponent extends StartableStoppableComponent { public void stop() { - throw new IllegalStateException("Fail to stop"); + super.stop(); + throw new Error("Faking Error thrown by stop method of " + getClass().getSimpleName()); } } + public static class FailingStopWithErrorComponent2 extends FailingStopWithErrorComponent { + } + + public static class FailingStopWithOOMComponent extends StartableStoppableComponent { + public void stop() { + super.stop(); + consumeAvailableMemory(); + } + + private static List consumeAvailableMemory() { + List holder = new ArrayList<>(); + while (true) { + holder.add(new byte[128 * 1024]); + } + } + } + + public static class FailingStopWithOOMComponent2 extends FailingStopWithOOMComponent { + + } + @Property(key = "foo", defaultValue = "bar", name = "Foo") public static class ComponentWithProperty { diff --git a/sonar-scanner-engine/src/main/java/org/sonar/batch/bootstrapper/Batch.java b/sonar-scanner-engine/src/main/java/org/sonar/batch/bootstrapper/Batch.java index 9d745c53008..d324618ea43 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/batch/bootstrapper/Batch.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/batch/bootstrapper/Batch.java @@ -67,12 +67,10 @@ public final class Batch { public synchronized Batch execute() { configureLogging(); doStart(); - boolean threw = true; try { doExecuteTask(globalProperties); - threw = false; } finally { - doStop(threw); + doStop(); } return this; } @@ -150,12 +148,12 @@ public final class Batch { public synchronized void stop() { checkStarted(); configureLogging(); - doStop(false); + doStop(); } - private void doStop(boolean swallowException) { + private void doStop() { try { - bootstrapContainer.stopComponents(swallowException); + bootstrapContainer.stopComponents(); } catch (RuntimeException e) { throw handleException(e); } -- 2.39.5