SONAR-12043 Refactor ProcessEntryPoint

Simplifies a bit Stop and HardStop operations

3 files changed, 61 insertions, 134 deletions

diff --git a/server/sonar-process/src/main/java/org/sonar/process/ProcessEntryPoint.java b/server/sonar-process/src/main/java/org/sonar/process/ProcessEntryPoint.java
index 3e4bc25b611..308dbeccb8a 100644
--- a/server/sonar-process/src/main/java/org/sonar/process/ProcessEntryPoint.java
+++ b/server/sonar-process/src/main/java/org/sonar/process/ProcessEntryPoint.java
@@ -20,17 +20,13 @@
 package org.sonar.process;
 
 import java.io.File;
-import java.util.Optional;
 import java.util.concurrent.CountDownLatch;
 import java.util.function.Predicate;
-import javax.annotation.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.sonar.process.sharedmemoryfile.DefaultProcessCommands;
 import org.sonar.process.sharedmemoryfile.ProcessCommands;
 
-import static java.util.Optional.of;
-import static java.util.Optional.ofNullable;
 import static org.sonar.process.Lifecycle.State.STOPPED;
 
 public class ProcessEntryPoint {
@@ -51,11 +47,10 @@ public class ProcessEntryPoint {
   private final StopWatcher hardStopWatcher;
   // new Runnable() is important to avoid conflict of call to ProcessEntryPoint#stop() with Thread#stop()
   private final Runtime runtime;
-  private volatile Monitored monitored;
+  private Monitored monitored;
   private volatile StopperThread stopperThread;
-  private volatile HardStopperThread hardStopperThread;
 
-  private ProcessEntryPoint(Props props, SystemExit exit, ProcessCommands commands, Runtime runtime) {
+  public ProcessEntryPoint(Props props, SystemExit exit, ProcessCommands commands, Runtime runtime) {
     this.props = props;
     this.processKey = props.nonNullValue(PROPERTY_PROCESS_KEY);
     this.exit = exit;
@@ -133,72 +128,42 @@ public class ProcessEntryPoint {
 
   void stop() {
     stopAsync();
-    waitForStop();
-
-
-    stopAsync()
-      .ifPresent(stoppingThread -> {
-        try {
-          // join() does nothing if thread already finished
-          stoppingThread.join();
-          commands.endWatch();
-          exit.exit(0);
-        } catch (InterruptedException e) {
-          // stop can be aborted by a hard stop
-          Thread.currentThread().interrupt();
-        }
-      });
+    monitored.awaitStop();
   }
 
-  private void waitForStop() {
-    try {
-      stopLatch.await();
-    } catch (InterruptedException e) {
-      e.printStackTrace();
-    }
+  /**
+   * Blocks until stopped in a timely fashion (see {@link HardStopperThread})
+   */
+  void hardStop() {
+    hardStopAsync();
+    monitored.awaitStop();
   }
 
-  private CountDownLatch stopLatch = new CountDownLatch(1);
-
-  private Optional<StopperThread> stopAsync() {
+  private void stopAsync() {
     if (lifecycle.tryToMoveTo(Lifecycle.State.STOPPING)) {
+      LoggerFactory.getLogger(ProcessEntryPoint.class).info("Stopping process");
+      stopWatcher.stopWatching();
       long terminationTimeoutMs = Long.parseLong(props.nonNullValue(PROPERTY_GRACEFUL_STOP_TIMEOUT_MS));
-      stopperThread = new StopperThread(monitored, lifecycle, terminationTimeoutMs);
+      stopperThread = new StopperThread(monitored, this::terminate, terminationTimeoutMs);
       stopperThread.start();
-      stopWatcher.stopWatching();
-      return of(stopperThread);
     }
-    // stopperThread could already exist
-    return ofNullable(stopperThread);
   }
 
-  /**
-   * Blocks until stopped in a timely fashion (see {@link HardStopperThread})
-   */
-  void hardStop() {
-    hardStopAsync()
-      .ifPresent(stoppingThread -> {
-        try {
-          // join() does nothing if thread already finished
-          stoppingThread.join();
-        } catch (InterruptedException e) {
-          Thread.currentThread().interrupt();
-        }
-        commands.endWatch();
-        exit.exit(0);
-      });
-  }
-
-  private Optional<HardStopperThread> hardStopAsync() {
+  private void hardStopAsync() {
     if (lifecycle.tryToMoveTo(Lifecycle.State.HARD_STOPPING)) {
-      hardStopperThread = new HardStopperThread(monitored, lifecycle, HARD_STOP_TIMEOUT_MS, stopperThread);
-      hardStopperThread.start();
+      LoggerFactory.getLogger(ProcessEntryPoint.class).info("Hard stopping process");
+      if (stopperThread != null) {
+        stopperThread.stopIt();
+      }
       hardStopWatcher.stopWatching();
       stopWatcher.stopWatching();
-      return of(hardStopperThread);
+      new HardStopperThread(monitored, this::terminate).start();
     }
-    // hardStopperThread could already exist
-    return ofNullable(hardStopperThread);
+  }
+
+  private void terminate() {
+    lifecycle.tryToMoveTo(STOPPED);
+    commands.endWatch();
   }
 
   public static ProcessEntryPoint createForArguments(String[] args) {
@@ -236,15 +201,12 @@ public class ProcessEntryPoint {
    */
   private static class StopperThread extends AbstractStopperThread {
 
-    private StopperThread(Monitored monitored, Lifecycle lifecycle, long terminationTimeoutMs) {
+    private StopperThread(Monitored monitored, Runnable postAction, long terminationTimeoutMs) {
       super("Stopper", () -> {
-        if (!lifecycle.isCurrentState(STOPPED)) {
-          monitored.stop();
-          lifecycle.tryToMoveTo(STOPPED);
-        }
+        monitored.stop();
+        postAction.run();
       }, terminationTimeoutMs);
     }
-
   }
 
   /**
@@ -252,18 +214,12 @@ public class ProcessEntryPoint {
    */
   private static class HardStopperThread extends AbstractStopperThread {
 
-    private HardStopperThread(Monitored monitored, Lifecycle lifecycle, long terminationTimeoutMs, @Nullable StopperThread stopperThread) {
+    private HardStopperThread(Monitored monitored, Runnable postAction) {
       super(
-        "HardStopper",
-        () -> {
-          if (stopperThread != null) {
-            stopperThread.stopIt();
-          }
+        "HardStopper", () -> {
           monitored.hardStop();
-          lifecycle.tryToMoveTo(STOPPED);
-        },
-        terminationTimeoutMs);
+          postAction.run();
+        }, HARD_STOP_TIMEOUT_MS);
     }
-
   }
 }
diff --git a/server/sonar-process/src/test/java/org/sonar/process/ProcessEntryPointTest.java b/server/sonar-process/src/test/java/org/sonar/process/ProcessEntryPointTest.java
index 1fe1da2d147..43b7dd06118 100644
--- a/server/sonar-process/src/test/java/org/sonar/process/ProcessEntryPointTest.java
+++ b/server/sonar-process/src/test/java/org/sonar/process/ProcessEntryPointTest.java
@@ -24,23 +24,27 @@ import java.io.IOException;
 import java.util.Properties;
 import java.util.concurrent.atomic.AtomicBoolean;
 import org.apache.commons.io.FileUtils;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.DisableOnDebug;
 import org.junit.rules.TemporaryFolder;
 import org.junit.rules.TestRule;
 import org.junit.rules.Timeout;
+import org.mockito.ArgumentCaptor;
 import org.sonar.process.Lifecycle.State;
 import org.sonar.process.sharedmemoryfile.ProcessCommands;
 import org.sonar.process.test.StandardProcess;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.fail;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.sonar.process.ProcessEntryPoint.PROPERTY_GRACEFUL_STOP_TIMEOUT_MS;
 import static org.sonar.process.ProcessEntryPoint.PROPERTY_PROCESS_INDEX;
 import static org.sonar.process.ProcessEntryPoint.PROPERTY_PROCESS_KEY;
 import static org.sonar.process.ProcessEntryPoint.PROPERTY_SHARED_PATH;
-import static org.sonar.process.ProcessEntryPoint.PROPERTY_GRACEFUL_STOP_TIMEOUT_MS;
 
 public class ProcessEntryPointTest {
 
@@ -53,6 +57,12 @@ public class ProcessEntryPointTest {
   public TemporaryFolder temp = new TemporaryFolder();
 
   private ProcessCommands commands = new OperationalFlagOnlyProcessCommands();
+  private Runtime runtime;
+
+  @Before
+  public void setUp() {
+    runtime = mock(Runtime.class);
+  }
 
   @Test
   public void load_properties_from_file() throws Exception {
@@ -67,17 +77,15 @@ public class ProcessEntryPointTest {
   @Test
   public void test_initial_state() throws Exception {
     Props props = createProps();
-    ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands);
+    ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands, runtime);
 
     assertThat(entryPoint.getProps()).isSameAs(props);
-    assertThat(entryPoint.isStarted()).isFalse();
-    assertThat(entryPoint.isCurrentState(State.INIT)).isTrue();
   }
 
   @Test
   public void fail_to_launch_multiple_times() throws IOException {
     Props props = createProps();
-    ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands);
+    ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands, runtime);
 
     entryPoint.launch(new NoopProcess());
     try {
@@ -91,17 +99,10 @@ public class ProcessEntryPointTest {
   @Test
   public void launch_then_request_graceful_stop() throws Exception {
     Props props = createProps();
-    final ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands);
+    final ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands, runtime);
     final StandardProcess process = new StandardProcess();
 
-    Thread runner = new Thread() {
-      @Override
-      public void run() {
-        // starts and waits until terminated
-        entryPoint.launch(process);
-      }
-    };
-    runner.start();
+    new Thread(() -> entryPoint.launch(process)).start();
 
     waitForOperational(process, commands);
 
@@ -117,16 +118,11 @@ public class ProcessEntryPointTest {
   @Test
   public void launch_then_request_hard_stop() throws Exception {
     Props props = createProps();
-    final ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands);
+    final ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands, runtime);
     final StandardProcess process = new StandardProcess();
 
-    Thread runner = new Thread() {
-      @Override
-      public void run() {
-        // starts and waits until terminated
-        entryPoint.launch(process);
-      }
-    };
+    // starts and waits until terminated
+    Thread runner = new Thread(() -> entryPoint.launch(process));
     runner.start();
 
     waitForOperational(process, commands);
@@ -141,7 +137,7 @@ public class ProcessEntryPointTest {
   @Test
   public void terminate_if_unexpected_shutdown() throws Exception {
     Props props = createProps();
-    final ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands);
+    final ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands, runtime);
     final StandardProcess process = new StandardProcess();
 
     Thread runner = new Thread() {
@@ -156,10 +152,9 @@ public class ProcessEntryPointTest {
     waitForOperational(process, commands);
 
     // emulate signal to shutdown process
-    entryPoint.getShutdownHook().start();
-
-    // hack to prevent JUnit JVM to fail when executing the shutdown hook a second time
-    Runtime.getRuntime().removeShutdownHook(entryPoint.getShutdownHook());
+    ArgumentCaptor<Thread> shutdownHookCaptor = ArgumentCaptor.forClass(Thread.class);
+    verify(runtime).addShutdownHook(shutdownHookCaptor.capture());
+    shutdownHookCaptor.getValue().start();
 
     while (process.getState() != State.STOPPED) {
       Thread.sleep(10L);
@@ -170,11 +165,11 @@ public class ProcessEntryPointTest {
   @Test
   public void terminate_if_startup_error() throws IOException {
     Props props = createProps();
-    final ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands);
-    final Monitored process = new StartupErrorProcess();
+    final ProcessEntryPoint entryPoint = new ProcessEntryPoint(props, exit, commands, runtime);
+    final Monitored process = mock(Monitored.class);
+    doThrow(IllegalStateException.class).when(process).start();
 
     entryPoint.launch(process);
-    assertThat(entryPoint.isCurrentState(State.STOPPED)).isTrue();
   }
 
   private static void waitForOperational(StandardProcess process, ProcessCommands commands) throws InterruptedException {
@@ -220,34 +215,6 @@ public class ProcessEntryPointTest {
     }
   }
 
-  private static class StartupErrorProcess implements Monitored {
-
-    @Override
-    public void start() {
-      throw new IllegalStateException("ERROR");
-    }
-
-    @Override
-    public Status getStatus() {
-      return Status.DOWN;
-    }
-
-    @Override
-    public void awaitStop() {
-
-    }
-
-    @Override
-    public void stop() {
-
-    }
-
-    @Override
-    public void hardStop() {
-
-    }
-  }
-
   private static class OperationalFlagOnlyProcessCommands implements ProcessCommands {
     private final AtomicBoolean operational = new AtomicBoolean(false);
 
diff --git a/server/sonar-process/src/test/java/org/sonar/process/test/StandardProcess.java b/server/sonar-process/src/test/java/org/sonar/process/test/StandardProcess.java
index e360cd03605..768f49bbe8e 100644
--- a/server/sonar-process/src/test/java/org/sonar/process/test/StandardProcess.java
+++ b/server/sonar-process/src/test/java/org/sonar/process/test/StandardProcess.java
@@ -19,6 +19,7 @@
  */
 package org.sonar.process.test;
 
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicReference;
 import org.sonar.process.Lifecycle.State;
 import org.sonar.process.Monitored;
@@ -31,6 +32,7 @@ public class StandardProcess implements Monitored {
   private AtomicReference<State> state = new AtomicReference<>(State.INIT);
   private volatile boolean stopped = false;
   private volatile boolean hardStopped = false;
+  private CountDownLatch stopLatch = new CountDownLatch(1);
 
   private final Thread daemon = new Thread() {
     @Override
@@ -61,7 +63,7 @@ public class StandardProcess implements Monitored {
   @Override
   public void awaitStop() {
     try {
-      daemon.join();
+      stopLatch.await();
     } catch (InterruptedException e) {
       Thread.currentThread().interrupt();
     }
@@ -73,6 +75,7 @@ public class StandardProcess implements Monitored {
     daemon.interrupt();
     stopped = true;
     state.compareAndSet(State.STOPPING, State.STOPPED);
+    stopLatch.countDown();
   }
 
   /**
@@ -84,6 +87,7 @@ public class StandardProcess implements Monitored {
     daemon.interrupt();
     hardStopped = true;
     state.compareAndSet(State.HARD_STOPPING, State.STOPPED);
+    stopLatch.countDown();
   }
 
   public State getState() {