Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect transition between nested states #421

Closed
thedrow opened this issue Apr 5, 2020 · 17 comments
Closed

Incorrect transition between nested states #421

thedrow opened this issue Apr 5, 2020 · 17 comments
Milestone

Comments

@thedrow
Copy link
Contributor

thedrow commented Apr 5, 2020

Consider the following state machine:

class ServiceRestartState(Enum):
    starting = auto()
    stopping = auto()
    stopped = auto()


class ServiceState(Enum):
    initialized = auto()
    starting = auto()
    started = auto()
    restarting = ServiceRestartState
    stopping = auto()
    stopped = auto()


class Service(HierarchicalAsyncMachine):
    def __init__(self):
        transitions = [
            ['starting', [ServiceState.initialized, ServiceState.stopped], ServiceState.starting],
            ['started', [ServiceState.starting, ServiceRestartState.starting], ServiceState.started],
            ['restarting', ServiceState.started, 'restarting'],
            ['stopping', 'restarting', ServiceRestartState.stopping],
            ['stopped', ServiceRestartState.stopping, ServiceRestartState.stopped],
            ['starting', ServiceRestartState.stopped, ServiceRestartState.starting],
            ['stopping', ServiceState.started, ServiceState.stopping],
            ['stopped', ServiceState.stopping, ServiceState.stopped],
        ]
        super().__init__(states=ServiceState,
                         transitions=transitions,
                         initial=ServiceState.initialized,
                         auto_transitions=False)

If I am currently at the stopped stage I can't transition to stopped.

However if you do attempt to do so the stopped trigger will transition the state machine to ServiceRestartState.stopped.

Try the code below:

m = Service()
m.add_transition('travel', ServiceState.initialized, ServiceState.stopped)
m.travel()
m.stopped()

I tried changing the name of the nested states, I tried to rename the triggers to not be the same. Nothing works.

@thedrow thedrow changed the title Incorrect transition between nested states. Incorrect transition between nested states Apr 5, 2020
@thedrow
Copy link
Contributor Author

thedrow commented Apr 10, 2020

@aleneum Can you please take a look?

@aleneum
Copy link
Member

aleneum commented Jun 10, 2020

I am not sure whether this is still relevant for you. I guess you refer to
transitions.core.MachineError: "Can't trigger event 'stopped' from state(s) ServiceState.stopped!" when calling m.stopped()? If so, the HierarchicalGraphMachine shows that there is no valid trigger stopped from the stopped State:

enum_states

@thedrow
Copy link
Contributor Author

thedrow commented Jun 15, 2020

That's exactly the problem.
No exception is raised.

Instead, we transition to ServiceRestartState.stopped.

Could this be related to #419?

@aleneum
Copy link
Member

aleneum commented Jun 15, 2020

No exception is raised.

Oh, I see. Did you await the event triggers or did you run them in the asyncio loop:

m = Service()
m.add_transition('travel', ServiceState.initialized, ServiceState.stopped)
asyncio.run(m.travel())
asyncio.run(m.stopped())

If so, then this problem might be indeed solved by #419.

@thedrow
Copy link
Contributor Author

thedrow commented Jun 15, 2020

I awaited them

@aleneum
Copy link
Member

aleneum commented Jun 18, 2020

Does it work for you now with the latest version from master?

@aleneum aleneum added this to the 0.8.2 milestone Jun 18, 2020
@thedrow
Copy link
Contributor Author

thedrow commented Jun 21, 2020

Unfortunately, there's another bug and this is not related to #419.

