Bugfix: if "false" is always True

[nachovizzo]

🦟 Bug fix

There is an issue in this launch file when passing the string 'false' as an argument. In Python, non-empty strings are always evaluated as True, regardless of their content. This means that even if you pass 'false', the system will still evaluate it as True.

This bug results in the launch system incorrectly calling the OnShutdown method twice. When any ROS launch action invokes a RosAdapter, it triggers the following exception: "Cannot shutdown a ROS adapter that is not running."

To temporarily work around this issue, you can launch gz_sim_launch.py with the on_exit_shutdown argument set to an empty string. This prevents the erroneous shutdown sequence and avoids the associated exception.

Full project that helps to reproduce the bug

example.zip

Summary

The problem is rather simple to reproduce:

Let's say I have a dummy composable node, and I need to launch it with a Gazebo simulation using the same launch file. This will raise an error and make the simulation exit with an error code:

$ ros2 launch example_component example.launch.py -d; echo $?

error

[DEBUG] [launch]: Traceback (most recent call last):
  File "/opt/ros/iron/lib/python3.10/site-packages/launch/launch_service.py", line 337, in run_async
    raise completed_tasks_exceptions[0]
  File "/opt/ros/iron/lib/python3.10/site-packages/launch/launch_service.py", line 230, in _process_one_event
    await self.__process_event(next_event)
  File "/opt/ros/iron/lib/python3.10/site-packages/launch/launch_service.py", line 239, in __process_event
    entities = event_handler.handle(event, self.__context)
  File "/opt/ros/iron/lib/python3.10/site-packages/launch/event_handlers/on_shutdown.py", line 67, in handle
    return self.__on_shutdown(cast(Shutdown, event), context)
  File "/opt/ros/iron/lib/python3.10/site-packages/launch_ros/ros_adapters.py", line 122, in <lambda>
    on_shutdown=lambda *args, **kwargs: ros_adapter.shutdown()
  File "/opt/ros/iron/lib/python3.10/site-packages/launch_ros/ros_adapters.py", line 86, in shutdown
    raise RuntimeError('Cannot shutdown a ROS adapter that is not running')
RuntimeError: Cannot shutdown a ROS adapter that is not running

[ERROR] [launch]: Caught exception in launch (see debug for traceback): Cannot shutdown a ROS adapter that is not running
1

node.cpp

class ComposableNodeExample : public rclcpp::Node
{
public:
  ComposableNodeExample(const rclcpp::NodeOptions & options) : Node("hello_world_node", options)
  {
    RCLCPP_INFO(this->get_logger(), "It does not matter");
  }
};

#include "rclcpp_components/register_node_macro.hpp"
RCLCPP_COMPONENTS_REGISTER_NODE(ComposableNodeExample)

launch.py

def generate_launch_description():
    container = ComposableNodeContainer(
        name="example_container",
        namespace="",
        package="rclcpp_components",
        executable="component_container",
        composable_node_descriptions=[
            ComposableNode(
                package="example_component",
                plugin="ComposableNodeExample",
                name="example_node",
            ),
        ],
        output="screen",
    )

    gazebo = IncludeLaunchDescription(
        PythonLaunchDescriptionSource(
            get_package_share_directory("ros_gz_sim") + "/launch/gz_sim.launch.py"
        ),
        launch_arguments={
            "gz_args": ["-v 0 -r -s shapes.sdf"],
            #  "on_exit_shutdown": "",
        }.items(),
    )

    return LaunchDescription(
        [
            container,
            gazebo,
        ]
    )

The real problem

The real issue is that we are registering with gz_sim.launch.py twice the OnShutdown handler, as the launch file misuses Python.

How to verify the solution

Option 1 "on_exit_shutdown": ""

On the example launch file, pass an empty string, therefore forcing the if to evaluate to False

            #  "on_exit_shutdown": "",

Option 2

Merge this PR 😎

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎈 Release

Preparation for <X.Y.Z> release.

Comparison to <x.y.z>: https://github.com/gazebosim//compare/<LATEST_TAG_BRANCH>...<RELEASE_BRANCH>

Needed by <PR(s)>

Checklist

• Asked team if this is a good time for a release
• There are no changes to be ported from the previous major version
• No PRs targeted at this major version are close to getting in
• Bumped minor for new features, patch for bug fixes
• Updated changelog
• Updated migration guide (as needed)
• Link to PR updating dependency versions in appropriate repository in gazebo-release (as needed):

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[Amronos]

Shouldn't this also be backported to Jazzy?

[HovorunBh]

@ahcorde Can you please backport this to Jazzy? 🙏

[ahcorde]

https://github.com/Mergifyio backport jazzy

[mergify]

backport jazzy

✅ Backports have been created

• #640 Bugfix: if "false" is always True (backport #617) has been created for branch jazzy