Use exec instead of popen to run ign-launch binary

[azeey]

🦟 Bug fix

Fixes #149

Summary

Currently, the ruby processes creates a new process to run the ign-launch binary. For some reason, signals sent to the ruby processes don't propagate to the child process. In addition to the issue described in #149, sending a SIGINT signal (or even SIGKILL) directly via kill to the parent process doesn't terminate the launched process.

For example, launch the following:

sleep.ign

<?xml version='1.0'?>
<ignition version='1.0'>
  <executable name='sleep_ign'>
    <command>sleep 100</command>
  </executable>
</ignition>

Launch with: ign launch sleep.ign

The process list shows there are two processes associated with the launch

❯ ps x --forest  | grep ign
1423316 pts/24   Sl+    0:00  |   |   \_ ign launch sleep.ign
1423346 pts/24   Sl+    0:00  |   |       \_ /home/addisu/ws/fortress/install/lib/ignition/launch5/ign-launch sleep.ign

Killing the top/parent process does not terminate the child

❯ kill -SIGINT 1423316
❯ ps x --forest  | grep ign
1423346 pts/24   Sl     0:00  \_ /home/addisu/ws/fortress/install/lib/ignition/launch5/ign-launch sleep.ign

https://www.vidarholen.net/contents/blog/?p=34 explains the problem (which I learned about from ros2/launch#484).

The solution I propose is to replace the ruby process with the ign-launch process using exec. Here's the process list with the new approach.

❯ ps x --forest  | grep ign
1431877 pts/24   Sl+    0:00  |   |   \_ /home/addisu/ws/fortress/install/lib/ignition/launch5/ign-launch sleep.ign

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• 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.

[codecov]

Codecov Report

Merging #151 (e25dc07) into ign-launch5 (b63eb2e) will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           ign-launch5     #151   +/-   ##
============================================
  Coverage        32.92%   32.92%           
============================================
  Files                4        4           
  Lines              817      817           
============================================
  Hits               269      269           
  Misses             548      548           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b63eb2e...e25dc07. Read the comment docs.

[adityapande-1995]

I'm not familiar with this package, but is there a possible case whereign launch ... launches one more process in addition to ign-launch binary ?

[azeey]

I'm not familiar with this package, but is there a possible case whereign launch ... launches one more process in addition to ign-launch binary ?

I don't think so. All the processes that are in the launch file are started by the ign-launch binary.

[osrf-triage]

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1