From 31498511d9862f7b40ba6fcc2bbd94c9a0383850 Mon Sep 17 00:00:00 2001 From: Sébastien Lesaint Date: Thu, 5 Sep 2019 12:52:44 +0200 Subject: protect ManagedProcessHandler stop finalization code with lifecycle state since this code is interrupting other threads, this ensures concurrent threads attempting to run this code will never interrupt each other leading to unpredictable and unreproducible behavior --- .../application/process/ManagedProcessHandler.java | 7 +- .../process/ManagedProcessLifecycle.java | 18 ++-- .../process/ManagedProcessLifecycleTest.java | 118 +++++++++++++++------ 3 files changed, 99 insertions(+), 44 deletions(-) diff --git a/server/sonar-main/src/main/java/org/sonar/application/process/ManagedProcessHandler.java b/server/sonar-main/src/main/java/org/sonar/application/process/ManagedProcessHandler.java index 508c73b5e0b..14da55a2ef8 100644 --- a/server/sonar-main/src/main/java/org/sonar/application/process/ManagedProcessHandler.java +++ b/server/sonar-main/src/main/java/org/sonar/application/process/ManagedProcessHandler.java @@ -73,7 +73,8 @@ public class ManagedProcessHandler { this.process = commandLauncher.get(); } catch (RuntimeException e) { LOG.error("Fail to launch process [{}]", processId.getKey(), e); - lifecycle.tryToMoveTo(ManagedProcessLifecycle.State.STOPPED); + lifecycle.tryToMoveTo(ManagedProcessLifecycle.State.STOPPING); + finalizeStop(); throw e; } this.stdOutGobbler = new StreamGobbler(process.getInputStream(), processId.getKey()); @@ -179,6 +180,10 @@ public class ManagedProcessHandler { } private void finalizeStop() { + if (!lifecycle.tryToMoveTo(ManagedProcessLifecycle.State.FINALIZE_STOPPING)) { + return; + } + interrupt(eventWatcher); interrupt(stopWatcher); if (process != null) { diff --git a/server/sonar-main/src/main/java/org/sonar/application/process/ManagedProcessLifecycle.java b/server/sonar-main/src/main/java/org/sonar/application/process/ManagedProcessLifecycle.java index ef4f6d07d0c..7583ddb1450 100644 --- a/server/sonar-main/src/main/java/org/sonar/application/process/ManagedProcessLifecycle.java +++ b/server/sonar-main/src/main/java/org/sonar/application/process/ManagedProcessLifecycle.java @@ -30,6 +30,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.sonar.process.ProcessId; +import static org.sonar.application.process.ManagedProcessLifecycle.State.FINALIZE_STOPPING; import static org.sonar.application.process.ManagedProcessLifecycle.State.HARD_STOPPING; import static org.sonar.application.process.ManagedProcessLifecycle.State.INIT; import static org.sonar.application.process.ManagedProcessLifecycle.State.STARTED; @@ -40,7 +41,7 @@ import static org.sonar.application.process.ManagedProcessLifecycle.State.STOPPI public class ManagedProcessLifecycle { public enum State { - INIT, STARTING, STARTED, STOPPING, HARD_STOPPING, STOPPED + INIT, STARTING, STARTED, STOPPING, HARD_STOPPING, FINALIZE_STOPPING, STOPPED } private static final Logger LOG = LoggerFactory.getLogger(ManagedProcessLifecycle.class); @@ -51,22 +52,19 @@ public class ManagedProcessLifecycle { private State state; public ManagedProcessLifecycle(ProcessId processId, List listeners) { - this(processId, listeners, INIT); - } - - ManagedProcessLifecycle(ProcessId processId, List listeners, State initialState) { this.processId = processId; this.listeners = listeners; - this.state = initialState; + this.state = INIT; } private static Map> buildTransitions() { Map> res = new EnumMap<>(State.class); res.put(INIT, toSet(STARTING)); - res.put(STARTING, toSet(STARTED, STOPPING, HARD_STOPPING, STOPPED)); - res.put(STARTED, toSet(STOPPING, HARD_STOPPING, STOPPED)); - res.put(STOPPING, toSet(HARD_STOPPING, STOPPED)); - res.put(HARD_STOPPING, toSet(STOPPED)); + res.put(STARTING, toSet(STARTED, STOPPING, HARD_STOPPING)); + res.put(STARTED, toSet(STOPPING, HARD_STOPPING)); + res.put(STOPPING, toSet(HARD_STOPPING, FINALIZE_STOPPING)); + res.put(HARD_STOPPING, toSet(FINALIZE_STOPPING)); + res.put(FINALIZE_STOPPING, toSet(STOPPED)); res.put(STOPPED, toSet()); return Collections.unmodifiableMap(res); } diff --git a/server/sonar-main/src/test/java/org/sonar/application/process/ManagedProcessLifecycleTest.java b/server/sonar-main/src/test/java/org/sonar/application/process/ManagedProcessLifecycleTest.java index a2e6ff0fb9b..ade75608b69 100644 --- a/server/sonar-main/src/test/java/org/sonar/application/process/ManagedProcessLifecycleTest.java +++ b/server/sonar-main/src/test/java/org/sonar/application/process/ManagedProcessLifecycleTest.java @@ -19,29 +19,45 @@ */ package org.sonar.application.process; +import com.tngtech.java.junit.dataprovider.DataProvider; +import com.tngtech.java.junit.dataprovider.DataProviderRunner; +import com.tngtech.java.junit.dataprovider.UseDataProvider; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.List; +import java.util.function.Predicate; import org.junit.Test; +import org.junit.runner.RunWith; import org.sonar.process.ProcessId; import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; import static org.assertj.core.api.Assertions.assertThat; +import static org.sonar.application.process.ManagedProcessLifecycle.State; +import static org.sonar.application.process.ManagedProcessLifecycle.State.FINALIZE_STOPPING; +import static org.sonar.application.process.ManagedProcessLifecycle.State.HARD_STOPPING; import static org.sonar.application.process.ManagedProcessLifecycle.State.INIT; import static org.sonar.application.process.ManagedProcessLifecycle.State.STARTED; import static org.sonar.application.process.ManagedProcessLifecycle.State.STARTING; -import static org.sonar.application.process.ManagedProcessLifecycle.State.HARD_STOPPING; +import static org.sonar.application.process.ManagedProcessLifecycle.State.STOPPED; import static org.sonar.application.process.ManagedProcessLifecycle.State.STOPPING; +@RunWith(DataProviderRunner.class) public class ManagedProcessLifecycleTest { @Test public void initial_state_is_INIT() { - ManagedProcessLifecycle lifecycle = new ManagedProcessLifecycle(ProcessId.ELASTICSEARCH, Collections.emptyList()); + ManagedProcessLifecycle lifecycle = new ManagedProcessLifecycle(ProcessId.ELASTICSEARCH, emptyList()); assertThat(lifecycle.getState()).isEqualTo(INIT); } + @Test + public void listeners_are_not_notified_of_INIT_state() { + TestLifeCycleListener listener = new TestLifeCycleListener(); + new ManagedProcessLifecycle(ProcessId.ELASTICSEARCH, Arrays.asList(listener)); + assertThat(listener.states).isEmpty(); + } + @Test public void try_to_move_does_not_support_jumping_states() { TestLifeCycleListener listener = new TestLifeCycleListener(); @@ -59,51 +75,87 @@ public class ManagedProcessLifecycleTest { } @Test - public void no_state_can_not_move_to_itself() { - for (ManagedProcessLifecycle.State state : ManagedProcessLifecycle.State.values()) { - assertThat(newLifeCycle(state).tryToMoveTo(state)).isFalse(); - } + @UseDataProvider("allStates") + public void no_state_can_not_move_to_itself(State state) { + assertThat(newLifeCycle(state).tryToMoveTo(state)).isFalse(); + } + + @Test + @UseDataProvider("allStates") + public void can_move_to_HARD_STOPPING_from_STARTING_AND_STARTED_and_STOPPING_only(State state) { + verifyCanMoveTo(t -> t == STARTING || t == STARTED || t == STOPPING, + state, HARD_STOPPING); } @Test - public void can_move_to_STOPPING_from_STARTING_STARTED_and_STOPPING_only() { - for (ManagedProcessLifecycle.State state : ManagedProcessLifecycle.State.values()) { - TestLifeCycleListener listener = new TestLifeCycleListener(); - boolean tryToMoveTo = newLifeCycle(state, listener).tryToMoveTo(HARD_STOPPING); - if (state == STARTING || state == STARTED || state == STOPPING) { - assertThat(tryToMoveTo).as("from state " + state).isTrue(); - assertThat(listener.states).containsOnly(HARD_STOPPING); - } else { - assertThat(tryToMoveTo).as("from state " + state).isFalse(); - assertThat(listener.states).isEmpty(); - } + @UseDataProvider("allStates") + public void can_move_to_STARTED_from_STARTING_only(State state) { + verifyCanMoveTo(t -> t == STARTING, state, STARTED); + } + + private void verifyCanMoveTo(Predicate isAllowed, State from, State to) { + TestLifeCycleListener listener = new TestLifeCycleListener(); + ManagedProcessLifecycle underTest = newLifeCycle(from, listener); + boolean tryToMoveTo = underTest.tryToMoveTo(to); + if (isAllowed.test(from)) { + assertThat(tryToMoveTo).isTrue(); + } else { + assertThat(tryToMoveTo).isFalse(); } } @Test - public void can_move_to_STARTED_from_STARTING_only() { - for (ManagedProcessLifecycle.State state : ManagedProcessLifecycle.State.values()) { - TestLifeCycleListener listener = new TestLifeCycleListener(); - boolean tryToMoveTo = newLifeCycle(state, listener).tryToMoveTo(STARTED); - if (state == STARTING) { - assertThat(tryToMoveTo).as("from state " + state).isTrue(); - assertThat(listener.states).containsOnly(STARTED); - } else { - assertThat(tryToMoveTo).as("from state " + state).isFalse(); - assertThat(listener.states).isEmpty(); - } + @UseDataProvider("allStates") + public void cannot_move_to_any_state_from_STOPPED(State state) { + assertThat(newLifeCycle(STOPPED).tryToMoveTo(state)).isFalse(); + } + + private static ManagedProcessLifecycle newLifeCycle(State from, State to, TestLifeCycleListener... listeners) { + ManagedProcessLifecycle lifecycle = newLifeCycle(from, listeners); + assertThat(lifecycle.tryToMoveTo(to)).isTrue(); + assertThat(lifecycle.getState()).isEqualTo(to); + Arrays.stream(listeners) + .forEach(t -> assertThat(t.states).endsWith(to)); + return lifecycle; + } + + /** + * Creates a Lifecycle object in the specified state emulating that the shortest path to this state has been followed + * to reach it. + */ + private static ManagedProcessLifecycle newLifeCycle(State state, TestLifeCycleListener... listeners) { + switch (state) { + case INIT: + return new ManagedProcessLifecycle(ProcessId.ELASTICSEARCH, asList(listeners)); + case STARTING: + return newLifeCycle(INIT, state, listeners); + case STARTED: + return newLifeCycle(STARTING, state, listeners); + case STOPPING: + return newLifeCycle(STARTED, state, listeners); + case HARD_STOPPING: + return newLifeCycle(STARTED, state, listeners); + case FINALIZE_STOPPING: + return newLifeCycle(STOPPING, state, listeners); + case STOPPED: + return newLifeCycle(FINALIZE_STOPPING, state, listeners); + default: + throw new IllegalStateException("new state is not supported:" + state); } } - private static ManagedProcessLifecycle newLifeCycle(ManagedProcessLifecycle.State state, TestLifeCycleListener... listeners) { - return new ManagedProcessLifecycle(ProcessId.ELASTICSEARCH, Arrays.asList(listeners), state); + @DataProvider + public static Object[][] allStates() { + return Arrays.stream(State.values()) + .map(t -> new Object[] {t}) + .toArray(Object[][]::new); } private static final class TestLifeCycleListener implements ProcessLifecycleListener { - private final List states = new ArrayList<>(); + private final List states = new ArrayList<>(); @Override - public void onProcessState(ProcessId processId, ManagedProcessLifecycle.State state) { + public void onProcessState(ProcessId processId, State state) { this.states.add(state); } } -- cgit v1.2.3