XFAIL [ 22%]
initial_state = <ServiceRestartState.stopped: 3>, transition = 'stopping'

    @pytest.mark.parametrize(
        "initial_state,transition",
        [
            (ServiceState.initializing, "starting"),
            (ServiceState.initializing, "started"),
            (ServiceState.initializing, "stopping"),
            (ServiceState.initializing, "stopped"),
            (ServiceState.initializing, "restarting"),
            (ServiceState.initialized, "started"),
            (ServiceState.initialized, "restarting"),
            (ServiceState.initialized, "stopping"),
            (ServiceState.initialized, "stopped"),
            (ServiceState.starting, "initialized"),
            (ServiceState.starting, "starting"),
            (ServiceState.starting, "restarting"),
            (ServiceState.starting, "stopping"),
            (ServiceState.starting, "stopped"),
            (ServiceState.started, "initialized"),
            (ServiceState.started, "starting"),
            (ServiceState.started, "started"),
            (ServiceState.started, "stopped"),
            (ServiceState.stopping, "initialized"),
            (ServiceState.stopping, "starting"),
            (ServiceState.stopping, "started"),
            (ServiceState.stopping, "restarting"),
            (ServiceState.stopping, "stopping"),
            (ServiceState.stopped, "initialized"),
            (ServiceState.stopped, "stopped"),
            (ServiceState.stopped, "started"),
            (ServiceState.stopped, "restarting"),
            (ServiceState.stopped, "stopping"),
            (ServiceState.restarting, "initialized"),
            (ServiceState.restarting, "restarting"),
            (ServiceState.restarting, "stopped"),
            (ServiceState.restarting, "starting"),
            (ServiceState.restarting, "started"),
            (ServiceRestartState.starting, "initialized"),
            (ServiceRestartState.starting, "starting"),
            (ServiceRestartState.starting, "stopped"),
            pytest.param(ServiceRestartState.starting, "stopping", marks=pytest.mark.xfail),
            (ServiceRestartState.starting, "restarting"),
            (ServiceRestartState.stopping, "initialized"),
            (ServiceRestartState.stopping, "starting"),
            (ServiceRestartState.stopping, "started"),
            pytest.param(ServiceRestartState.stopping, "stopping", marks=pytest.mark.xfail),
            (ServiceRestartState.stopping, "restarting"),
            (ServiceRestartState.stopped, "initialized"),
            (ServiceRestartState.stopped, "started"),
            (ServiceRestartState.stopped, "stopped"),
            pytest.param(ServiceRestartState.stopped, "stopping", marks=pytest.mark.xfail),
            (ServiceRestartState.stopped, "restarting"),
        ],
    )
    @pytest.mark.anyio
    async def test_service_state_machine_forbidden_transitions(initial_state, transition):
        """The state machine cannot change state from {initial_state} using the {transition} transition."""
        machine = Service()
        machine.add_transition("travel", ServiceState.initializing, initial_state)
        await machine.travel()
    
        with pytest.raises(MachineError, match=rf"Can\'t trigger event \'{transition}\' from state\(s\) {initial_state}!"):
            await getattr(machine, transition)()
>           pytest.fail(f"State machine transitioned to {machine.state} instead of failing")
E           Failed: State machine transitioned to ServiceRestartState.stopping instead of failing

test_services.py:85: Failed

@aleneum
Copy link
Member

aleneum commented Jun 21, 2020

I am not sure if I get this test right but dont you always add a transition named 'travel' and then call it? Why should that fail? Edit: Oh! Sorry, I missed the second part! My bad.

@aleneum
Copy link
Member

aleneum commented Jun 21, 2020

You defined ['stopping', 'restarting', ServiceRestartState.stopping] which means that when you are in ServiceRestartState or any substate and your particular state has no 'stopping' event itself it will trigger that one. The execution order goes from child to parent: If a child cannot resolve an event, transitions will check whether the parent can resolve that event until a root state is reached. When you remove stopping from the available transitions ...

        transitions = [
            ['starting', [ServiceState.initialized, ServiceState.stopped], ServiceState.starting],
            ['started', [ServiceState.starting, ServiceRestartState.starting], ServiceState.started],
            ['restarting', ServiceState.started, ServiceState.restarting],
            # ['stopping', ServiceState.restarting, ServiceRestartState.stopping],
            ['stopped', ServiceRestartState.stopping, ServiceRestartState.stopped],
            ['starting', ServiceRestartState.stopped, ServiceRestartState.starting],
            ['stopping', ServiceState.started, ServiceState.stopping],
            ['stopped', ServiceState.stopping, ServiceState.stopped],
        ]

