Improve error handling of ComponentContainer lifecycle

2 files changed, 42 insertions, 18 deletions

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/platform/ComponentContainer.java b/sonar-plugin-api/src/main/java/org/sonar/api/platform/ComponentContainer.java
index dd4b5719cd5..593e5a30b4c 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/platform/ComponentContainer.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/platform/ComponentContainer.java
@@ -27,7 +27,6 @@ import org.picocontainer.MutablePicoContainer;
 import org.picocontainer.behaviors.OptInCaching;
 import org.picocontainer.lifecycle.ReflectionLifecycleStrategy;
 import org.picocontainer.monitors.NullComponentMonitor;
-import org.slf4j.LoggerFactory;
 import org.sonar.api.BatchComponent;
 import org.sonar.api.ServerComponent;
 import org.sonar.api.config.PropertyDefinitions;
@@ -119,14 +118,15 @@ public class ComponentContainer implements BatchComponent, ServerComponent {
   public ComponentContainer stopComponents(boolean swallowException) {
     try {
       pico.stop();
-      if (parent != null) {
-        parent.removeChild();
-      }
+
     } catch (RuntimeException e) {
       if (!swallowException) {
         throw PicoUtils.propagate(e);
       }
-      LoggerFactory.getLogger(getClass()).error("Fail to stop container", e);
+    } finally {
+      if (parent != null) {
+        parent.removeChild();
+      }
     }
     return this;
   }
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/platform/ComponentContainerTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/platform/ComponentContainerTest.java
index 6722b6e550c..6bf96d37def 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/platform/ComponentContainerTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/platform/ComponentContainerTest.java
@@ -22,7 +22,6 @@ package org.sonar.api.platform;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
-import org.picocontainer.Startable;
 import org.picocontainer.injectors.ProviderAdapter;
 import org.sonar.api.Property;
 import org.sonar.api.config.PropertyDefinitions;
@@ -30,6 +29,7 @@ import org.sonar.api.config.PropertyDefinitions;
 import java.util.Arrays;
 
 import static junit.framework.Assert.assertTrue;
+import static junit.framework.Assert.fail;
 import static org.fest.assertions.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
@@ -166,7 +166,7 @@ public class ComponentContainerTest {
   }
 
   @Test
-  public void should_fail_to_start_execution() {
+  public void should_sanitize_pico_exception_on_start_failure() {
     ComponentContainer container = new ComponentContainer();
     container.add(UnstartableComponent.class);
 
@@ -176,35 +176,59 @@ public class ComponentContainerTest {
   }
 
   @Test
-  public void stop_exception_should_not_hide_start_exception() {
+  public void test_start_failure() {
     ComponentContainer container = new ComponentContainer();
-    container.add(UnstartableComponent.class, UnstoppableComponent.class);
+    StartableComponent startable = new StartableComponent();
+    container.add(startable, UnstartableComponent.class);
 
-    thrown.expect(IllegalStateException.class);
-    thrown.expectMessage("Fail to start");
-    container.execute();
+    try {
+      container.execute();
+      fail();
+    } catch (Exception e) {
+      assertThat(startable.started).isTrue();
+
+      // container stops the components that have already been started
+      assertThat(startable.stopped).isTrue();
+    }
   }
 
   @Test
-  public void should_fail_to_stop_execution() {
+  public void test_stop_failure() {
     ComponentContainer container = new ComponentContainer();
-    container.add(UnstoppableComponent.class);
+    StartableComponent startable = new StartableComponent();
+    container.add(startable, UnstoppableComponent.class);
+
+    try {
+      container.execute();
+      fail();
+    } catch (Exception e) {
+      assertThat(startable.started).isTrue();
+
+      // 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);
 
     thrown.expect(IllegalStateException.class);
-    thrown.expectMessage("Fail to stop");
+    thrown.expectMessage("Fail to start");
     container.execute();
   }
 
   @Test
   public void should_execute_components() {
     ComponentContainer container = new ComponentContainer();
-    Startable component = mock(Startable.class);
+    StartableComponent component = new StartableComponent();
     container.add(component);
 
     container.execute();
 
-    verify(component).start();
-    verify(component).stop();
+    assertThat(component.started).isTrue();
+    assertThat(component.stopped).isTrue();
   }
 
   public static class StartableComponent {