SONAR-12043 make Lifecycle threadsafe

and make ProcessEntryPointTest#launch_then_request_graceful_stop stable
and add trace log when some code tests the current state of Lifecyle (#getState() replaced by isCurrentState(State))

server/sonar-process/src/main/java/org/sonar/process/Lifecycle.java

@@ -25,6 +25,7 @@ import java.util.EnumMap;
 import java.util.EnumSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
 import javax.annotation.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -47,7 +48,7 @@ public class Lifecycle {
 
   private static final Map<State, Set<State>> TRANSITIONS = buildTransitions();
 
-  private State state = INIT;
+  private AtomicReference<State> state = new AtomicReference<>(INIT);
 
   private static Map<State, Set<State>> buildTransitions() {
     Map<State, Set<State>> res = new EnumMap<>(State.class);
@@ -72,19 +73,26 @@ public class Lifecycle {
     return EnumSet.copyOf(Arrays.asList(states));
   }
 
-  public State getState() {
-    return state;
+  public boolean isCurrentState(State candidateState) {
+    State currentState = this.state.get();
+    boolean res = currentState == candidateState;
+    LOG.trace("isCurrentState({}): {} ({})", candidateState, res, currentState);
+    return res;
   }
 
-  public synchronized boolean tryToMoveTo(State to) {
-    boolean res = false;
-    State currentState = state;
-    if (TRANSITIONS.get(currentState).contains(to)) {
-      this.state = to;
-      res = true;
-    }
-    LOG.trace("tryToMoveTo from {} to {} => {}", currentState, to, res);
-    return res;
+  public boolean tryToMoveTo(State to) {
+    AtomicReference<State> lastFrom = new AtomicReference<>();
+    State newState = this.state.updateAndGet(from -> {
+      lastFrom.set(from);
+      if (TRANSITIONS.get(from).contains(to)) {
+        return to;
+      }
+      return from;
+    });
+
+    boolean updated = newState == to && lastFrom.get() != to;
+    LOG.trace("tryToMoveTo from {} to {} => {}", lastFrom.get(), to, updated);
+    return updated;
   }
 
   @Override
@@ -96,11 +104,11 @@ public class Lifecycle {
       return false;
     }
     Lifecycle lifecycle = (Lifecycle) o;
-    return state == lifecycle.state;
+    return state.get() == lifecycle.state.get();
   }
 
   @Override
   public int hashCode() {
-    return state.hashCode();
+    return state.get().hashCode();
   }
 }

server/sonar-process/src/main/java/org/sonar/process/ProcessEntryPoint.java

@@ -29,6 +29,7 @@ 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 {
 
@@ -108,6 +109,7 @@ public class ProcessEntryPoint {
     } catch (Exception e) {
       logger.warn("Fail to start {}", getKey(), e);
     } finally {
+      logger.trace("Hard stopping to clean any resource...");
       hardStop();
     }
   }
@@ -133,6 +135,7 @@ public class ProcessEntryPoint {
         monitored.awaitStop();
       }
     } else {
+      logger.trace("Timeout waiting for process to go UP or OPERATIONAL. Hard stopping...");
       hardStop();
     }
   }
@@ -157,7 +160,7 @@ public class ProcessEntryPoint {
   }
 
   boolean isStarted() {
-    return lifecycle.getState() == Lifecycle.State.STARTED;
+    return lifecycle.isCurrentState(Lifecycle.State.STARTED);
   }
 
   void stop() {
@@ -166,7 +169,6 @@ public class ProcessEntryPoint {
         try {
           // join() does nothing if thread already finished
           stoppingThread.join();
-          lifecycle.tryToMoveTo(Lifecycle.State.STOPPED);
           commands.endWatch();
           exit.exit(0);
         } catch (InterruptedException e) {
@@ -179,7 +181,7 @@ public class ProcessEntryPoint {
   private Optional<StopperThread> stopAsync() {
     if (lifecycle.tryToMoveTo(Lifecycle.State.STOPPING)) {
       long terminationTimeoutMs = Long.parseLong(props.nonNullValue(PROPERTY_TERMINATION_TIMEOUT_MS));
-      stopperThread = new StopperThread(monitored, terminationTimeoutMs);
+      stopperThread = new StopperThread(monitored, lifecycle, terminationTimeoutMs);
       stopperThread.start();
       stopWatcher.stopWatching();
       return of(stopperThread);
@@ -197,7 +199,6 @@ public class ProcessEntryPoint {
         try {
           // join() does nothing if thread already finished
           stoppingThread.join();
-          lifecycle.tryToMoveTo(Lifecycle.State.STOPPED);
         } catch (InterruptedException e) {
           Thread.currentThread().interrupt();
         }
@@ -209,7 +210,7 @@ public class ProcessEntryPoint {
   private Optional<HardStopperThread> hardStopAsync() {
     if (lifecycle.tryToMoveTo(Lifecycle.State.HARD_STOPPING)) {
       long terminationTimeoutMs = Long.parseLong(props.nonNullValue(PROPERTY_TERMINATION_TIMEOUT_MS));
-      hardStopperThread = new HardStopperThread(monitored, terminationTimeoutMs, stopperThread);
+      hardStopperThread = new HardStopperThread(monitored, lifecycle, terminationTimeoutMs, stopperThread);
       hardStopperThread.start();
       hardStopWatcher.stopWatching();
       stopWatcher.stopWatching();
@@ -219,8 +220,8 @@ public class ProcessEntryPoint {
     return ofNullable(hardStopperThread);
   }
 
-  Lifecycle.State getState() {
-    return lifecycle.getState();
+  boolean isCurrentState(Lifecycle.State candidateState) {
+    return lifecycle.isCurrentState(candidateState);
   }
 
   Thread getShutdownHook() {
@@ -282,8 +283,13 @@ public class ProcessEntryPoint {
    */
   private static class StopperThread extends AbstractStopperThread {
 
-    private StopperThread(Monitored monitored, long terminationTimeoutMs) {
-      super("Stopper", monitored::stop, terminationTimeoutMs);
+    private StopperThread(Monitored monitored, Lifecycle lifecycle, long terminationTimeoutMs) {
+      super("Stopper", () -> {
+        if (!lifecycle.isCurrentState(STOPPED)) {
+          monitored.stop();
+          lifecycle.tryToMoveTo(STOPPED);
+        }
+      }, terminationTimeoutMs);
     }
 
   }
@@ -293,7 +299,7 @@ public class ProcessEntryPoint {
    */
   private static class HardStopperThread extends AbstractStopperThread {
 
-    private HardStopperThread(Monitored monitored, long terminationTimeoutMs, @Nullable StopperThread stopperThread) {
+    private HardStopperThread(Monitored monitored, Lifecycle lifecycle, long terminationTimeoutMs, @Nullable StopperThread stopperThread) {
       super(
         "HardStopper",
         () -> {
@@ -301,6 +307,7 @@ public class ProcessEntryPoint {
             stopperThread.stopIt();
           }
           monitored.hardStop();
+          lifecycle.tryToMoveTo(STOPPED);
         },
         terminationTimeoutMs);
     }

server/sonar-process/src/test/java/org/sonar/process/LifecycleTest.java

@@ -53,13 +53,13 @@ public class LifecycleTest {
   @Test
   public void try_to_move_does_not_support_jumping_states() {
     Lifecycle lifecycle = new Lifecycle();
-    assertThat(lifecycle.getState()).isEqualTo(INIT);
+    assertThat(lifecycle.isCurrentState(INIT)).isTrue();
 
     assertThat(lifecycle.tryToMoveTo(STARTED)).isFalse();
-    assertThat(lifecycle.getState()).isEqualTo(INIT);
+    assertThat(lifecycle.isCurrentState(INIT)).isTrue();
 
     assertThat(lifecycle.tryToMoveTo(STARTING)).isTrue();
-    assertThat(lifecycle.getState()).isEqualTo(STARTING);
+    assertThat(lifecycle.isCurrentState(STARTING)).isTrue();
   }
 
   @Test

server/sonar-process/src/test/java/org/sonar/process/ProcessEntryPointTest.java

@@ -22,6 +22,7 @@ package org.sonar.process;
 import java.io.File;
 import java.io.IOException;
 import java.util.Properties;
+import java.util.concurrent.atomic.AtomicBoolean;
 import org.apache.commons.io.FileUtils;
 import org.junit.Rule;
 import org.junit.Test;
@@ -51,7 +52,7 @@ public class ProcessEntryPointTest {
   @Rule
   public TemporaryFolder temp = new TemporaryFolder();
 
-  ProcessCommands commands = mock(ProcessCommands.class);
+  private ProcessCommands commands = new OperationalFlagOnlyProcessCommands();
 
   @Test
   public void load_properties_from_file() throws Exception {
@@ -70,7 +71,7 @@ public class ProcessEntryPointTest {
 
     assertThat(entryPoint.getProps()).isSameAs(props);
     assertThat(entryPoint.isStarted()).isFalse();
-    assertThat(entryPoint.getState()).isEqualTo(State.INIT);
+    assertThat(entryPoint.isCurrentState(State.INIT)).isTrue();
   }
 
   @Test
@@ -102,15 +103,14 @@ public class ProcessEntryPointTest {
     };
     runner.start();
 
-    while (process.getState() != State.STARTED) {
-      Thread.sleep(10L);
-    }
+    waitForOperational(process, commands);
 
     // requests for graceful stop -> waits until down
     // Should terminate before the timeout of 30s
     entryPoint.stop();
 
     assertThat(process.getState()).isEqualTo(State.STOPPED);
+    assertThat(process.wasStopped()).isEqualTo(true);
     assertThat(process.wasHardStopped()).isEqualTo(false);
   }
 
@@ -129,9 +129,7 @@ public class ProcessEntryPointTest {
     };
     runner.start();
 
-    while (process.getState() != State.STARTED) {
-      Thread.sleep(10L);
-    }
+    waitForOperational(process, commands);
 
     // requests for stop hardly waiting
     entryPoint.hardStop();
@@ -154,9 +152,8 @@ public class ProcessEntryPointTest {
       }
     };
     runner.start();
-    while (process.getState() != State.STARTED) {
-      Thread.sleep(10L);
-    }
+
+    waitForOperational(process, commands);
 
     // emulate signal to shutdown process
     entryPoint.getShutdownHook().start();
@@ -177,7 +174,13 @@ public class ProcessEntryPointTest {
     final Monitored process = new StartupErrorProcess();
 
     entryPoint.launch(process);
-    assertThat(entryPoint.getState()).isEqualTo(State.STOPPED);
+    assertThat(entryPoint.isCurrentState(State.STOPPED)).isTrue();
+  }
+
+  private static void waitForOperational(StandardProcess process, ProcessCommands commands) throws InterruptedException {
+    while (!(process.getState() == State.STARTED && commands.isOperational())) {
+      Thread.sleep(10L);
+    }
   }
 
   private Props createProps() throws IOException {
@@ -244,4 +247,88 @@ public class ProcessEntryPointTest {
 
     }
   }
+
+  private static class OperationalFlagOnlyProcessCommands implements ProcessCommands {
+    private final AtomicBoolean operational = new AtomicBoolean(false);
+
+    @Override
+    public boolean isUp() {
+      return false;
+    }
+
+    @Override
+    public void setUp() {
+
+    }
+
+    @Override
+    public boolean isOperational() {
+      return operational.get();
+    }
+
+    @Override
+    public void setOperational() {
+      operational.set(true);
+    }
+
+    @Override
+    public void ping() {
+
+    }
+
+    @Override
+    public long getLastPing() {
+      return 0;
+    }
+
+    @Override
+    public void setHttpUrl(String s) {
+
+    }
+
+    @Override
+    public String getHttpUrl() {
+      return null;
+    }
+
+    @Override
+    public void askForStop() {
+
+    }
+
+    @Override
+    public boolean askedForStop() {
+      return false;
+    }
+
+    @Override
+    public void askForHardStop() {
+
+    }
+
+    @Override
+    public boolean askedForHardStop() {
+      return false;
+    }
+
+    @Override
+    public void askForRestart() {
+
+    }
+
+    @Override
+    public boolean askedForRestart() {
+      return false;
+    }
+
+    @Override
+    public void acknowledgeAskForRestart() {
+
+    }
+
+    @Override
+    public void endWatch() {
+
+    }
+  }
 }

server/sonar-process/src/test/java/org/sonar/process/test/StandardProcess.java

@@ -19,14 +19,18 @@
  */
 package org.sonar.process.test;
 
+import java.util.concurrent.atomic.AtomicReference;
+import org.sonar.process.Lifecycle.State;
 import org.sonar.process.Monitored;
 import org.sonar.process.ProcessEntryPoint;
-import org.sonar.process.Lifecycle.State;
+
+import static com.google.common.base.Preconditions.checkState;
 
 public class StandardProcess implements Monitored {
 
-  private State state = State.INIT;
-  private boolean hardStopped = false;
+  private AtomicReference<State> state = new AtomicReference<>(State.INIT);
+  private volatile boolean stopped = false;
+  private volatile boolean hardStopped = false;
 
   private final Thread daemon = new Thread() {
     @Override
@@ -36,7 +40,7 @@ public class StandardProcess implements Monitored {
           Thread.sleep(100L);
         }
       } catch (InterruptedException e) {
-        // return
+        Thread.currentThread().interrupt();
       }
     }
   };
@@ -46,14 +50,14 @@ public class StandardProcess implements Monitored {
    */
   @Override
   public void start() {
-    state = State.STARTING;
+    state.compareAndSet(State.INIT, State.STARTING);
     daemon.start();
-    state = State.STARTED;
+    state.compareAndSet(State.STARTING, State.STARTED);
   }
 
   @Override
   public Status getStatus() {
-    return state == State.STARTED ? Status.OPERATIONAL : Status.DOWN;
+    return state.get() == State.STARTED ? Status.OPERATIONAL : Status.DOWN;
   }
 
   @Override
@@ -61,15 +65,16 @@ public class StandardProcess implements Monitored {
     try {
       daemon.join();
     } catch (InterruptedException e) {
-      // interrupted by call to terminate()
+      Thread.currentThread().interrupt();
     }
   }
 
   @Override
   public void stop() {
-    state = State.STOPPING;
+    checkState(state.compareAndSet(State.STARTED, State.STOPPING), "not started?!! what?");
     daemon.interrupt();
-    state = State.STOPPED;
+    stopped = true;
+    state.compareAndSet(State.STOPPING, State.STOPPED);
   }
 
   /**
@@ -77,14 +82,18 @@ public class StandardProcess implements Monitored {
    */
   @Override
   public void hardStop() {
-    state = State.HARD_STOPPING;
+    state.set(State.HARD_STOPPING);
     daemon.interrupt();
     hardStopped = true;
-    state = State.STOPPED;
+    state.compareAndSet(State.HARD_STOPPING, State.STOPPED);
   }
 
   public State getState() {
-    return state;
+    return state.get();
+  }
+
+  public boolean wasStopped() {
+    return stopped;
   }
 
   public boolean wasHardStopped() {