Let exceptions from lifecycle transition callbacks bubble up.

[de-vri-es]

This PR removed the try/catch around transition callbacks that silently swallows errors.

Swallowing exceptions makes debugging and error handling extremely difficult. All error details are lost, both to the developer or end-user running the code and to other code that might want to inspect the error details at a higher level.

The function contract of the transistion callbacks also specifies the function should return a TransistionCallbackReturn. Raising exceptions to signal errors is not documented and unexpected.

Python code is difficult enough to debug without errors silently dissapearing :(

/edit: I now have to resort to this to find typos in my code. And it still doesn't allow me to capture the exception at a higher level.

    def on_configure(self, state: LifecycleState):
        try:
            # Do stuff
        except:
            traceback.print_exc()
            sys.exit(1)
        return TransitionCallbackReturn.SUCCESS

[fujitatomoya]

thanks for flagging this.
the silent swallow is a real problem and the that TODO has sat too long...
but i think the fix should be to log the exception (traceback could included) inside the existing handler and still return ERROR, rather than removing the handler.
that addresses the debugging pain without changing the callback contract, diverging from rclcpp, or risking FSM inconsistency. what do you think?

[de-vri-es]

I don't think catching the exception is correct behaviour, even if it is logged.

Firstly, it is this is not documented, which makes the behaviour a surprise.

But more importantly: even trivial programming errors (like a mistyped variable name) in python become a runtime exception. The standard behaviour of a python program in these scenarios is to crash. Differing from standard behaviour should require a good reason.

And there's no way for the user code to restore default behaviour. My work-around above is not standard behaviour either: it won't let a high level layer catch the exception anymore.

But also, keeping the program running after a bug triggered due to a programming error is what causes FSM inconsistencies, rather than solve them. Who knows what state the program is in now? It got interrupted at some arbitrary unknown point. There may be a user error handler that could function just fine if the program was correct, but because the bug didn't crash the program, we're in undefined behaviour.

In extreme cases this kind of thing could lead to a robot malfunctioning and causing bodily harm. I know this may sound far fetched, but this is such a low level component, it may be used for literally anything.

[de-vri-es]

Ok, hopefully the last commit can make everyone happy:

By default, exceptions are still caught, but also logged.

Optionally, by calling node.catch_callback_errors(False), this behavior can be completely disabled, letting exceptions bubble up as usual (without logging, because they haven't been caught, don't want to log it more than once).

[de-vri-es]

@fujitatomoya: Is this an acceptable trade-off?