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

6 files changed, 150 insertions, 73 deletions

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:
+ * <li>
+ *   <ul>implements support for methods {@code start()}, {@code stop()} and {@code close()} as methods of startable,
+ *       stoppable and/or disposable components in a SonarQube container (whichever side the container is on)</ul>
+ *   <ul>ensures that all stoppable and disposable components in a given container are stopped and/or disposed of
+ *       even if a {@link RuntimeException} or a {@link Error} is thrown by one or more of those stoppable and/or
+ *       disposable components</ul>
+ * </li>
+ */
+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<Object> consumeAvailableMemory() {
+      List<Object> 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);
     }