`on_exception` documentation and `event_data.result` with `HierarchicalMachine`

[spagh-eddie]

Describe the bug

I am unsure if this:

1. is not intended, or
2. is intended and should be documented in README

Expected behavior

I would have expected the end state to be B_D instead of A. Adding event.result = True after del event.error gets me my expected behavior.

# expected behavior
>>> lump.next()
True
>>> lump.next()
Fixing things ...
True
>>> lump.state
'B_D'

Minimal working example

from transitions.extensions import HierarchicalMachine

class Matter(object):
    def raise_error(self, event): raise ValueError("Oh no")
    def handle_error(self, event):
        print("Fixing things ...")
        del event.error  # it did not happen if we cannot see it ...
        # event.result = True

states=[
    'A',
    {
        'name': 'B',
        'states': ['C', {'name': 'D', 'on_enter': 'raise_error'}]
    }
]
transitions=[
    ['next', 'A', 'B_C'],
    ['next', 'B_C','B_D'],
    # end state is always 'B_D' if change this to be ['any_other_trigger', 'B', 'A']
    ['next', 'B', 'A'], 
]

lump = Matter()
m = HierarchicalMachine(lump, states,'A', transitions,on_exception='handle_error', send_event=True)

>>> lump.next()
True
>>> lump.next()
Fixing things ...
False
>>> lump.state
'A'

Additional context

transitions version 0.8.10

[aleneum]

Hello @spagh-eddie,

the documentation states:

If any callback raises an exception, the processing of callbacks is not continued. This means that when an error occurs before the transition (in state.on_exit or earlier), it is halted. In case there is a raise after the transition has been conducted (in state.on_enter or later), the state change persists and no rollback is happening.

since you raise your error in on_enter I'd expect B_D as a resulting state as well. I will have a look.

[aleneum]

I had a look and I would say that your machine ending in state A is the currently expected behavior because of

['next', 'B', 'A'], 

The relevant part of the documentation is:

In addition to the processing order of transitions known from Machine where transitions are considered in the order they were added, HierarchicalMachine considers hierarchy as well. Transitions defined in substates will be evaluated first ...

The transition B_C->B_D is considered invalid since it raises an exception (result is False). Transition will then proceed to look for other valid transitions. This will continue until a transition returns True or no valid transition can be found any longer. For your use case the one and only valid transition left is B->A and this will process successfully. While this transition is evaluated, your model Matter is actually in state B_D! So if you change it into

['next', 'B_C', 'A']`

you will end up in B_D.

The fact, that you can prevent transition 'bubbling' by setting EventData.result = True was not intended but it's a neat little trick. I will have a look if we can consider return values of exception callbacks easily. We could then add the behavior that when any exception callback returns True the transition is considered 'successful'. It will be halted anyway but no further transition is evaluated (for that particular source state; when evaluating parallel states things will be more complicated). What do you think, @spagh-eddie?

[spagh-eddie]

Thank you for your detailed response.

The transition B_C->B_D is considered invalid... will then proceed to look for other valid transitions.

Are you saying that transitions does not distinguish between no-available-transition and transition-raised-or-failed? and so it assumes no-available-transition and goes up the hierarchy in search of its parent's transition?

Is the raised MachineError enough to distinguish between these two?

I will have a look if we can consider return values of exception callbacks easily

This seems similar enough to context manager's __exit__ behavior. However, given that del event.error is already documented, putting event.result = True next to it would have precedent. Alternatively, handle both error and result with the __exit__-like return value.

I do not feel too strongly on this and would be happy with either as long as it is documented.

[aleneum]

Are you saying that transitions does not distinguish between no-available-transition and transition-raised-or-failed? and so it assumes no-available-transition and goes up the hierarchy in search of its parent's transition?

as of now, it does not distinguish between a failed (as in exception raised) transition and a transition that does not fulfill required conditions. If there is no valid transition at all for a source state, transitions will raise an Exception if ignore_invalid_triggers is not set to True.

As the documentation states, you can model conditional transitions like this:

transitions = [
  {"trigger": "next", "source": "A", "dest": "B", "conditions": "check_A"},
  {"trigger": "next", "source": "A", "dest": "C", "conditions": "check_B"}
  {"trigger": "next", "source": "A", "dest": "D"}
]

When a next event appears, transitions will evaluate transitions in the order they were added. So, when check_A returns True, the resulting state will be B. If neither check_A or check_B return True, A->D will act similar to an else statement. HierarchicalMachine applies the same concept to the order of potential transitions implied by hierarchy.

I guess how errors are currently handled is also related to #552. Increasing the try/catch scope might change the behavior to what you would have expected (stopping the evaluation of transitions). I will leave this issue open and come back to it, when #552 is resolved.

[aleneum]

As expected, your example will end up in B_D with transitions 0.9 (currently dev-0.9 branch) and the increased try/catch scope for processing transitions.

[aleneum]

This issue has been resolved with the aforementioned commit and should be fixed in transitions 0.9.0 which was recently pushed to the master branch. Feel free to comment if you still face this problem. I will reopen the issue if necessary.