From 6309b7510b59176c836102e2d19e736d260114da Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Fri, 12 Sep 2014 17:46:05 +0200 Subject: [PATCH] SONAR-4898 fix quality flaws --- .../process/monitor/RmiJmxConnector.java | 2 - .../sonar/process/monitor/StreamGobbler.java | 11 +- .../org/sonar/process/monitor/Timeouts.java | 2 +- .../process/monitor/JavaCommandTest.java | 7 + .../sonar/process/monitor/MonitorTest.java | 6 +- .../process/monitor/RmiJmxConnectorTest.java | 15 ++ .../process/monitor/StreamGobblerTest.java | 51 ++++++ .../java/org/sonar/process/StopperThread.java | 5 +- .../java/org/sonar/process/JmxUtilsTest.java | 9 +- .../org/sonar/search/SearchServerTest.java | 155 ------------------ 10 files changed, 95 insertions(+), 168 deletions(-) create mode 100644 server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/StreamGobblerTest.java delete mode 100644 server/sonar-search/src/test/java/org/sonar/search/SearchServerTest.java diff --git a/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/RmiJmxConnector.java b/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/RmiJmxConnector.java index 876ccc2b22f..68dfc34182b 100644 --- a/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/RmiJmxConnector.java +++ b/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/RmiJmxConnector.java @@ -68,8 +68,6 @@ class RmiJmxConnector implements JmxConnector { ProcessMXBean mxBean = execute(callable, timeoutMs); if (mxBean != null) { register(processRef, mxBean); - } else if (!processRef.isTerminated()) { - throw new IllegalStateException("Fail to connect to JMX RMI server of " + processRef); } } diff --git a/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/StreamGobbler.java b/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/StreamGobbler.java index 55d95c8a467..cb412965f55 100644 --- a/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/StreamGobbler.java +++ b/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/StreamGobbler.java @@ -38,9 +38,13 @@ class StreamGobbler extends Thread { private final Logger logger; StreamGobbler(InputStream is, String processKey) { + this(is, processKey, LoggerFactory.getLogger(processKey)); + } + + StreamGobbler(InputStream is, String processKey, Logger logger) { super(String.format("Gobbler[%s]", processKey)); this.is = is; - this.logger = LoggerFactory.getLogger(processKey); + this.logger = logger; } @Override @@ -51,8 +55,8 @@ class StreamGobbler extends Thread { while ((line = br.readLine()) != null) { logger.info(line); } - } catch (Exception ignored) { - + } catch (Exception e) { + logger.error("Fail to read process logs", e); } finally { IOUtils.closeQuietly(br); } @@ -63,6 +67,7 @@ class StreamGobbler extends Thread { try { gobbler.join(); } catch (InterruptedException ignored) { + // consider as finished } } } diff --git a/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/Timeouts.java b/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/Timeouts.java index 68934f21066..a3f7210f37f 100644 --- a/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/Timeouts.java +++ b/server/sonar-process-monitor/src/main/java/org/sonar/process/monitor/Timeouts.java @@ -25,7 +25,7 @@ package org.sonar.process.monitor; class Timeouts { private long terminationTimeout = 120000L; - private long jmxConnectionTimeout = 30000L; + private long jmxConnectionTimeout = 15000L; private long monitorPingInterval = 3000L; private long monitorIsReadyTimeout = 10000L; private long autokillPingTimeout = 60000L; diff --git a/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/JavaCommandTest.java b/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/JavaCommandTest.java index 766f24c86fc..8744fb73ff3 100644 --- a/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/JavaCommandTest.java +++ b/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/JavaCommandTest.java @@ -76,4 +76,11 @@ public class JavaCommandTest { command.addJavaOption("-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5005"); assertThat(command.isDebugMode()).isTrue(); } + + @Test + public void split_java_options() throws Exception { + JavaCommand command = new JavaCommand("foo"); + command.addJavaOptions("-Xmx512m -Xms256m -Dfoo"); + assertThat(command.getJavaOptions()).containsOnly("-Xmx512m", "-Xms256m", "-Dfoo"); + } } diff --git a/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/MonitorTest.java b/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/MonitorTest.java index 6f0be0eb5be..043054c40ee 100644 --- a/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/MonitorTest.java +++ b/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/MonitorTest.java @@ -33,7 +33,6 @@ import org.sonar.process.SystemExit; import java.io.File; import java.io.IOException; -import java.net.ConnectException; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -359,10 +358,7 @@ public class MonitorTest { .readTimeout(500).connectTimeout(500); return httpRequest.ok() && httpRequest.body().equals("ping"); } catch (HttpRequest.HttpRequestException e) { - if (e.getCause() instanceof ConnectException) { - return false; - } - throw new IllegalStateException("Fail to know the process status", e); + return false; } } diff --git a/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/RmiJmxConnectorTest.java b/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/RmiJmxConnectorTest.java index c4a0f735d60..2289c446566 100644 --- a/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/RmiJmxConnectorTest.java +++ b/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/RmiJmxConnectorTest.java @@ -22,6 +22,7 @@ package org.sonar.process.monitor; import org.junit.Test; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; +import org.sonar.process.NetworkUtils; import org.sonar.process.ProcessMXBean; import static org.fest.assertions.Assertions.assertThat; @@ -31,6 +32,20 @@ import static org.mockito.Mockito.when; public class RmiJmxConnectorTest { + @Test + public void fail_to_connect_in_timely_fashion() throws Exception { + RmiJmxConnector connector = new RmiJmxConnector(); + ProcessRef ref = mock(ProcessRef.class); + + JavaCommand command = new JavaCommand("foo").setJmxPort(NetworkUtils.freePort()); + try { + connector.connect(command, ref, 0L); + fail(); + } catch (IllegalStateException e) { + // ok + } + } + @Test public void throw_exception_on_timeout() throws Exception { RmiJmxConnector connector = new RmiJmxConnector(); diff --git a/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/StreamGobblerTest.java b/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/StreamGobblerTest.java new file mode 100644 index 00000000000..16ebb2837c5 --- /dev/null +++ b/server/sonar-process-monitor/src/test/java/org/sonar/process/monitor/StreamGobblerTest.java @@ -0,0 +1,51 @@ +/* + * SonarQube, open source software quality management tool. + * Copyright (C) 2008-2014 SonarSource + * mailto:contact AT sonarsource DOT com + * + * SonarQube 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. + * + * SonarQube 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.process.monitor; + +import org.apache.commons.io.IOUtils; +import org.junit.Test; +import org.slf4j.Logger; + +import java.io.InputStream; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.verifyZeroInteractions; + +public class StreamGobblerTest { + + @Test + public void forward_stream_to_log() throws Exception { + InputStream stream = IOUtils.toInputStream("one\nsecond log\nthird log\n"); + Logger logger = mock(Logger.class); + + StreamGobbler gobbler = new StreamGobbler(stream, "WEB", logger); + verifyZeroInteractions(logger); + + gobbler.start(); + StreamGobbler.waitUntilFinish(gobbler); + + verify(logger).info("one"); + verify(logger).info("second log"); + verify(logger).info("third log"); + verifyNoMoreInteractions(logger); + } +} diff --git a/server/sonar-process/src/main/java/org/sonar/process/StopperThread.java b/server/sonar-process/src/main/java/org/sonar/process/StopperThread.java index 2d0c6734b30..97e2183e985 100644 --- a/server/sonar-process/src/main/java/org/sonar/process/StopperThread.java +++ b/server/sonar-process/src/main/java/org/sonar/process/StopperThread.java @@ -19,6 +19,8 @@ */ package org.sonar.process; +import org.slf4j.LoggerFactory; + import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; @@ -50,7 +52,8 @@ class StopperThread extends Thread { try { future.get(terminationTimeout, TimeUnit.MILLISECONDS); } catch (Exception e) { - future.cancel(true); + LoggerFactory.getLogger(getClass()).error("Can not terminate in " + terminationTimeout + "ms", e); + } finally { executor.shutdownNow(); } } diff --git a/server/sonar-process/src/test/java/org/sonar/process/JmxUtilsTest.java b/server/sonar-process/src/test/java/org/sonar/process/JmxUtilsTest.java index 599ea5d7a30..0e9d320cd74 100644 --- a/server/sonar-process/src/test/java/org/sonar/process/JmxUtilsTest.java +++ b/server/sonar-process/src/test/java/org/sonar/process/JmxUtilsTest.java @@ -77,7 +77,7 @@ public class JmxUtilsTest { } @Test - public void testRegisterMBean() throws Exception { + public void register_mbean() throws Exception { // 0 Get mbServer and create out test MXBean MBeanServer mbeanServer = ManagementFactory.getPlatformMBeanServer(); MyBean mxBean = new MyBean(); @@ -87,6 +87,13 @@ public class JmxUtilsTest { assertThat(mbeanServer.isRegistered(objectName)).isFalse(); JmxUtils.registerMBean(mxBean, mxBean.getClass().getSimpleName()); assertThat(mbeanServer.isRegistered(objectName)).isTrue(); + + try { + JmxUtils.registerMBean(new Object(), ""); + fail(); + } catch (IllegalStateException e) { + // ok + } } @Test diff --git a/server/sonar-search/src/test/java/org/sonar/search/SearchServerTest.java b/server/sonar-search/src/test/java/org/sonar/search/SearchServerTest.java deleted file mode 100644 index 54fb02435e9..00000000000 --- a/server/sonar-search/src/test/java/org/sonar/search/SearchServerTest.java +++ /dev/null @@ -1,155 +0,0 @@ -/* - * SonarQube, open source software quality management tool. - * Copyright (C) 2008-2014 SonarSource - * mailto:contact AT sonarsource DOT com - * - * SonarQube 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. - * - * SonarQube 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.search; - -import org.elasticsearch.action.admin.cluster.health.ClusterHealthStatus; -import org.elasticsearch.client.transport.TransportClient; -import org.elasticsearch.common.settings.ImmutableSettings; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.transport.InetSocketTransportAddress; -import org.junit.After; -import org.junit.Before; -import org.junit.Ignore; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; -import org.sonar.process.JmxUtils; -import org.sonar.process.Props; - -import javax.management.InstanceNotFoundException; -import javax.management.MBeanRegistrationException; -import javax.management.MBeanServer; - -import java.io.IOException; -import java.lang.management.ManagementFactory; -import java.net.ServerSocket; -import java.util.Properties; - -import static org.fest.assertions.Assertions.assertThat; -import static org.junit.Assert.fail; - -@Ignore -public class SearchServerTest { - - @Rule - public TemporaryFolder temp = new TemporaryFolder(); - - SearchServer searchServer; - int freePort; - int freeESPort; - - @Before - public void setup() throws IOException { - ServerSocket socket = new ServerSocket(0); - freePort = socket.getLocalPort(); - socket.close(); - - socket = new ServerSocket(0); - freeESPort = socket.getLocalPort(); - socket.close(); - } - - @After - public void tearDown() throws MBeanRegistrationException, InstanceNotFoundException { - resetMBeanServer(); - } - - private void resetMBeanServer() throws MBeanRegistrationException, InstanceNotFoundException { - try { - MBeanServer mbeanServer = ManagementFactory.getPlatformMBeanServer(); - mbeanServer.unregisterMBean(JmxUtils.objectName("ES")); - } catch (Exception e) { - e.printStackTrace(); - } - } - - @Test - public void server_fail_to_start() throws Exception { - Properties properties = new Properties(); - - searchServer = new SearchServer(new Props(properties)); - new Thread(new Runnable() { - @Override - public void run() { - searchServer.start(); - } - }).start(); - assertThat(searchServer.isReady()).isFalse(); - - int count = 0; - while (!searchServer.isReady() && count < 5) { - try { - Thread.sleep(100); - } catch (InterruptedException e) { - e.printStackTrace(); - } - count++; - } - assertThat(count).isEqualTo(5); - } - - @Test - public void can_connect() throws Exception { - Properties properties = new Properties(); - properties.setProperty(SearchServer.SONAR_PATH_DATA, temp.newFolder().getAbsolutePath()); - properties.setProperty(SearchServer.SONAR_PATH_TEMP, temp.newFolder().getAbsolutePath()); - properties.setProperty(SearchServer.SONAR_PATH_LOG, temp.newFolder().getAbsolutePath()); - properties.setProperty(SearchServer.ES_PORT_PROPERTY, Integer.toString(freeESPort)); - properties.setProperty(SearchServer.ES_CLUSTER_PROPERTY, "sonarqube"); - - searchServer = new SearchServer(new Props(properties)); - new Thread(new Runnable() { - @Override - public void run() { - searchServer.start(); - } - }).start(); - assertThat(searchServer.isReady()).isFalse(); - - int count = 0; - while (!searchServer.isReady() && count < 100) { - try { - Thread.sleep(500); - } catch (InterruptedException e) { - e.printStackTrace(); - } - count++; - } - assertThat(count).isLessThan(100); - - Settings settings = ImmutableSettings.settingsBuilder() - .put("cluster.name", "sonarqube") - .build(); - TransportClient client = new TransportClient(settings) - .addTransportAddress(new InetSocketTransportAddress("localhost", freeESPort)); - - // 0 assert that we have a OK cluster available - assertThat(client.admin().cluster().prepareClusterStats().get().getStatus()).isEqualTo(ClusterHealthStatus.GREEN); - - // 2 assert that we can shut down ES - searchServer.terminate(); - try { - client.admin().cluster().prepareClusterStats().get().getStatus(); - fail(); - } catch (Exception e) { - assertThat(e.getMessage()).isEqualTo("No node available"); - } - } -} -- 2.39.5