Reconsider lifecycle state transition when returning FAILURE for on_shutdown transition.

[bpwilcox]

I can't say it's accurate to call this a bug, however the ROS2 lifecycle design documentation does not clearly specify the appropriate behavior for what happens when a FAILURE code is returned from a transition callback. The doc only mentions specifically that a failed configuring transition should result in the original (here unconfigured) state. It turns out that, though unspecified in the design diagram or otherwise in the doc, this is the behavior for all the transitions EXCEPT the shutdown transition. I believe this is fair behavior, however, when a user returns CallbackReturn::FAILURE in the on_shutdown transition callback, the resulting state is the same as if it had succeeded, the finalized state. This seems dangerous and prone to issues particular in interpreting the finalized state. To the design doc's own description, the shutdown transition should be responsible for any "cleanup necessary before destruction". Thus, if the user intentionally returns a failure code, it is counterintuitive to transition into the finalized state as though it had succeeded.

Yes, a user can return a CallbackReturn::ERROR, but these are different use cases. I believe that the failure return code should behave the same way here as it does for the other transitions. Since a shutdown can be triggered from any primary state, if that transition fails, it should return to the previous state.

[bpwilcox]

Looking at ros2/design#283 (comment) and ros2/design#283 (comment), I agree that the failure transitions should be properly documented and that the shutdown failure behavior should return to the previous state.

[CursedRock17]

As per this comment:

• Lifecycle primary state error transitions design#283 (comment)

It was recommended that if the node had shutdown, then it would raise a failure and be left as this node is now "dead" which means that its state doesn't matter. This could be left alone unless the core designers would rather change the way nodes change on shutdown, but that seems unnecessary.