From 9a20c5d4f9ce6183fb6f3182d5b793636f8a413e Mon Sep 17 00:00:00 2001 From: Stephane Gamard Date: Fri, 29 Aug 2014 16:47:58 +0200 Subject: [PATCH] SONAR-4898 - fixed concurrent termination and exception on exit --- .../main/java/org/sonar/process/Monitor.java | 4 +- .../org/sonar/process/MonitoredProcess.java | 32 +++--- .../org/sonar/process/ProcessWrapper.java | 101 ++++++++++-------- 3 files changed, 76 insertions(+), 61 deletions(-) diff --git a/server/process/sonar-process/src/main/java/org/sonar/process/Monitor.java b/server/process/sonar-process/src/main/java/org/sonar/process/Monitor.java index ca227daa248..baa0f00791c 100644 --- a/server/process/sonar-process/src/main/java/org/sonar/process/Monitor.java +++ b/server/process/sonar-process/src/main/java/org/sonar/process/Monitor.java @@ -122,6 +122,7 @@ public class Monitor extends Thread implements Terminable { @Override public synchronized void terminate() { + LOGGER.debug("Monitoring thread is terminating"); if (!monitorExecutionService.isShutdown()) { @@ -131,9 +132,6 @@ public class Monitor extends Thread implements Terminable { watch.cancel(true); } - for (int i = processes.size() - 1; i >= 0; i--) { - processes.get(i).terminate(); - } processes.clear(); interruptAndWait(); } diff --git a/server/process/sonar-process/src/main/java/org/sonar/process/MonitoredProcess.java b/server/process/sonar-process/src/main/java/org/sonar/process/MonitoredProcess.java index c36df449b9e..c1c5d172c4a 100644 --- a/server/process/sonar-process/src/main/java/org/sonar/process/MonitoredProcess.java +++ b/server/process/sonar-process/src/main/java/org/sonar/process/MonitoredProcess.java @@ -124,22 +124,24 @@ public abstract class MonitoredProcess implements ProcessMXBean { @Override public final void terminate() { - if (monitor != null) { - LOGGER.debug("Process[{}] terminating", name); - try { - doTerminate(); - } catch (Exception e) { - LOGGER.error("Fail to terminate " + name, e); - // do not propagate exception - } - monitor.shutdownNow(); - monitor = null; - if (pingTask != null) { - pingTask.cancel(true); - pingTask = null; + synchronized (monitor) { + if (monitor != null) { + LOGGER.debug("Process[{}] terminating", name); + try { + doTerminate(); + } catch (Exception e) { + LOGGER.error("Fail to terminate " + name, e); + // do not propagate exception + } + monitor.shutdownNow(); + monitor = null; + if (pingTask != null) { + pingTask.cancel(true); + pingTask = null; + } + LOGGER.debug("Process[{}] terminated", name); + terminated = true; } - LOGGER.debug("Process[{}] terminated", name); - terminated = true; } } diff --git a/server/process/sonar-process/src/main/java/org/sonar/process/ProcessWrapper.java b/server/process/sonar-process/src/main/java/org/sonar/process/ProcessWrapper.java index c014122288c..5251ca5bd0c 100644 --- a/server/process/sonar-process/src/main/java/org/sonar/process/ProcessWrapper.java +++ b/server/process/sonar-process/src/main/java/org/sonar/process/ProcessWrapper.java @@ -177,13 +177,11 @@ public class ProcessWrapper extends Thread implements Terminable { process.waitFor(); } } catch (Exception e) { - LOGGER.info("ProcessThread has been interrupted. Killing process."); + LOGGER.info("ProcessThread has been interrupted. Killing node."); LOGGER.trace("Process exception", e); } finally { - waitUntilFinish(outputGobbler); - waitUntilFinish(errorGobbler); - ProcessUtils.closeStreams(process); - interruptAndWait(); + ; + ; } } @@ -195,16 +193,6 @@ public class ProcessWrapper extends Thread implements Terminable { return processMXBean; } - private void waitUntilFinish(@Nullable Thread thread) { - if (thread != null) { - try { - thread.join(); - } catch (InterruptedException e) { - LOGGER.error("InterruptedException while waiting finish of " + thread.getName() + " in process '" + getName() + "'", e); - } - } - } - private String buildJavaCommand() { String separator = System.getProperty("file.separator"); return new File(new File(System.getProperty("java.home")), @@ -269,37 +257,41 @@ public class ProcessWrapper extends Thread implements Terminable { return null; } + private final Integer terminationLock = new Integer(1); @Override public void terminate() { - if (processMXBean != null && process != null) { - LOGGER.info("{} stopping", getName()); - // Send the terminate command to process in order to gracefully shutdown. - // Then hardly kill it if it didn't terminate in 30 seconds - ScheduledExecutorService killer = Executors.newScheduledThreadPool(1); - try { - Runnable killerTask = new Runnable() { - @Override - public void run() { - ProcessUtils.destroyQuietly(process); - } - }; - - ScheduledFuture killerFuture = killer.schedule(killerTask, 30, TimeUnit.SECONDS); - processMXBean.terminate(); - killerFuture.cancel(true); - LOGGER.info("{} stopped", getName()); - - } catch (Exception ignored) { - LOGGER.trace("Could not terminate process", ignored); - } finally { - killer.shutdownNow(); + synchronized (terminationLock) { + if (processMXBean != null && process != null) { + LOGGER.info("{} stopping", getName()); + // Send the terminate command to process in order to gracefully shutdown. + // Then hardly kill it if it didn't terminate in 30 seconds + ScheduledExecutorService killer = Executors.newScheduledThreadPool(1); + try { + Runnable killerTask = new Runnable() { + @Override + public void run() { + ProcessUtils.destroyQuietly(process); + } + }; + + ScheduledFuture killerFuture = killer.schedule(killerTask, 30, TimeUnit.SECONDS); + processMXBean.terminate(); + this.join(); + killerFuture.cancel(true); + LOGGER.info("{} stopped", getName()); + + } catch (Exception ignored) { + LOGGER.trace("Could not terminate process", ignored); + } finally { + killer.shutdownNow(); + interruptAndWait(); + } + } else { + // process is not monitored through JMX, but killing it though + ProcessUtils.destroyQuietly(process); } - interruptAndWait(); - } else { - // process is not monitored through JMX, but killing it though - ProcessUtils.destroyQuietly(process); + processMXBean = null; } - processMXBean = null; } public boolean waitForReady() throws InterruptedException { @@ -323,8 +315,20 @@ public class ProcessWrapper extends Thread implements Terminable { } private void interruptAndWait() { - this.interrupt(); try { + //after being interrupted, finalize the goblins + if (outputGobbler != null && outputGobbler.isAlive()) { + waitUntilFinish(outputGobbler); + } + + if (errorGobbler != null && errorGobbler.isAlive()) { + waitUntilFinish(errorGobbler); + } + if (process != null) { + ProcessUtils.closeStreams(process); + } + + //Join while the main thread terminates if (this.isAlive()) { this.join(); } @@ -333,6 +337,17 @@ public class ProcessWrapper extends Thread implements Terminable { } } + private void waitUntilFinish(@Nullable Thread thread) { + if (thread != null && thread.isAlive()) { + try { + thread.join(); + } catch (InterruptedException e) { + LOGGER.error("InterruptedException while waiting finish of " + thread.getName() + " in process '" + getName() + "'", e); + } + } + } + + private static class StreamGobbler extends Thread { private final InputStream is; private final Logger logger; -- 2.39.5