From 10ed7feeaed1c3789dd613a5a3c9b16d4c7dcd65 Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Sat, 28 Oct 2017 15:07:45 +0100 Subject: [PATCH] Fix state holder usage - In AbstractStateMachineFactory change used holder map to list so that same states don't get lost because of used key. - Backport #416 - Relates #307 --- .../config/AbstractStateMachineFactory.java | 34 +++++--- .../state/ExitEntryStateTests.java | 83 ++++++++++++++++++- 2 files changed, 104 insertions(+), 13 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 d245bcf7..8d4907f8 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 @@ -180,7 +180,7 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS Stack> regionStack = new Stack>(); Stack> stateStack = new Stack>(); Map> machineMap = new HashMap>(); - Map> holderMap = new HashMap>(); + List> holderList = new ArrayList<>(); Iterator>> iterator = buildStateDataIterator(stateMachineModel); while (iterator.hasNext()) { @@ -216,7 +216,7 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS if (initialCount > 1) { for (Collection> regionStateDatas : regionsStateDatas) { - machine = buildMachine(machineMap, stateMap, holderMap, regionStateDatas, transitionsData, resolveBeanFactory(stateMachineModel), + machine = buildMachine(machineMap, stateMap, holderList, regionStateDatas, transitionsData, resolveBeanFactory(stateMachineModel), contextEvents, defaultExtendedState, stateMachineModel.getTransitionsData(), resolveTaskExecutor(stateMachineModel), resolveTaskScheduler(stateMachineModel), machineId, null, stateMachineModel); regionStack.push(new MachineStackItem(machine)); @@ -245,7 +245,7 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS machine = m; } } else { - machine = buildMachine(machineMap, stateMap, holderMap, stateDatas, transitionsData, resolveBeanFactory(stateMachineModel), contextEvents, + machine = buildMachine(machineMap, stateMap, holderList, stateDatas, transitionsData, resolveBeanFactory(stateMachineModel), contextEvents, defaultExtendedState, stateMachineModel.getTransitionsData(), resolveTaskExecutor(stateMachineModel), resolveTaskScheduler(stateMachineModel), machineId, uuid, stateMachineModel); if (peek.isInitial() || (!peek.isInitial() && !machineMap.containsKey(peek.getParent()))) { @@ -321,8 +321,8 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS // go through holders and fix state references which // were not known at a time holder was created - for (Entry> holder : holderMap.entrySet()) { - holder.getValue().setState(stateMap.get(holder.getKey())); + for (HolderListItem holderItem : holderList) { + holderItem.value.setState(stateMap.get(holderItem.key)); } // set parent machines for each built machine @@ -519,7 +519,7 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS @SuppressWarnings("unchecked") private StateMachine buildMachine(Map> machineMap, Map> stateMap, - Map> holderMap, Collection> stateDatas, Collection> transitionsData, + List> holderList, Collection> stateDatas, Collection> transitionsData, BeanFactory beanFactory, Boolean contextEvents, DefaultExtendedState defaultExtendedState, TransitionsData stateMachineTransitions, TaskExecutor taskExecutor, TaskScheduler taskScheduler, String machineId, UUID uuid, StateMachineModel stateMachineModel) { @@ -614,7 +614,7 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS StateHolder defaultStateHolder = new StateHolder(defaultState); StateHolder containingStateHolder = new StateHolder(stateMap.get(stateData.getParent())); if (containingStateHolder.getState() == null) { - holderMap.put((S)stateData.getParent(), containingStateHolder); + holderList.add(new HolderListItem((S)stateData.getParent(), containingStateHolder)); } PseudoState pseudoState = new HistoryPseudoState(PseudoStateKind.HISTORY_SHALLOW, defaultStateHolder, containingStateHolder); state = buildStateInternal(stateData.getState(), stateData.getDeferred(), stateData.getEntryActions(), @@ -634,7 +634,7 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS StateHolder defaultStateHolder = new StateHolder(defaultState); StateHolder containingStateHolder = new StateHolder(stateMap.get(stateData.getParent())); if (containingStateHolder.getState() == null) { - holderMap.put((S)stateData.getParent(), containingStateHolder); + holderList.add(new HolderListItem((S)stateData.getParent(), containingStateHolder)); } PseudoState pseudoState = new HistoryPseudoState(PseudoStateKind.HISTORY_DEEP, defaultStateHolder, containingStateHolder); state = buildStateInternal(stateData.getState(), stateData.getDeferred(), stateData.getEntryActions(), @@ -651,7 +651,7 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS for (ChoiceData c : list) { StateHolder holder = new StateHolder(stateMap.get(c.getTarget())); if (holder.getState() == null) { - holderMap.put(c.getTarget(), holder); + holderList.add(new HolderListItem(c.getTarget(), holder)); } choices.add(new ChoiceStateData(holder, c.getGuard(), c.getActions())); } @@ -667,7 +667,7 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS for (JunctionData c : list) { StateHolder holder = new StateHolder(stateMap.get(c.getTarget())); if (holder.getState() == null) { - holderMap.put(c.getTarget(), holder); + holderList.add(new HolderListItem(c.getTarget(), holder)); } junctions.add(new JunctionStateData(holder, c.getGuard(), c.getActions())); } @@ -696,7 +696,7 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS if (s.equals(entry.getSource())) { StateHolder holder = new StateHolder(stateMap.get(entry.getTarget())); if (holder.getState() == null) { - holderMap.put(entry.getTarget(), holder); + holderList.add(new HolderListItem(entry.getTarget(), holder)); } PseudoState pseudoState = new ExitPseudoState(holder); state = buildStateInternal(stateData.getState(), stateData.getDeferred(), stateData.getEntryActions(), @@ -751,7 +751,7 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS if (tt.getSource() == s) { StateHolder holder = new StateHolder(stateMap.get(tt.getTarget())); if (holder.getState() == null) { - holderMap.put(tt.getTarget(), holder); + holderList.add(new HolderListItem(tt.getTarget(), holder)); } joinTargets.add(new JoinStateData(holder, tt.getGuard())); } @@ -891,4 +891,14 @@ public abstract class AbstractStateMachineFactory extends LifecycleObjectS latch.countDown(); } } + + private static class HolderListItem { + S key; + StateHolder value; + + public HolderListItem(S key, StateHolder value) { + this.key = key; + this.value = value; + } + } } diff --git a/spring-statemachine-core/src/test/java/org/springframework/statemachine/state/ExitEntryStateTests.java b/spring-statemachine-core/src/test/java/org/springframework/statemachine/state/ExitEntryStateTests.java index e0a1b7bf..6ab44a63 100644 --- a/spring-statemachine-core/src/test/java/org/springframework/statemachine/state/ExitEntryStateTests.java +++ b/spring-statemachine-core/src/test/java/org/springframework/statemachine/state/ExitEntryStateTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2016 the original author or authors. + * Copyright 2016-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -48,6 +48,29 @@ public class ExitEntryStateTests extends AbstractStateMachineTests { assertThat(machine.getState().getIds(), contains("S4")); } + @SuppressWarnings("unchecked") + @Test + public void testMultipleExitsToSameState() { + context.register(Config2.class); + context.refresh(); + StateMachine machine = + context.getBean(StateMachineSystemConstants.DEFAULT_ID_STATEMACHINE, StateMachine.class); + assertThat(machine, notNullValue()); + machine.start(); + + assertThat(machine.getState().getIds(), contains("S1")); + machine.sendEvent("E1"); + assertThat(machine.getState().getIds(), contains("S2", "S22")); + machine.sendEvent("EXIT2"); + assertThat(machine.getState().getIds(), contains("S1")); + + machine.sendEvent("E2"); + assertThat(machine.getState().getIds(), contains("S3", "S32")); + machine.sendEvent("EXIT3"); + assertThat(machine.getState().getIds(), contains("S1")); + + } + @Configuration @EnableStateMachine static class Config1 extends StateMachineConfigurerAdapter { @@ -116,6 +139,64 @@ public class ExitEntryStateTests extends AbstractStateMachineTests { } } + @Configuration + @EnableStateMachine + static class Config2 extends StateMachineConfigurerAdapter { + + @Override + public void configure(StateMachineStateConfigurer states) throws Exception { + states + .withStates() + .initial("S1") + .state("S2") + .state("S3") + .and() + .withStates() + .parent("S2") + .initial("S21") + .exit("S2EXIT") + .state("S22") + .and() + .withStates() + .parent("S3") + .initial("S31") + .exit("S3EXIT") + .state("S32"); + } + + @Override + public void configure(StateMachineTransitionConfigurer transitions) throws Exception { + transitions + .withExternal() + .source("S1").target("S2") + .event("E1") + .and() + .withExternal() + .source("S1").target("S3") + .event("E2") + .and() + .withExternal() + .source("S21").target("S22") + .and() + .withExternal() + .source("S31").target("S32") + .and() + .withExternal() + .source("S22").target("S2EXIT") + .event("EXIT2") + .and() + .withExternal() + .source("S32").target("S3EXIT") + .event("EXIT3") + .and() + .withExit() + .source("S2EXIT").target("S1") + .and() + .withExit() + .source("S3EXIT").target("S1"); + } + } + @Override protected AnnotationConfigApplicationContext buildContext() {