From 49451f5a6eb77a234072584192eaa576c78e3390 Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Mon, 20 May 2019 12:17:54 +0200 Subject: [PATCH] SONAR-10034 drop support of reflection-based Pico Container lifecycle management one must now implement org.picocontainer.Startable, org.sonar.api.Startable, java.io.Closeable and/or java.lang.Closeable --- .../ce/task/container/TaskContainerImpl.java | 10 +- .../java/org/sonar/ce/httpd/CeHttpServer.java | 7 +- .../engine/MigrationContainerImpl.java | 10 +- .../server/util/StoppableExecutorService.java | 11 +- .../platform/web/RegisterServletFilters.java | 12 +- .../RegisterQualityProfiles.java | 9 +- .../startup/ClusterConfigurationCheck.java | 1 + .../sonar/server/startup/RegisterMetrics.java | 9 +- .../startup/RegisterPermissionTemplates.java | 9 +- .../startup/RenameDeprecatedPropertyKeys.java | 11 +- .../core/platform/ComponentContainer.java | 3 +- ...tartableCloseableSafeLifecyleStrategy.java | 105 +++++++++ .../StopSafeReflectionLifecycleStrategy.java | 60 ----- .../core/platform/ComponentContainerTest.java | 149 +++++++++++- .../sonar/core/platform/PicoUtilsTest.java | 51 ++-- ...ableCloseableSafeLifecyleStrategyTest.java | 218 ++++++++++++++++++ .../api/utils/internal/TempFolderCleaner.java | 5 +- 17 files changed, 554 insertions(+), 126 deletions(-) create mode 100644 sonar-core/src/main/java/org/sonar/core/platform/StartableCloseableSafeLifecyleStrategy.java delete mode 100644 sonar-core/src/main/java/org/sonar/core/platform/StopSafeReflectionLifecycleStrategy.java create mode 100644 sonar-core/src/test/java/org/sonar/core/platform/StartableCloseableSafeLifecyleStrategyTest.java diff --git a/server/sonar-ce-task/src/main/java/org/sonar/ce/task/container/TaskContainerImpl.java b/server/sonar-ce-task/src/main/java/org/sonar/ce/task/container/TaskContainerImpl.java index 52b78b22a06..9dc7160622f 100644 --- a/server/sonar-ce-task/src/main/java/org/sonar/ce/task/container/TaskContainerImpl.java +++ b/server/sonar-ce-task/src/main/java/org/sonar/ce/task/container/TaskContainerImpl.java @@ -21,18 +21,17 @@ package org.sonar.ce.task.container; import java.util.List; import org.picocontainer.ComponentAdapter; -import org.picocontainer.ComponentMonitor; import org.picocontainer.DefaultPicoContainer; +import org.picocontainer.LifecycleStrategy; import org.picocontainer.MutablePicoContainer; import org.picocontainer.behaviors.OptInCaching; -import org.picocontainer.lifecycle.ReflectionLifecycleStrategy; import org.picocontainer.monitors.NullComponentMonitor; import org.sonar.api.config.PropertyDefinitions; 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 org.sonar.core.platform.StartableCloseableSafeLifecyleStrategy; import static java.util.Objects.requireNonNull; @@ -61,15 +60,14 @@ public class TaskContainerImpl extends ComponentContainer implements TaskContain * and lazily starts its components. */ private static MutablePicoContainer createContainer(ComponentContainer parent) { - ComponentMonitor componentMonitor = new NullComponentMonitor(); - ReflectionLifecycleStrategy lifecycleStrategy = new StopSafeReflectionLifecycleStrategy(componentMonitor) { + LifecycleStrategy lifecycleStrategy = new StartableCloseableSafeLifecyleStrategy() { @Override public boolean isLazy(ComponentAdapter adapter) { return adapter.getComponentImplementation().getAnnotation(EagerStart.class) == null; } }; - return new DefaultPicoContainer(new OptInCaching(), lifecycleStrategy, parent.getPicoContainer(), componentMonitor); + return new DefaultPicoContainer(new OptInCaching(), lifecycleStrategy, parent.getPicoContainer(), new NullComponentMonitor()); } @Override diff --git a/server/sonar-ce/src/main/java/org/sonar/ce/httpd/CeHttpServer.java b/server/sonar-ce/src/main/java/org/sonar/ce/httpd/CeHttpServer.java index fc368c72668..faa06886406 100644 --- a/server/sonar-ce/src/main/java/org/sonar/ce/httpd/CeHttpServer.java +++ b/server/sonar-ce/src/main/java/org/sonar/ce/httpd/CeHttpServer.java @@ -29,6 +29,7 @@ import java.util.Locale; import java.util.Map; import java.util.Optional; import java.util.Properties; +import org.picocontainer.Startable; import org.slf4j.LoggerFactory; import org.sonar.process.sharedmemoryfile.DefaultProcessCommands; @@ -46,7 +47,7 @@ import static org.sonar.process.ProcessEntryPoint.PROPERTY_SHARED_PATH; * This HTTP server exports data required for display of System Info page (and the related web service). * It listens on loopback address only, so it does not need to be secure (no HTTPS, no authentication). */ -public class CeHttpServer { +public class CeHttpServer implements Startable { private final Properties processProps; private final List actions; @@ -60,7 +61,7 @@ public class CeHttpServer { this.nanoHttpd = new CeNanoHttpd(InetAddress.getLoopbackAddress().getHostAddress(), 0, actionRegistry); } - // do not rename. This naming convention is required for picocontainer. + @Override public void start() { try { registerActions(); @@ -85,7 +86,7 @@ public class CeHttpServer { } } - // do not rename. This naming convention is required for picocontainer. + @Override public void stop() { nanoHttpd.stop(); } 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 bcbb9ce67dd..bc09347827f 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 @@ -20,15 +20,14 @@ package org.sonar.server.platform.db.migration.engine; import org.picocontainer.ComponentAdapter; -import org.picocontainer.ComponentMonitor; import org.picocontainer.DefaultPicoContainer; +import org.picocontainer.LifecycleStrategy; import org.picocontainer.MutablePicoContainer; import org.picocontainer.behaviors.OptInCaching; -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 org.sonar.core.platform.StartableCloseableSafeLifecyleStrategy; import static java.util.Objects.requireNonNull; @@ -49,14 +48,13 @@ public class MigrationContainerImpl extends ComponentContainer implements Migrat * Creates a PicContainer which extends the specified ComponentContainer but is not referenced in return. */ private static MutablePicoContainer createContainer(ComponentContainer parent) { - ComponentMonitor componentMonitor = new NullComponentMonitor(); - ReflectionLifecycleStrategy lifecycleStrategy = new StopSafeReflectionLifecycleStrategy(componentMonitor) { + LifecycleStrategy lifecycleStrategy = new StartableCloseableSafeLifecyleStrategy() { @Override public boolean isLazy(ComponentAdapter adapter) { return true; } }; - return new DefaultPicoContainer(new OptInCaching(), lifecycleStrategy, parent.getPicoContainer(), componentMonitor); + return new DefaultPicoContainer(new OptInCaching(), lifecycleStrategy, parent.getPicoContainer(), new NullComponentMonitor()); } @Override diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/util/StoppableExecutorService.java b/server/sonar-server-common/src/main/java/org/sonar/server/util/StoppableExecutorService.java index eee20746290..fc1315a2d26 100644 --- a/server/sonar-server-common/src/main/java/org/sonar/server/util/StoppableExecutorService.java +++ b/server/sonar-server-common/src/main/java/org/sonar/server/util/StoppableExecutorService.java @@ -20,15 +20,16 @@ package org.sonar.server.util; import java.util.concurrent.ExecutorService; +import org.picocontainer.Startable; /** * ExecutorService that exposes a {@code stop} method which can be invoked by Pico container to shutdown properly * the service. */ -public interface StoppableExecutorService extends ExecutorService { +public interface StoppableExecutorService extends ExecutorService, Startable { + @Override + default void start() { + // nothing to do + } - /** - * Stops the ExecutorService nicely (ie. first let a little time for jobs to end and then abort them) - */ - void stop(); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/platform/web/RegisterServletFilters.java b/server/sonar-server/src/main/java/org/sonar/server/platform/web/RegisterServletFilters.java index a9fe6290c08..88b1692362e 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/platform/web/RegisterServletFilters.java +++ b/server/sonar-server/src/main/java/org/sonar/server/platform/web/RegisterServletFilters.java @@ -20,13 +20,13 @@ package org.sonar.server.platform.web; import java.util.Arrays; -import javax.servlet.ServletException; +import org.picocontainer.Startable; import org.sonar.api.web.ServletFilter; /** * @since 3.5 */ -public class RegisterServletFilters { +public class RegisterServletFilters implements Startable { private final ServletFilter[] filters; public RegisterServletFilters(ServletFilter[] filters) { @@ -37,7 +37,8 @@ public class RegisterServletFilters { this(new ServletFilter[0]); } - public void start() throws ServletException { + @Override + public void start() { if (MasterServletFilter.INSTANCE != null) { // Probably a database upgrade. MasterSlaveFilter was instantiated by the servlet container // while picocontainer was not completely up. @@ -45,4 +46,9 @@ public class RegisterServletFilters { MasterServletFilter.INSTANCE.initFilters(Arrays.asList(filters)); } } + + @Override + public void stop() { + // nothing to do + } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java index 8073e5a4c25..4b9df426660 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java +++ b/server/sonar-server/src/main/java/org/sonar/server/qualityprofile/RegisterQualityProfiles.java @@ -27,6 +27,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.function.Function; +import org.picocontainer.Startable; import org.sonar.api.server.ServerSide; import org.sonar.api.utils.System2; import org.sonar.api.utils.log.Logger; @@ -47,7 +48,7 @@ import static org.sonar.server.qualityprofile.ActiveRuleInheritance.NONE; * Synchronize Quality profiles during server startup */ @ServerSide -public class RegisterQualityProfiles { +public class RegisterQualityProfiles implements Startable { private static final Logger LOGGER = Loggers.get(RegisterQualityProfiles.class); @@ -69,6 +70,7 @@ public class RegisterQualityProfiles { this.system2 = system2; } + @Override public void start() { List builtInQProfiles = builtInQProfileRepository.get(); if (builtInQProfiles.isEmpty()) { @@ -106,6 +108,11 @@ public class RegisterQualityProfiles { profiler.stopDebug(); } + @Override + public void stop() { + // nothing to do + } + private Map loadPersistedProfiles(DbSession dbSession) { return dbClient.qualityProfileDao().selectBuiltInRuleProfiles(dbSession).stream() .collect(MoreCollectors.uniqueIndex(rp -> new QProfileName(rp.getLanguage(), rp.getName()))); diff --git a/server/sonar-server/src/main/java/org/sonar/server/startup/ClusterConfigurationCheck.java b/server/sonar-server/src/main/java/org/sonar/server/startup/ClusterConfigurationCheck.java index 2279694159b..92a1f6e0829 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/startup/ClusterConfigurationCheck.java +++ b/server/sonar-server/src/main/java/org/sonar/server/startup/ClusterConfigurationCheck.java @@ -43,6 +43,7 @@ public class ClusterConfigurationCheck implements Startable { } } + @Override public void stop() { // Nothing to do } diff --git a/server/sonar-server/src/main/java/org/sonar/server/startup/RegisterMetrics.java b/server/sonar-server/src/main/java/org/sonar/server/startup/RegisterMetrics.java index f68c44aea50..39bdbf8edfb 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/startup/RegisterMetrics.java +++ b/server/sonar-server/src/main/java/org/sonar/server/startup/RegisterMetrics.java @@ -26,6 +26,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import javax.annotation.Nonnull; +import org.picocontainer.Startable; import org.sonar.api.measures.CoreMetrics; import org.sonar.api.measures.Metric; import org.sonar.api.measures.Metrics; @@ -40,7 +41,7 @@ import static com.google.common.collect.FluentIterable.from; import static com.google.common.collect.Iterables.concat; import static com.google.common.collect.Lists.newArrayList; -public class RegisterMetrics { +public class RegisterMetrics implements Startable { private static final Logger LOG = Loggers.get(RegisterMetrics.class); @@ -59,10 +60,16 @@ public class RegisterMetrics { this(dbClient, new Metrics[] {}); } + @Override public void start() { register(concat(CoreMetrics.getMetrics(), getPluginMetrics())); } + @Override + public void stop() { + // nothing to do + } + void register(Iterable metrics) { Profiler profiler = Profiler.create(LOG).startInfo("Register metrics"); try (DbSession session = dbClient.openSession(false)) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/startup/RegisterPermissionTemplates.java b/server/sonar-server/src/main/java/org/sonar/server/startup/RegisterPermissionTemplates.java index e16a4cca99e..fd2ff4590cf 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/startup/RegisterPermissionTemplates.java +++ b/server/sonar-server/src/main/java/org/sonar/server/startup/RegisterPermissionTemplates.java @@ -19,6 +19,7 @@ */ package org.sonar.server.startup; +import org.picocontainer.Startable; import org.sonar.api.security.DefaultGroups; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; @@ -37,7 +38,7 @@ import java.util.Optional; import static java.lang.String.format; -public class RegisterPermissionTemplates { +public class RegisterPermissionTemplates implements Startable { private static final Logger LOG = Loggers.get(RegisterPermissionTemplates.class); private static final String DEFAULT_TEMPLATE_UUID = "default_template"; @@ -50,6 +51,7 @@ public class RegisterPermissionTemplates { this.defaultOrganizationProvider = defaultOrganizationProvider; } + @Override public void start() { Profiler profiler = Profiler.create(Loggers.get(getClass())).startInfo("Register permission templates"); @@ -66,6 +68,11 @@ public class RegisterPermissionTemplates { profiler.stopDebug(); } + @Override + public void stop() { + // nothing to do + } + private PermissionTemplateDto getOrInsertDefaultTemplate(DbSession dbSession, String defaultOrganizationUuid) { PermissionTemplateDto permissionTemplateDto = dbClient.permissionTemplateDao().selectByUuid(dbSession, DEFAULT_TEMPLATE_UUID); if (permissionTemplateDto != null) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/startup/RenameDeprecatedPropertyKeys.java b/server/sonar-server/src/main/java/org/sonar/server/startup/RenameDeprecatedPropertyKeys.java index a05840d0741..7be27df5c8a 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/startup/RenameDeprecatedPropertyKeys.java +++ b/server/sonar-server/src/main/java/org/sonar/server/startup/RenameDeprecatedPropertyKeys.java @@ -20,15 +20,16 @@ package org.sonar.server.startup; import com.google.common.base.Strings; -import org.sonar.api.utils.log.Loggers; +import org.picocontainer.Startable; import org.sonar.api.config.PropertyDefinition; import org.sonar.api.config.PropertyDefinitions; +import org.sonar.api.utils.log.Loggers; import org.sonar.db.property.PropertiesDao; /** * @since 3.4 */ -public class RenameDeprecatedPropertyKeys { +public class RenameDeprecatedPropertyKeys implements Startable { private PropertiesDao dao; private PropertyDefinitions definitions; @@ -38,6 +39,7 @@ public class RenameDeprecatedPropertyKeys { this.definitions = definitions; } + @Override public void start() { Loggers.get(RenameDeprecatedPropertyKeys.class).info("Rename deprecated property keys"); for (PropertyDefinition definition : definitions.getAll()) { @@ -46,4 +48,9 @@ public class RenameDeprecatedPropertyKeys { } } } + + @Override + public void stop() { + // nothing to do + } } 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 6e57afd8ead..da2587ca9df 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 @@ -310,8 +310,7 @@ public class ComponentContainer implements ContainerPopulator.Container { } public static MutablePicoContainer createPicoContainer() { - NullComponentMonitor componentMonitor = new NullComponentMonitor(); - return new ExtendedDefaultPicoContainer(new OptInCaching(), new StopSafeReflectionLifecycleStrategy(componentMonitor), null, componentMonitor); + return new ExtendedDefaultPicoContainer(new OptInCaching(), new StartableCloseableSafeLifecyleStrategy(), null, new NullComponentMonitor()); } public ComponentContainer getParent() { diff --git a/sonar-core/src/main/java/org/sonar/core/platform/StartableCloseableSafeLifecyleStrategy.java b/sonar-core/src/main/java/org/sonar/core/platform/StartableCloseableSafeLifecyleStrategy.java new file mode 100644 index 00000000000..a2b912bf722 --- /dev/null +++ b/sonar-core/src/main/java/org/sonar/core/platform/StartableCloseableSafeLifecyleStrategy.java @@ -0,0 +1,105 @@ +/* + * SonarQube + * Copyright (C) 2009-2019 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 java.io.Closeable; +import java.io.Serializable; +import java.util.Arrays; +import java.util.stream.Stream; +import org.picocontainer.ComponentAdapter; +import org.picocontainer.LifecycleStrategy; +import org.picocontainer.Startable; +import org.sonar.api.utils.log.Logger; +import org.sonar.api.utils.log.Loggers; + +public class StartableCloseableSafeLifecyleStrategy implements LifecycleStrategy, Serializable { + private static final Class[] TYPES_WITH_LIFECYCLE = new Class[] {Startable.class, org.sonar.api.Startable.class, Closeable.class, AutoCloseable.class}; + + private static final Logger LOG = Loggers.get(StartableCloseableSafeLifecyleStrategy.class); + + @Override + public void start(Object component) { + if (component instanceof Startable) { + ((Startable) component).start(); + } else if (component instanceof org.sonar.api.Startable) { + ((org.sonar.api.Startable) component).start(); + } + } + + @Override + public void stop(Object component) { + try { + if (component instanceof Startable) { + ((Startable) component).stop(); + } else if (component instanceof org.sonar.api.Startable) { + ((org.sonar.api.Startable) component).stop(); + } + } catch (RuntimeException | Error e) { + Loggers.get(StartableCloseableSafeLifecyleStrategy.class) + .warn("Stopping of component {} failed", component.getClass().getCanonicalName(), e); + } + } + + @Override + public void dispose(Object component) { + try { + if (component instanceof Closeable) { + ((Closeable) component).close(); + } else if (component instanceof AutoCloseable) { + ((AutoCloseable) component).close(); + } + } catch (Exception e) { + Loggers.get(StartableCloseableSafeLifecyleStrategy.class) + .warn("Dispose of component {} failed", component.getClass().getCanonicalName(), e); + } + } + + @Override + public boolean hasLifecycle(Class type) { + if (Arrays.stream(TYPES_WITH_LIFECYCLE).anyMatch(t1 -> t1.isAssignableFrom(type))) { + return true; + } + + if (Stream.of("start", "stop").anyMatch(t -> hasMethod(type, t))) { + LOG.warn("Component of type {} defines methods start() and/or stop(). Neither will be invoked to start/stop the component." + + " Please implement either {} or {}", + type, Startable.class.getName(), org.sonar.api.Startable.class.getName()); + } + if (hasMethod(type, "close")) { + LOG.warn("Component of type {} defines method close(). It won't be invoked to dispose the component." + + " Please implement either {} or {}", + type, Closeable.class.getName(), AutoCloseable.class.getName()); + } + return false; + } + + private static boolean hasMethod(Class type, String methodName) { + try { + return type.getMethod(methodName) != null; + } catch (NoSuchMethodException e) { + return false; + } + } + + @Override + public boolean isLazy(ComponentAdapter adapter) { + return false; + } +} 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 deleted file mode 100644 index 92c28c83b1f..00000000000 --- a/sonar-core/src/main/java/org/sonar/core/platform/StopSafeReflectionLifecycleStrategy.java +++ /dev/null @@ -1,60 +0,0 @@ -/* - * SonarQube - * Copyright (C) 2009-2019 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: - *
  • - *
      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)
    - *
      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
    - *
    • - */ -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 67bd7b63ff7..4584ce8991b 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,12 +19,14 @@ */ package org.sonar.core.platform; +import java.io.Closeable; 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; +import org.picocontainer.Startable; import org.picocontainer.injectors.ProviderAdapter; import org.sonar.api.Property; import org.sonar.api.config.PropertyDefinitions; @@ -48,7 +50,7 @@ public class ComponentContainerTest { } @Test - public void should_start_and_stop() { + public void should_start_and_stop_component_extending_pico_Startable() { ComponentContainer container = spy(new ComponentContainer()); container.addSingleton(StartableStoppableComponent.class); container.startComponents(); @@ -62,6 +64,37 @@ public class ComponentContainerTest { assertThat(container.getComponentByType(StartableStoppableComponent.class).stopped).isTrue(); } + @Test + public void should_start_and_stop_component_extending_API_Startable() { + ComponentContainer container = spy(new ComponentContainer()); + container.addSingleton(StartableStoppableApiComponent.class); + container.startComponents(); + + assertThat(container.getComponentByType(StartableStoppableApiComponent.class).started).isTrue(); + assertThat(container.getComponentByType(StartableStoppableApiComponent.class).stopped).isFalse(); + verify(container).doBeforeStart(); + verify(container).doAfterStart(); + + container.stopComponents(); + assertThat(container.getComponentByType(StartableStoppableApiComponent.class).stopped).isTrue(); + } + + @Test + public void should_not_start_and_stop_component_just_having_start_and_stop_method() { + ComponentContainer container = spy(new ComponentContainer()); + container.addSingleton(ReflectionStartableStoppableComponent.class); + container.startComponents(); + + assertThat(container.getComponentByType(ReflectionStartableStoppableComponent.class).started).isFalse(); + assertThat(container.getComponentByType(ReflectionStartableStoppableComponent.class).stopped).isFalse(); + verify(container).doBeforeStart(); + verify(container).doAfterStart(); + + container.stopComponents(); + assertThat(container.getComponentByType(ReflectionStartableStoppableComponent.class).started).isFalse(); + assertThat(container.getComponentByType(ReflectionStartableStoppableComponent.class).stopped).isFalse(); + } + @Test public void should_start_and_stop_hierarchy_of_containers() { StartableStoppableComponent parentComponent = new StartableStoppableComponent(); @@ -296,6 +329,10 @@ public class ComponentContainerTest { container.getComponentByType(FailingStopWithOOMComponent2.class) }; + container.stopComponents(); + + assertThat(container.getPicoContainer().getLifecycleState().isDisposed()).isTrue(); + Arrays.stream(components).forEach(cpt -> assertThat(cpt.stopped).isTrue()); } @Test @@ -308,6 +345,16 @@ public class ComponentContainerTest { container.execute(); } + @Test + public void stop_exceptionin_API_component_should_not_hide_start_exception() { + ComponentContainer container = new ComponentContainer(); + container.add(UnstartableApiComponent.class, FailingStopWithISEComponent.class); + + thrown.expect(IllegalStateException.class); + thrown.expectMessage("Fail to start"); + container.execute(); + } + @Test public void should_execute_components() { ComponentContainer container = new ComponentContainer(); @@ -339,7 +386,7 @@ public class ComponentContainerTest { * Method close() must be executed after stop() */ @Test - public void should_close_components_with_lifecycle() { + public void should_close_Closeable_components_with_lifecycle() { ComponentContainer container = new ComponentContainer(); StartableCloseableComponent component = new StartableCloseableComponent(); container.add(component); @@ -351,24 +398,84 @@ public class ComponentContainerTest { assertThat(component.isClosedAfterStop).isTrue(); } - public static class StartableStoppableComponent { + /** + * Method close() must be executed after stop() + */ + @Test + public void should_close_AutoCloseable_components_with_lifecycle() { + ComponentContainer container = new ComponentContainer(); + StartableAutoCloseableComponent component = new StartableAutoCloseableComponent(); + container.add(component); + + container.execute(); + + assertThat(component.isStopped).isTrue(); + assertThat(component.isClosed).isTrue(); + assertThat(component.isClosedAfterStop).isTrue(); + } + + public static class StartableStoppableComponent implements Startable { public boolean started = false; public boolean stopped = false; + @Override public void start() { started = true; } + @Override public void stop() { stopped = true; } } - public static class UnstartableComponent { + public static class StartableStoppableApiComponent implements org.sonar.api.Startable { + public boolean started = false; + public boolean stopped = false; + + @Override + public void start() { + started = true; + } + + @Override + public void stop() { + stopped = true; + } + } + + public static class ReflectionStartableStoppableComponent { + public boolean started = false; + public boolean stopped = false; + + public void start() { + started = true; + } + + public void stop() { + stopped = true; + } + } + + public static class UnstartableComponent implements Startable { + @Override + public void start() { + throw new IllegalStateException("Fail to start"); + } + + @Override + public void stop() { + + } + } + + public static class UnstartableApiComponent implements org.sonar.api.Startable { + @Override public void start() { throw new IllegalStateException("Fail to start"); } + @Override public void stop() { } @@ -431,22 +538,50 @@ public class ComponentContainerTest { public boolean isClosed = false; @Override - public void close() throws Exception { + public void close() { isClosed = true; } } - public static class StartableCloseableComponent implements AutoCloseable { + public static class StartableAutoCloseableComponent implements Startable,AutoCloseable { public boolean isClosed = false; public boolean isStopped = false; public boolean isClosedAfterStop = false; + @Override + public void start() { + // nothing to do + } + + @Override + public void stop() { + isStopped = true; + } + + @Override + public void close() { + isClosed = true; + isClosedAfterStop = isStopped; + } + } + + public static class StartableCloseableComponent implements Startable, Closeable { + public boolean isClosed = false; + public boolean isStopped = false; + public boolean isClosedAfterStop = false; + + @Override + public void start() { + // nothing to do + } + + @Override public void stop() { isStopped = true; } @Override - public void close() throws Exception { + public void close() { isClosed = true; isClosedAfterStop = isStopped; } diff --git a/sonar-core/src/test/java/org/sonar/core/platform/PicoUtilsTest.java b/sonar-core/src/test/java/org/sonar/core/platform/PicoUtilsTest.java index 4cc7746b72d..0e168695980 100644 --- a/sonar-core/src/test/java/org/sonar/core/platform/PicoUtilsTest.java +++ b/sonar-core/src/test/java/org/sonar/core/platform/PicoUtilsTest.java @@ -19,11 +19,12 @@ */ package org.sonar.core.platform; -import java.io.IOException; +import java.lang.reflect.Method; import org.junit.Test; import org.picocontainer.Characteristics; import org.picocontainer.MutablePicoContainer; import org.picocontainer.PicoLifecycleException; +import org.picocontainer.Startable; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.fail; @@ -31,11 +32,19 @@ import static org.junit.Assert.fail; public class PicoUtilsTest { @Test - public void shouldSanitizePicoLifecycleException() { - Throwable th = PicoUtils.sanitize(newPicoLifecycleException(false)); + public void shouldSanitizePicoLifecycleException() throws NoSuchMethodException { + UncheckedFailureComponent instance = new UncheckedFailureComponent(); + Method method = UncheckedFailureComponent.class.getMethod("start"); + try { + instance.start(); + fail("Start should have thrown a IllegalStateException"); + } + catch (IllegalStateException e) { + Throwable th = PicoUtils.sanitize(new PicoLifecycleException(method, instance, e)); - assertThat(th).isInstanceOf(IllegalStateException.class); - assertThat(th.getMessage()).isEqualTo("A good reason to fail"); + assertThat(th).isInstanceOf(IllegalStateException.class); + assertThat(th.getMessage()).isEqualTo("A good reason to fail"); + } } @Test @@ -57,49 +66,35 @@ public class PicoUtilsTest { @Test public void shouldPropagateInitialUncheckedException() { try { - PicoUtils.propagate(newPicoLifecycleException(false)); + PicoUtils.propagate(newPicoLifecycleException()); fail(); } catch (RuntimeException e) { assertThat(e).isInstanceOf(IllegalStateException.class); } } - @Test - public void shouldThrowUncheckedExceptionWhenPropagatingCheckedException() { - try { - PicoUtils.propagate(newPicoLifecycleException(true)); - fail(); - } catch (RuntimeException e) { - assertThat(e.getCause()).isInstanceOf(IOException.class); - assertThat(e.getCause().getMessage()).isEqualTo("Checked"); - } - } - private PicoLifecycleException newPicoLifecycleException(boolean initialCheckedException) { + private PicoLifecycleException newPicoLifecycleException() { MutablePicoContainer container = ComponentContainer.createPicoContainer().as(Characteristics.CACHE); - if (initialCheckedException) { - container.addComponent(CheckedFailureComponent.class); - } else { - container.addComponent(UncheckedFailureComponent.class); - } + container.addComponent(UncheckedFailureComponent.class); try { container.start(); - return null; + throw new IllegalStateException("An exception should have been thrown by start()"); } catch (PicoLifecycleException e) { return e; } } - public static class UncheckedFailureComponent { + public static class UncheckedFailureComponent implements Startable { public void start() { throw new IllegalStateException("A good reason to fail"); } - } - public static class CheckedFailureComponent { - public void start() throws IOException { - throw new IOException("Checked"); + @Override + public void stop() { + // nothing to do } } + } diff --git a/sonar-core/src/test/java/org/sonar/core/platform/StartableCloseableSafeLifecyleStrategyTest.java b/sonar-core/src/test/java/org/sonar/core/platform/StartableCloseableSafeLifecyleStrategyTest.java new file mode 100644 index 00000000000..4c92df1e01b --- /dev/null +++ b/sonar-core/src/test/java/org/sonar/core/platform/StartableCloseableSafeLifecyleStrategyTest.java @@ -0,0 +1,218 @@ +/* + * SonarQube + * Copyright (C) 2009-2019 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 java.io.Closeable; +import java.io.IOException; +import org.junit.Rule; +import org.junit.Test; +import org.sonar.api.Startable; +import org.sonar.api.utils.log.LogTester; +import org.sonar.api.utils.log.LoggerLevel; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; + +public class StartableCloseableSafeLifecyleStrategyTest { + @Rule + public LogTester logTester = new LogTester(); + + private StartableCloseableSafeLifecyleStrategy underTest = new StartableCloseableSafeLifecyleStrategy(); + + @Test + public void start_calls_start_on_Startable_subclass() { + Startable startable = mock(Startable.class); + + underTest.start(startable); + + verify(startable).start(); + verifyNoMoreInteractions(startable); + } + + @Test + public void start_calls_start_on_api_Startable_subclass() { + org.picocontainer.Startable startable = mock(org.picocontainer.Startable.class); + + underTest.start(startable); + + verify(startable).start(); + verifyNoMoreInteractions(startable); + } + + @Test + public void start_does_not_call_stop_on_class_with_method_start_not_implementing_startable() { + Object startable = spy(new Object() { + public void start() { + // nothing to do + } + }); + + underTest.start(startable); + + verifyNoMoreInteractions(startable); + } + + @Test + public void stop_calls_stop_on_Startable_subclass() { + Startable startable = mock(Startable.class); + + underTest.stop(startable); + + verify(startable).stop(); + verifyNoMoreInteractions(startable); + } + + @Test + public void stop_calls_stop_on_api_Startable_subclass() { + org.picocontainer.Startable startable = mock(org.picocontainer.Startable.class); + + underTest.stop(startable); + + verify(startable).stop(); + verifyNoMoreInteractions(startable); + } + + @Test + public void stop_does_not_call_stop_on_class_with_method_stop_not_implementing_startable() { + Object startable = spy(new Object() { + public void stop() { + // nothing to do + } + }); + + underTest.stop(startable); + + verifyNoMoreInteractions(startable); + } + + @Test + public void dispose_calls_close_on_Closeable_subclass() throws IOException { + Closeable closeable = mock(Closeable.class); + + underTest.dispose(closeable); + + verify(closeable).close(); + verifyNoMoreInteractions(closeable); + } + + @Test + public void dispose_calls_close_on_AutoCloseable_subclass() throws Exception { + AutoCloseable autoCloseable = mock(AutoCloseable.class); + + underTest.dispose(autoCloseable); + + verify(autoCloseable).close(); + verifyNoMoreInteractions(autoCloseable); + } + + @Test + public void dispose_does_not_call_close_on_class_with_method_close_not_implementing_Closeable_nor_AutoCloseable() { + Object closeable = spy(new Object() { + public void close() { + // nothing to do + } + }); + + underTest.dispose(closeable); + + verifyNoMoreInteractions(closeable); + } + + @Test + public void hasLifecycle_returns_true_on_Startable_and_subclass() { + Startable startable = mock(Startable.class); + + assertThat(underTest.hasLifecycle(Startable.class)).isTrue(); + assertThat(underTest.hasLifecycle(startable.getClass())).isTrue(); + } + + @Test + public void hasLifecycle_returns_true_on_api_Startable_and_subclass() { + org.picocontainer.Startable startable = mock(org.picocontainer.Startable.class); + + assertThat(underTest.hasLifecycle(org.picocontainer.Startable.class)).isTrue(); + assertThat(underTest.hasLifecycle(startable.getClass())).isTrue(); + } + + @Test + public void hasLifecycle_returns_true_on_api_Closeable_and_subclass() { + Closeable closeable = mock(Closeable.class); + + assertThat(underTest.hasLifecycle(Closeable.class)).isTrue(); + assertThat(underTest.hasLifecycle(closeable.getClass())).isTrue(); + } + + @Test + public void hasLifecycle_returns_true_on_api_AutoCloseable_and_subclass() { + AutoCloseable autoCloseable = mock(AutoCloseable.class); + + assertThat(underTest.hasLifecycle(AutoCloseable.class)).isTrue(); + assertThat(underTest.hasLifecycle(autoCloseable.getClass())).isTrue(); + } + + @Test + public void hasLifeCycle_returns_false_and_log_a_warning_for_type_defining_start_without_implementating_Startable() { + Object startable = new Object() { + public void start() { + // nothing to do + } + }; + + assertThat(underTest.hasLifecycle(startable.getClass())).isFalse(); + verifyWarnLog(startable.getClass()); + } + + @Test + public void hasLifeCycle_returns_false_and_log_a_warning_for_type_defining_stop_without_implementating_Startable() { + Object startable = new Object() { + public void stop() { + // nothing to do + } + }; + + assertThat(underTest.hasLifecycle(startable.getClass())).isFalse(); + verifyWarnLog(startable.getClass()); + } + + private void verifyWarnLog(Class type) { + assertThat(logTester.logs()).hasSize(1); + assertThat(logTester.logs(LoggerLevel.WARN)) + .contains("Component of type class " + type.getName() + " defines methods start() and/or stop(). Neither will be invoked to start/stop the component. " + + "Please implement either org.picocontainer.Startable or org.sonar.api.Startable"); + } + + @Test + public void hasLifeCycle_returns_false_and_log_a_warning_for_type_defining_close_without_implementating_Closeable_nor_AutoCloseable() { + Object startable = new Object() { + public void close() { + // nothing to do + } + }; + + assertThat(underTest.hasLifecycle(startable.getClass())).isFalse(); + assertThat(logTester.logs()).hasSize(1); + assertThat(logTester.logs(LoggerLevel.WARN)) + .contains("Component of type class " + startable.getClass().getName() + " defines method close(). It won't be invoked to dispose the component. " + + "Please implement either java.io.Closeable or java.lang.AutoCloseable"); + } +} diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/utils/internal/TempFolderCleaner.java b/sonar-plugin-api/src/main/java/org/sonar/api/utils/internal/TempFolderCleaner.java index eed8a5fed65..3a7836cff4f 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/utils/internal/TempFolderCleaner.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/utils/internal/TempFolderCleaner.java @@ -19,11 +19,12 @@ */ package org.sonar.api.utils.internal; +import org.sonar.api.Startable; import org.sonar.api.server.ServerSide; import org.sonar.api.utils.TempFolder; @ServerSide -public class TempFolderCleaner { +public class TempFolderCleaner implements Startable { private TempFolder defaultTempFolder; @@ -35,6 +36,7 @@ public class TempFolderCleaner { * This method should not be renamed. It follows the naming convention * defined by IoC container. */ + @Override public void start() { // Nothing to do } @@ -43,6 +45,7 @@ public class TempFolderCleaner { * This method should not be renamed. It follows the naming convention * defined by IoC container. */ + @Override public void stop() { ((DefaultTempFolder) defaultTempFolder).clean(); } -- 2.39.5