... you will get a MachineError.

Design-wise you could add an 'initial' state to ServiceRestartState to get stopping out of the parent's scope:

class ServiceRestartState(Enum):
    pending = auto()
    starting = auto()
    stopping = auto()
    stopped = auto()

# ...
        transitions = [
            ['starting', [ServiceState.initialized, ServiceState.stopped], ServiceState.starting],
            ['started', [ServiceState.starting, ServiceRestartState.starting], ServiceState.started],
            ['restarting', ServiceState.started, ServiceRestartState.pending],
            ['stopping', ServiceRestartState.pending, ServiceRestartState.stopping],
            ['stopped', ServiceRestartState.stopping, ServiceRestartState.stopped],
            ['starting', ServiceRestartState.stopped, ServiceRestartState.starting],
            ['stopping', ServiceState.started, ServiceState.stopping],
            ['stopped', ServiceState.stopping, ServiceState.stopped],
        ]

test

@thedrow
Copy link
Contributor Author

thedrow commented Jun 22, 2020

I am not sure if I get this test right but dont you always add a transition named 'travel' and then call it? Why should that fail? Edit: Oh! Sorry, I missed the second part! My bad.

I created a custom state because of #427 which is now fixed.

@thedrow
Copy link
Contributor Author

thedrow commented Jun 24, 2020

You defined ['stopping', 'restarting', ServiceRestartState.stopping] which means that when you are in ServiceRestartState or any substate and your particular state has no 'stopping' event itself it will trigger that one. The execution order goes from child to parent: If a child cannot resolve an event, transitions will check whether the parent can resolve that event until a root state is reached. When you remove stopping from the available transitions ...

        transitions = [
            ['starting', [ServiceState.initialized, ServiceState.stopped], ServiceState.starting],
            ['started', [ServiceState.starting, ServiceRestartState.starting], ServiceState.started],
            ['restarting', ServiceState.started, ServiceState.restarting],
            # ['stopping', ServiceState.restarting, ServiceRestartState.stopping],
            ['stopped', ServiceRestartState.stopping, ServiceRestartState.stopped],
            ['starting', ServiceRestartState.stopped, ServiceRestartState.starting],
            ['stopping', ServiceState.started, ServiceState.stopping],
            ['stopped', ServiceState.stopping, ServiceState.stopped],
        ]

... you will get a MachineError.

Design-wise you could add an 'initial' state to ServiceRestartState to get stopping out of the parent's scope:

class ServiceRestartState(Enum):
    pending = auto()
    starting = auto()
    stopping = auto()
    stopped = auto()

# ...
        transitions = [
            ['starting', [ServiceState.initialized, ServiceState.stopped], ServiceState.starting],
            ['started', [ServiceState.starting, ServiceRestartState.starting], ServiceState.started],
            ['restarting', ServiceState.started, ServiceRestartState.pending],
            ['stopping', ServiceRestartState.pending, ServiceRestartState.stopping],
            ['stopped', ServiceRestartState.stopping, ServiceRestartState.stopped],
            ['starting', ServiceRestartState.stopped, ServiceRestartState.starting],
            ['stopping', ServiceState.started, ServiceState.stopping],
            ['stopped', ServiceState.stopping, ServiceState.stopped],
        ]

test

Maybe I'm just too tired, but I've been staring at this for a couple of days and I can't figure out what you're saying and why I'd need another state.

Can you rephrase that?

@aleneum
Copy link
Member

aleneum commented Jun 24, 2020

Can you rephrase that?

