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

Fixes gazebo shutdown error when using nested launch files #1376

Merged
merged 4 commits into from
May 9, 2022

Conversation

adityapande-1995
Copy link

@adityapande-1995 adityapande-1995 commented Apr 28, 2022

Resolves ros2/launch#545

Originally, gzserver and gzclient were launched using shell=True.

This means the launch process executes a shell, which executes the server and client. When launch_testing exits, it sends SIGINT to the children shell processes, which do not forward SIGINT to the respective server and client children. These processes remain alive even after launch exits, as children of systemd.

Check out https://linuxconfig.org/how-to-propagate-a-signal-to-child-processes-from-a-bash-script for reference.

Testing

The following test launch file should exit properly and kill both the client and server processes :

import sys
import time
import unittest

import launch
from launch.actions import IncludeLaunchDescription
from launch.launch_description_sources import PythonLaunchDescriptionSource
from launch_ros.substitutions import FindPackageShare
from launch_testing.actions import ReadyToTest
import launch_testing.markers
import pytest


@pytest.mark.launch_test
def generate_test_description():
    return launch.LaunchDescription(
        [
            IncludeLaunchDescription(
                PythonLaunchDescriptionSource([FindPackageShare('gazebo_ros'), '/launch/gazebo.launch.py'])
            ),
            ReadyToTest(),
        ])


class MyTestFixture(unittest.TestCase):

    def test_something(self):
        time.sleep(10.0)
        print('END TEST', file=sys.stderr)

Copy link
Collaborator

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
I'm not sure if there was any reason to use shell=True.

@adityapande-1995
Copy link
Author

@ros-pull-request-builder retest this please

Copy link
Collaborator

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I try the launch a complex Gazebo world with these changes, it fails to load plugins:

[gzserver-1] [Err] [Plugin.hh:212] Failed to load plugin  libgazebo_ros_init.so:  libgazebo_ros_init.so: cannot open shared object file: No such file or directory
[gzserver-1] [Err] [Plugin.hh:212] Failed to load plugin  libgazebo_ros_factory.so:  libgazebo_ros_factory.so: cannot open shared object file: No such file or directory
[gzserver-1] [Err] [Plugin.hh:212] Failed to load plugin  libgazebo_ros_force_system.so:  libgazebo_ros_force_system.so: cannot open shared object file: No such file or directory

I guess that the changes are somehow affecting command line argument parsing?

@jacobperron
Copy link
Collaborator

jacobperron commented May 2, 2022

Actually, the errors occur in an empty world too. You just need to pass verbose:=true to see the errors:

ros2 launch gazebo_ros gazebo.launch.py verbose:=true

@adityapande-1995
Copy link
Author

Hmm, I'm not sure why that is happening, as the environment variables are included here in gzserver.launch.py :

    env = {
        'GAZEBO_MODEL_PATH': model,
        'GAZEBO_PLUGIN_PATH': plugin,
        'GAZEBO_RESOURCE_PATH': media
    }

@jacobperron
Copy link
Collaborator

I'm not sure either. The difference between shell=True and shell=False is that the former calls subprocess_shell and the later calls subprocess_exec, both are documented to treat the env argument the same as subprocess.Popen.

Copy link
Collaborator

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@adityapande-1995
Copy link
Author

Not sure what is happening with CI though

@jacobperron
Copy link
Collaborator

We're working on fixing CI issues (e.g. #1380). Please stand by.

@jacobperron
Copy link
Collaborator

Rebased on ros2

@adityapande-1995
Copy link
Author

The errors in CI are unrelated to this PR :

 ALSA lib confmisc.c:855:(parse_card) cannot find card '0'
10: ALSA lib conf.c:5178:(_snd_config_evaluate) function snd_func_card_inum returned error: No such file or directory
10: ALSA lib confmisc.c:422:(snd_func_concat) error evaluating strings
10: ALSA lib conf.c:5178:(_snd_config_evaluate) function snd_func_concat returned error: No such file or directory
10: ALSA lib confmisc.c:1334:(snd_func_refer) error evaluating name
10: ALSA lib conf.c:5178:(_snd_config_evaluate) function snd_func_refer returned error: No such file or directory
10: ALSA lib conf.c:5701:(snd_config_expand) Evaluate error: No such file or directory
10: ALSA lib pcm.c:2664:(snd_pcm_open_noupdate) Unknown PCM default
10: AL lib: (EE) ALCplaybackAlsa_open: Could not open playback device 'default': No such file or directory

@jacobperron jacobperron merged commit 352649f into ros2 May 9, 2022
@jacobperron jacobperron deleted the aditya/fix_gazebo_shutdown branch May 9, 2022 21:58
@ivanpauno
Copy link
Collaborator

Are we sure the issue commented here was not related to the PR?
It would important to double check before backporting, as that could cause many issues.

@adityapande-1995
Copy link
Author

Yeah it was caused due to earlier changes in this PR, fixed in this commit : e5b5676 , the space character was causing the issue.

@Arne-Christoph
Copy link

Arne-Christoph commented Jul 15, 2022

Hey all!
If I use these changes and try to change the publish frequency of the /clock topic via the extra_gazebo_args parameter

e.g.
"extra_gazebo_args",
default_value="--ros-args --params-file gazebo_params.yaml",

this is no longer working.
If I change the shell parameter back to true it is working again. Does someone has a clue what is going wrong?

@adityapande-1995 @ivanpauno have you noticed something like this?

@jacobperron
Copy link
Collaborator

@Arne-Christoph For visibility, it would be better to open up an issue with the exact commands that used to work, describing the previous and new behavior.

@Arne-Christoph
Copy link

alright. will do so.

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

Successfully merging this pull request may close these issues.

4 participants