Do not swallow errors from ExecuteProcess

[Aposhian]

Bug report

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • binaries
• Version or commit hash:
  • 0.17.0
• DDS implementation:
  • N/A
• Client library (if applicable):
  • N/A

Steps to reproduce issue

repro.py

from launch import LaunchDescription, LaunchService
from launch.actions import ExecuteProcess

def test_fakecommand():
    def check_ok_process(event, context):
        assert 0 == event.returncode
    ls = LaunchService()
    ls.include_launch_description(
        LaunchDescription(
            [
                ExecuteProcess(
                    cmd=["fakecommand"],
                    on_exit=check_ok_process
                )
            ]
        )
    )
    assert 0 == ls.run()

then run

python3 -m pytest -rP repro.py

Expected behavior

Either expect LaunchService::run() to return a nonzero exit code, or the process to return a nonzero exit code. Either of these cases should cause the test to fail. This sort of test allows me to write automated tests that should validate my launch files.

Actual behavior

The test passes, and the error is swallowed, without an easy way to catch it in the test.

================================= test session starts ==================================
platform linux -- Python 3.8.10, pytest-4.6.9, py-1.8.1, pluggy-0.13.0
rootdir: ...
plugins: ament-lint-0.10.6, ament-mypy-0.10.6, ament-flake8-0.10.6, launch-testing-ros-0.14.2, ament-copyright-0.10.6, ament-pep257-0.10.6, launch-testing-0.17.0, ament-xmllint-0.10.6, colcon-core-0.6.1, cov-2.8.1
collected 1 item                                                                       
repro.py .                               [100%]

======================================== PASSES ========================================
________________________________ test_fakecommand ________________________________
--------------------------------- Captured stdout call ---------------------------------
[INFO] [launch]: All log files can be found below ...
[INFO] [launch]: Default logging verbosity is set to INFO
[ERROR] [fakecommand-1]: exception occurred while executing process:
Traceback (most recent call last):
  File "/opt/ros/galactic/lib/python3.8/site-packages/launch/actions/execute_process.py", line 737, in __execute_process
    transport, self._subprocess_protocol = await async_execute_process(
  File "/opt/ros/galactic/lib/python3.8/site-packages/osrf_pycommon/process_utils/async_execute_process_asyncio/impl.py", line 139, in async_execute_process
    transport, protocol = await _async_execute_process_nopty(
  File "/opt/ros/galactic/lib/python3.8/site-packages/osrf_pycommon/process_utils/async_execute_process_asyncio/impl.py", line 45, in _async_execute_process_nopty
    transport, protocol = await loop.subprocess_exec(
  File "/usr/lib/python3.8/asyncio/base_events.py", line 1630, in subprocess_exec
    transport = await self._make_subprocess_transport(
  File "/usr/lib/python3.8/asyncio/unix_events.py", line 197, in _make_subprocess_transport
    transp = _UnixSubprocessTransport(self, protocol, args, shell,
  File "/usr/lib/python3.8/asyncio/base_subprocess.py", line 36, in __init__
    self._start(args=args, shell=shell, stdin=stdin, stdout=stdout,
  File "/usr/lib/python3.8/asyncio/unix_events.py", line 789, in _start
    self._proc = subprocess.Popen(
  File "/usr/lib/python3.8/subprocess.py", line 858, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.8/subprocess.py", line 1704, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'fakecommand'

=============================== 1 passed in 0.02 seconds ===============================

Additional information

This behavior is aligns with the exception being swallowed here: https://github.com/ros2/launch/blob/32dbb67a5b74e67c62483c213d3346906d64669e/launch/launch/actions/execute_local.py#L551-L556

I understand that not every error in a process should mean that the entire launch service should return an error. But anything that is a process-level error should result in a ProcessExited event or something to handle it with. This sort of error lives in-between, with nothing good to catch it with.

[hidmic]

@Aposhian hmm, a ProcessExited event seems inadequate if no process was ever started. Letting the exception bubble up to the event loop exception handler is perhaps best. @wjwwood @ivanpauno WDYT?

[ivanpauno]

ProcessExited event seems inadequate if no process was ever started.

can we distinguish between the exceptions that happened before the process started and the ones that happened after?

Letting the exception bubble up to the event loop exception handler is perhaps best.

That sounds fine to me