Sure, first a quote from the SCXML documentation that describes this behaviour:

Compound states also affect how transitions are selected. When looking for transitions, the state machine first looks in the most deeply nested active state(s), i.e., in the atomic state(s) that have no substates. If no transitions match in the atomic state, the state machine will look in its parent state, then in the parent's parent, etc. Thus transitions in ancestor states serve as defaults that will be taken if no transition matches in a descendant state. If no transition matches in any state, the event is discarded.

An event definition like trigger='stopping', source='restarting', dest='restarting.stopping' means in a sentence: "When receiving a 'stopping' event in state restarting or any substate which does not have its own handling of the event, transition to restarting.stopping". restarting.stopped is a substate of restarting which makes stopping a valid event and thus triggers a transition from restarting.stopped to restarting.stopping.

s.restarting()
# INFO:transitions.core:Entered state restarting
s.stopping()
# INFO:transitions.core:Entered state restarting_stopping
s.stopped()
# INFO:transitions.core:Exited state restarting_stopping
# INFO:transitions.core:Entered state restarting_stopped
s.stopping()
# !restarting_stopped should be exited here! <-- this is a bug [fixed!]
# INFO:transitions.core:Entered state restarting_stopping

If I get you right, you don't want 'stopping' to be a valid event in restarting_stopped but -- by the above mentioned definition -- events defined in a parent are ALWAYS valid in a child.

@aleneum
Copy link
Member

aleneum commented Jun 24, 2020

Maybe its easier to see when using transitions-gui since references FROM parent states to child states arent depicted nicely in graphviz (self-references to compound states are even worse).
This is the transitions and state sequence when emitting 'restarting', 'stopping', 'stopped', 'stopping':
service_states

@aleneum
Copy link
Member

aleneum commented Jun 27, 2020

0.8.2 has been released. I will close this since it seems like the issue mentioned in the beginning has been solved. Feel free to comment though if you face further issues with enums and HSMs. I will reopen the issue if necessary. For unrelated problems, please open a new issue.

@aleneum aleneum closed this as completed Jun 27, 2020
@thedrow
Copy link
Contributor Author

thedrow commented Jun 28, 2020

If I get you right, you don't want 'stopping' to be a valid event in restarting_stopped but -- by the above mentioned definition -- events defined in a parent are ALWAYS valid in a child.

Yes exactly.
I don't need another state though. It doesn't make sense in my context.
Is there another solution?

@aleneum
Copy link
Member

aleneum commented Jun 28, 2020

You could use conditions and allow transitions only from the parent state itself:

class Service(HierarchicalMachine):

    def __init__(self):
        transitions = [
            ['starting', [ServiceState.initialized, ServiceState.stopped], ServiceState.starting],
            ['started', [ServiceState.starting, ServiceRestartState.starting], ServiceState.started],
            ['restarting', ServiceState.started, ServiceState.restarting],
            dict(trigger='stopping', source=ServiceState.restarting, dest=ServiceRestartState.stopping, conditions='is_restarting'),
            ['stopped', ServiceRestartState.stopping, ServiceRestartState.stopped],
            ['starting', ServiceRestartState.stopped, ServiceRestartState.starting],
            ['stopping', ServiceState.started, ServiceState.stopping],
            ['stopped', ServiceState.stopping, ServiceState.stopped],
        ]
        super().__init__(states=ServiceState,
                         transitions=transitions,
                         initial=ServiceState.initialized,
                         auto_transitions=False)

logging.basicConfig(level=logging.INFO)

s = Service()
s.starting()
s.started()
s.restarting()
s.stopping()
s.stopped()
assert not s.stopping()  # will return False since it does not fulfill condition 'is_restarting'
assert s.is_restarting_stopped()

@thedrow
Copy link
Contributor Author

thedrow commented Jun 29, 2020

You could use conditions and allow transitions only from the parent state itself

I'll try that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants