From 8292bbff003517890cf2d645d5ee1cda0b63ad84 Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Fri, 10 Jul 2015 17:24:56 +0100 Subject: [PATCH] Fixing initial state transitions - Reported and fixex #81 - Change how internal state is entered and handled when transition doesn't terminate into initial state. - Needed to change stuff in various places because some of the concepts were literally broken which were overlooked due to missing tests. --- .../config/AbstractStateMachineFactory.java | 7 +- .../statemachine/state/StateMachineState.java | 70 ++++++++++--------- .../support/AbstractStateMachine.java | 46 ++++++++---- .../statemachine/SubStateMachineTests.java | 18 ++--- .../transition/TransitionTests.java | 6 +- .../java/demo/showcase/ShowcaseTests.java | 70 +++++++++++++++---- 6 files changed, 146 insertions(+), 71 deletions(-) diff --git a/spring-statemachine-core/src/main/java/org/springframework/statemachine/config/AbstractStateMachineFactory.java b/spring-statemachine-core/src/main/java/org/springframework/statemachine/config/AbstractStateMachineFactory.java index 46f1a0e0..c8f1de10 100644 --- a/spring-statemachine-core/src/main/java/org/springframework/statemachine/config/AbstractStateMachineFactory.java +++ b/spring-statemachine-core/src/main/java/org/springframework/statemachine/config/AbstractStateMachineFactory.java @@ -348,9 +348,12 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS continue; } if (stateMachine != null) { + PseudoState pseudoState = null; + if (stateData.isInitial()) { + pseudoState = new DefaultPseudoState(PseudoStateKind.INITIAL); + } state = new StateMachineState(stateData.getState(), stateMachine, stateData.getDeferred(), - stateData.getEntryActions(), stateData.getExitActions(), new DefaultPseudoState( - PseudoStateKind.INITIAL)); + stateData.getEntryActions(), stateData.getExitActions(), pseudoState); // TODO: below if/else doesn't feel right if (stateDatas.size() > 1 && stateData.isInitial()) { initialState = state; diff --git a/spring-statemachine-core/src/main/java/org/springframework/statemachine/state/StateMachineState.java b/spring-statemachine-core/src/main/java/org/springframework/statemachine/state/StateMachineState.java index d06e0517..1441852c 100644 --- a/spring-statemachine-core/src/main/java/org/springframework/statemachine/state/StateMachineState.java +++ b/spring-statemachine-core/src/main/java/org/springframework/statemachine/state/StateMachineState.java @@ -162,44 +162,48 @@ public class StateMachineState extends AbstractState { } } - if (getPseudoState() != null && getPseudoState().getKind() == PseudoStateKind.INITIAL) { - // disable initial state if it looks like we're about - // to transit directory into a non initial state - // we do transit via initial state if we're returning - // via history state - boolean initialEnabled = true; - if (context.getTransition() != null) { - State target = context.getTransition().getTarget(); - PseudoStateKind kind = target.getPseudoState() != null ? target.getPseudoState().getKind() : null; - State findDeepParent = findDeepParent(getSubmachine().getStates(), target); - if (findDeepParent != null && findDeepParent.isSubmachineState()) { - ((StateMachineState) findDeepParent).getSubmachine().getStateMachineAccessor() - .doWithRegion(new StateMachineFunction>() { + if (context.getTransition() != null) { + State target = context.getTransition().getTarget(); + State immediateDeepParent = findDeepParent(getSubmachine().getStates(), target); - @Override - public void apply(StateMachineAccess function) { - function.setInitialEnabled(false); - } - }); - } - if (getSubmachine().getStates().contains(target) && kind != PseudoStateKind.HISTORY_SHALLOW - && kind != PseudoStateKind.HISTORY_DEEP) { - initialEnabled = false; - } + // disable initial state where needed + if (immediateDeepParent != null && immediateDeepParent.isSubmachineState() && ( !isInitial(target) ) ) { + + ((StateMachineState) immediateDeepParent).getSubmachine().getStateMachineAccessor() + .doWithRegion(new StateMachineFunction>() { + + @Override + public void apply(StateMachineAccess function) { + function.setInitialEnabled(false); + } + }); } - // need final for state machine access - final boolean enabled = initialEnabled; - getSubmachine().getStateMachineAccessor().doWithRegion( - new StateMachineFunction>() { + if (immediateDeepParent != null && !isInitial(immediateDeepParent)) { + getSubmachine().getStateMachineAccessor().doWithRegion( + new StateMachineFunction>() { - @Override - public void apply(StateMachineAccess function) { - function.setInitialEnabled(enabled); - } - }); - getSubmachine().start(); + @Override + public void apply(StateMachineAccess function) { + function.setInitialEnabled(false); + } + }); + } else if (immediateDeepParent != null && isInitial(immediateDeepParent) && isInitial(target)) { + ((StateMachineState) immediateDeepParent).getSubmachine().getStateMachineAccessor().doWithRegion( + new StateMachineFunction>() { + + @Override + public void apply(StateMachineAccess function) { + function.setInitialEnabled(false); + } + }); + } } + getSubmachine().start(); + } + + private boolean isInitial(State state) { + return state.getPseudoState() != null && state.getPseudoState().getKind() == PseudoStateKind.INITIAL; } private State findDeepParent(Collection> states, State state) { diff --git a/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java b/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java index e61da32f..84ec2b7c 100644 --- a/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java +++ b/spring-statemachine-core/src/main/java/org/springframework/statemachine/support/AbstractStateMachine.java @@ -265,6 +265,9 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo protected void doStart() { // if state is set assume nothing to do if (currentState != null) { + if (log.isDebugEnabled()) { + log.debug("State already set, disabling initial"); + } stateMachineExecutor.setInitialEnabled(false); stateMachineExecutor.start(); return; @@ -272,6 +275,9 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo registerPseudoStateListener(); if (initialEnabled != null && !initialEnabled) { + if (log.isDebugEnabled()) { + log.debug("Initial disable asked, disabling initial"); + } stateMachineExecutor.setInitialEnabled(false); } @@ -326,9 +332,7 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo @Override public void setInitialEnabled(boolean enabled) { - if (initialEnabled == null) { - initialEnabled = enabled; - } + initialEnabled = enabled; } @SuppressWarnings("unchecked") @@ -554,8 +558,13 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo if (isTargetSubOf && currentState == transition.getTarget()) { state = transition.getSource(); } +// else if (currentState == null && StateMachineUtils.isSubstate(findDeep, state)) { +// state = findDeep; +// } } + boolean nonDeepStatePresent = false; + if (states.contains(state)) { if (exit) { exitCurrentState(state, message, transition, stateMachine); @@ -567,7 +576,21 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo } entryToState(state, message, transition, stateMachine); notifyStateChanged(notifyFrom, state); - } else if (currentState != null) { + nonDeepStatePresent = true; + } else if (currentState == null && StateMachineUtils.isSubstate(findDeep, state)) { + if (exit) { + exitCurrentState(findDeep, message, transition, stateMachine); + } + State notifyFrom = currentState; + currentState = findDeep; + if (!isRunning()) { + start(); + } + entryToState(findDeep, message, transition, stateMachine); + notifyStateChanged(notifyFrom, findDeep); + } + + if (currentState != null && !nonDeepStatePresent) { if (findDeep != null) { if (exit) { exitCurrentState(state, message, transition, stateMachine); @@ -603,9 +626,12 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo } } } - boolean shouldEntry = findDeep != currentState; + boolean shouldTryEntry = findDeep != currentState; + if (!shouldTryEntry && (transition.getSource() == currentState && StateMachineUtils.isSubstate(currentState, transition.getTarget()))) { + shouldTryEntry = true; + } currentState = findDeep; - if (shouldEntry) { + if (shouldTryEntry) { entryToState(currentState, message, transition, stateMachine); } @@ -654,7 +680,6 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo State findDeep = findDeepParent(transition.getTarget()); boolean isTargetSubOfOtherState = findDeep != null && findDeep != currentState; - boolean isTargetSubOfSource = StateMachineUtils.isSubstate(transition.getSource(), transition.getTarget()); boolean isSubOfSource = StateMachineUtils.isSubstate(transition.getSource(), currentState); boolean isSubOfTarget = StateMachineUtils.isSubstate(transition.getTarget(), currentState); @@ -666,14 +691,11 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo } else if (!isSubOfSource && !isSubOfTarget && currentState == transition.getTarget()) { } else if (isTargetSubOfOtherState) { } else if (!isSubOfSource && !isSubOfTarget && findDeep == null) { + } else if (!isSubOfSource && !isSubOfTarget && (transition.getSource() == currentState && StateMachineUtils.isSubstate(currentState, transition.getTarget()))) { } else if (!isSubOfSource && !isSubOfTarget) { return; } - if (transition.getSource() == currentState && isTargetSubOfSource) { - return; - } - } log.debug("Exit state=[" + state + "]"); @@ -700,7 +722,7 @@ public abstract class AbstractStateMachine extends StateMachineObjectSuppo } else if (isComingFromOtherSubmachine) { } else if (!isSubOfSource && !isSubOfTarget && findDeep2 == null) { } else if (isSubOfSource && !isSubOfTarget && currentState == transition.getTarget()) { - return; + } else if (!isSubOfSource && !isSubOfTarget && (transition.getSource() == currentState && StateMachineUtils.isSubstate(currentState, transition.getTarget()))) { } else if (!isSubOfSource && !isSubOfTarget) { return; } diff --git a/spring-statemachine-core/src/test/java/org/springframework/statemachine/SubStateMachineTests.java b/spring-statemachine-core/src/test/java/org/springframework/statemachine/SubStateMachineTests.java index 7b0adfee..b71f9056 100644 --- a/spring-statemachine-core/src/test/java/org/springframework/statemachine/SubStateMachineTests.java +++ b/spring-statemachine-core/src/test/java/org/springframework/statemachine/SubStateMachineTests.java @@ -146,9 +146,9 @@ public class SubStateMachineTests extends AbstractStateMachineTests { assertThat(entryActionS1.onExecuteLatch.await(1, TimeUnit.SECONDS), is(true)); assertThat(exitActionS1.onExecuteLatch.await(1, TimeUnit.SECONDS), is(true)); - assertThat(entryActionS11.stateContexts.size(), is(1)); - assertThat(exitActionS11.stateContexts.size(), is(1)); - assertThat(entryActionS11.stateContexts.size(), is(1)); + assertThat(entryActionS111.stateContexts.size(), is(2)); + assertThat(exitActionS111.stateContexts.size(), is(1)); + assertThat(entryActionS11.stateContexts.size(), is(2)); assertThat(exitActionS11.stateContexts.size(), is(1)); assertThat(entryActionS1.stateContexts.size(), is(1)); assertThat(exitActionS1.stateContexts.size(), is(1)); @@ -336,9 +336,9 @@ public class SubStateMachineTests extends AbstractStateMachineTests { assertThat(entryActionS1.onExecuteLatch.await(1, TimeUnit.SECONDS), is(true)); assertThat(exitActionS1.onExecuteLatch.await(1, TimeUnit.SECONDS), is(false)); - assertThat(entryActionS11.stateContexts.size(), is(1)); - assertThat(exitActionS11.stateContexts.size(), is(1)); - assertThat(entryActionS11.stateContexts.size(), is(1)); + assertThat(entryActionS111.stateContexts.size(), is(2)); + assertThat(exitActionS111.stateContexts.size(), is(1)); + assertThat(entryActionS11.stateContexts.size(), is(2)); assertThat(exitActionS11.stateContexts.size(), is(1)); assertThat(entryActionS1.stateContexts.size(), is(1)); assertThat(exitActionS1.stateContexts.size(), is(0)); @@ -371,9 +371,9 @@ public class SubStateMachineTests extends AbstractStateMachineTests { assertThat(entryActionS1.onExecuteLatch.await(1, TimeUnit.SECONDS), is(true)); assertThat(exitActionS1.onExecuteLatch.await(1, TimeUnit.SECONDS), is(true)); - assertThat(entryActionS11.stateContexts.size(), is(1)); - assertThat(exitActionS11.stateContexts.size(), is(1)); - assertThat(entryActionS11.stateContexts.size(), is(1)); + assertThat(entryActionS111.stateContexts.size(), is(2)); + assertThat(exitActionS111.stateContexts.size(), is(1)); + assertThat(entryActionS11.stateContexts.size(), is(2)); assertThat(exitActionS11.stateContexts.size(), is(1)); assertThat(entryActionS1.stateContexts.size(), is(1)); assertThat(exitActionS1.stateContexts.size(), is(1)); diff --git a/spring-statemachine-core/src/test/java/org/springframework/statemachine/transition/TransitionTests.java b/spring-statemachine-core/src/test/java/org/springframework/statemachine/transition/TransitionTests.java index a892ca37..19277142 100644 --- a/spring-statemachine-core/src/test/java/org/springframework/statemachine/transition/TransitionTests.java +++ b/spring-statemachine-core/src/test/java/org/springframework/statemachine/transition/TransitionTests.java @@ -200,7 +200,7 @@ public class TransitionTests extends AbstractStateMachineTests { listener.reset(0, 2); machine.sendEvent(TestEvents2.PAUSE); assertThat(listener.stateEnteredLatch.await(2, TimeUnit.SECONDS), is(true)); - assertThat(listener.stateEnteredCount, is(2)); + assertThat(listener.stateEnteredCount, is(3)); assertThat(machine.getState().getIds(), contains(TestStates2.BUSY, TestStates2.PAUSED)); } @@ -221,10 +221,10 @@ public class TransitionTests extends AbstractStateMachineTests { assertThat(listener.stateChangedCount, is(2)); assertThat(machine.getState().getIds(), contains(TestStates2.IDLE, TestStates2.CLOSED)); - listener.reset(0, 4); + listener.reset(0, 3); machine.sendEvent(TestEvents2.PAUSE); assertThat(listener.stateEnteredLatch.await(2, TimeUnit.SECONDS), is(true)); - assertThat(listener.stateEnteredCount, is(4)); + assertThat(listener.stateEnteredCount, is(3)); assertThat(machine.getState().getIds(), contains(TestStates2.BUSY, TestStates2.PAUSED, TestStates2.PAUSED2)); } diff --git a/spring-statemachine-samples/showcase/src/test/java/demo/showcase/ShowcaseTests.java b/spring-statemachine-samples/showcase/src/test/java/demo/showcase/ShowcaseTests.java index c24c1833..a346a897 100644 --- a/spring-statemachine-samples/showcase/src/test/java/demo/showcase/ShowcaseTests.java +++ b/spring-statemachine-samples/showcase/src/test/java/demo/showcase/ShowcaseTests.java @@ -52,8 +52,8 @@ public class ShowcaseTests { @Test public void testInitialState() throws Exception { - listener.stateChangedLatch.await(1, TimeUnit.SECONDS); - listener.stateEnteredLatch.await(1, TimeUnit.SECONDS); + assertThat(listener.stateChangedLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateEnteredLatch.await(1, TimeUnit.SECONDS), is(true)); assertThat(machine.getState().getIds(), contains(States.S0, States.S1, States.S11)); assertThat(listener.statesEntered.size(), is(3)); assertThat(listener.statesEntered.get(0).getId(), is(States.S0)); @@ -64,14 +64,30 @@ public class ShowcaseTests { @Test public void testA() throws Exception { - listener.reset(1, 0, 0); + testInitialState(); + listener.reset(1, 2, 2); machine.sendEvent(Events.A); - listener.stateChangedLatch.await(1, TimeUnit.SECONDS); + // variable foo is 0, guard denies transition + assertThat(listener.stateChangedLatch.await(1, TimeUnit.SECONDS), is(false)); + assertThat(listener.stateEnteredLatch.await(1, TimeUnit.SECONDS), is(false)); + assertThat(listener.stateExitedLatch.await(1, TimeUnit.SECONDS), is(false)); + assertThat(machine.getState().getIds(), contains(States.S0, States.S1, States.S11)); + } + + @Test + public void testB() throws Exception { + testInitialState(); + listener.reset(1, 2, 2); + machine.sendEvent(Events.B); + assertThat(listener.stateChangedLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateEnteredLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateExitedLatch.await(1, TimeUnit.SECONDS), is(true)); assertThat(machine.getState().getIds(), contains(States.S0, States.S1, States.S11)); } @Test public void testCHCA() throws Exception { + testInitialState(); listener.reset(3, 0, 0); machine.sendEvent(Events.C); machine.sendEvent(Events.H); @@ -96,6 +112,7 @@ public class ShowcaseTests { @Test public void testC() throws Exception { + testInitialState(); listener.reset(1, 3, 0); machine.sendEvent(Events.C); listener.stateChangedLatch.await(1, TimeUnit.SECONDS); @@ -109,6 +126,7 @@ public class ShowcaseTests { @Test public void testCC() throws Exception { + testInitialState(); listener.reset(1, 3, 0); machine.sendEvent(Events.C); listener.stateChangedLatch.await(1, TimeUnit.SECONDS); @@ -125,6 +143,7 @@ public class ShowcaseTests { @Test public void testD() throws Exception { + testInitialState(); listener.reset(3, 3, 0); machine.sendEvent(Events.D); listener.stateChangedLatch.await(1, TimeUnit.SECONDS); @@ -139,6 +158,7 @@ public class ShowcaseTests { @Test public void testCD() throws Exception { + testInitialState(); listener.reset(1, 3, 0); machine.sendEvent(Events.C); listener.stateChangedLatch.await(1, TimeUnit.SECONDS); @@ -154,6 +174,7 @@ public class ShowcaseTests { @Test public void testI() throws Exception { + testInitialState(); listener.reset(1, 1, 1); machine.sendEvent(Events.I); listener.stateChangedLatch.await(1, TimeUnit.SECONDS); @@ -168,17 +189,30 @@ public class ShowcaseTests { @Test public void testII() throws Exception { + testInitialState(); + listener.reset(1, 1, 1); machine.sendEvent(Events.I); + listener.stateChangedLatch.await(1, TimeUnit.SECONDS); + assertThat(listener.stateChangedLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateEnteredLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateExitedLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.statesEntered.size(), is(1)); + assertThat(listener.statesExited.size(), is(1)); + assertThat(machine.getState().getIds(), contains(States.S0, States.S1, States.S12)); - listener.reset(2, 0, 0); + listener.reset(1, 3, 2); machine.sendEvent(Events.I); assertThat(listener.stateChangedLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateEnteredLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateExitedLatch.await(1, TimeUnit.SECONDS), is(true)); assertThat(listener.statesEntered.size(), is(3)); + assertThat(listener.statesExited.size(), is(2)); assertThat(machine.getState().getIds(), contains(States.S0, States.S2, States.S21, States.S212)); } @Test public void testH() throws Exception { + testInitialState(); listener.reset(0, 0, 0, 1); machine.sendEvent(Events.H); listener.transitionLatch.await(1, TimeUnit.SECONDS); @@ -188,6 +222,7 @@ public class ShowcaseTests { @Test public void testCH() throws Exception { + testInitialState(); machine.sendEvent(Events.C); listener.reset(0, 0, 0, 1); machine.sendEvent(Events.H); @@ -198,6 +233,7 @@ public class ShowcaseTests { @Test public void testACH() throws Exception { + testInitialState(); machine.sendEvent(Events.A); machine.sendEvent(Events.C); listener.reset(0, 0, 0, 1); @@ -209,19 +245,26 @@ public class ShowcaseTests { @Test public void testE() throws Exception { - listener.reset(1, 2, 3, 0); + testInitialState(); + listener.reset(1, 4, 3, 0); machine.sendEvent(Events.E); listener.stateChangedLatch.await(1, TimeUnit.SECONDS); + assertThat(listener.stateChangedLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateEnteredLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateExitedLatch.await(1, TimeUnit.SECONDS), is(true)); assertThat(machine.getState().getIds(), contains(States.S0, States.S2, States.S21, States.S211)); - assertThat(listener.statesExited.size(), is(2)); - assertThat(listener.statesEntered.size(), is(2)); + assertThat(listener.statesExited.size(), is(3)); + assertThat(listener.statesEntered.size(), is(4)); } @Test public void testF() throws Exception { - listener.reset(1, 4, 2, 0); + testInitialState(); + listener.reset(1, 3, 2, 0); machine.sendEvent(Events.F); - listener.stateChangedLatch.await(1, TimeUnit.SECONDS); + assertThat(listener.stateChangedLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateEnteredLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateExitedLatch.await(1, TimeUnit.SECONDS), is(true)); assertThat(machine.getState().getIds(), contains(States.S0, States.S2, States.S21, States.S211)); assertThat(listener.statesExited.size(), is(2)); assertThat(listener.statesEntered.size(), is(3)); @@ -229,9 +272,12 @@ public class ShowcaseTests { @Test public void testG() throws Exception { - listener.reset(1, 4, 2, 0); + testInitialState(); + listener.reset(1, 3, 2, 0); machine.sendEvent(Events.G); - listener.stateChangedLatch.await(1, TimeUnit.SECONDS); + assertThat(listener.stateChangedLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateEnteredLatch.await(1, TimeUnit.SECONDS), is(true)); + assertThat(listener.stateExitedLatch.await(1, TimeUnit.SECONDS), is(true)); assertThat(machine.getState().getIds(), contains(States.S0, States.S2, States.S21, States.S211)); assertThat(listener.statesExited.size(), is(2)); assertThat(listener.statesEntered.size(), is(3));