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

avoid SIGINT on Windows #94

Merged
merged 1 commit into from
Jun 27, 2018
Merged

avoid SIGINT on Windows #94

merged 1 commit into from
Jun 27, 2018

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jun 27, 2018

This is required to avoid a (non-fatal, but annoying) traceback on Windows when launch tries to send SIGINT to it's children.

This patch automatically escalates to SIGTERM (which does work), which is what the old launch did, and is also what eventually happens with launch now, but only after waiting five seconds. This removes the wait as well.

Eventually this might be fixed with some combination of creationflags and signal.CTRL_C_EVENT, but that's an ongoing issue with subprocess on Windows.

@wjwwood wjwwood added bug Something isn't working in review Waiting for review (Kanban column) labels Jun 27, 2018
@wjwwood wjwwood added this to the bouncy milestone Jun 27, 2018
@wjwwood wjwwood self-assigned this Jun 27, 2018
@wjwwood
Copy link
Member Author

wjwwood commented Jun 27, 2018

Before:

C:\dev\ros2>ros2 launch demo_nodes_cpp add_two_ints.launch.py
[INFO] [launch]: process[add_two_ints_server.EXE-1]: started with pid [12376]
[INFO] [launch]: process[add_two_ints_client.EXE-2]: started with pid [7220]
[INFO] [add_two_ints_server]: Incoming request
a: 2 b: 3
[INFO] [add_two_ints_client]: Result of add_two_ints: 5
[INFO] [launch]: process[add_two_ints_client.EXE-2]: process has finished cleanly
[INFO] [launch]: sending signal 'SIGINT' to process[add_two_ints_server.EXE-1]
[ERROR] [asyncio]: Task exception was never retrieved
future: <Task finished coro=<LaunchService._process_one_event() done, defined at C:\dev\ros2\install\Lib\site-packages\launch\launch_service.py:176> exception=ValueError('Unsupported signal: 2',)>
Traceback (most recent call last):
  File "C:\dev\ros2\install\Lib\site-packages\launch\launch_service.py", line 178, in _process_one_event
    await self.__process_event(next_event)
  File "C:\dev\ros2\install\Lib\site-packages\launch\launch_service.py", line 197, in __process_event
    visit_all_entities_and_collect_futures(entity, self.__context))
  File "C:\dev\ros2\install\Lib\site-packages\launch\utilities\visit_all_entities_and_collect_futures_impl.py", line 38, in visit_all_entities_and_collect_futures
    sub_entities = entity.visit(context)
  File "C:\dev\ros2\install\Lib\site-packages\launch\action.py", line 39, in visit
    return self.execute(context)  # type: ignore
  File "C:\dev\ros2\install\Lib\site-packages\launch\actions\opaque_function.py", line 72, in execute
    return self.__function(context, *self.__args, **self.__kwargs)
  File "C:\dev\ros2\install\Lib\site-packages\launch\actions\execute_process.py", line 249, in __on_signal_process_event
    self._subprocess_transport.send_signal(typed_event.signal)
  File "c:\python36\lib\asyncio\base_subprocess.py", line 152, in send_signal
    self._proc.send_signal(signal)
  File "c:\python36\lib\subprocess.py", line 1131, in send_signal
    raise ValueError("Unsupported signal: {}".format(sig))
ValueError: Unsupported signal: 2
[ERROR] [launch]: process[add_two_ints_server.EXE-1] failed to terminate '5' seconds after receiving 'SIGINT', escalating to 'SIGTERM'
[INFO] [launch]: sending signal 'SIGTERM' to process[add_two_ints_server.EXE-1]
[ERROR] [launch]: process[add_two_ints_server.EXE-1] process has died [pid 12376, exit code 1, cmd 'C:\dev\ros2\install\lib\demo_nodes_cpp\add_two_ints_server.EXE'].

After:

C:\dev\ros2>ros2 launch demo_nodes_cpp add_two_ints.launch.py
[INFO] [launch]: process[add_two_ints_server.EXE-1]: started with pid [11484]
[INFO] [launch]: process[add_two_ints_client.EXE-2]: started with pid [1344]
[INFO] [add_two_ints_server]: Incoming request
a: 2 b: 3
[INFO] [add_two_ints_client]: Result of add_two_ints: 5
[INFO] [launch]: process[add_two_ints_client.EXE-2]: process has finished cleanly
[WARNING] [launch]: 'SIGINT' sent to process[add_two_ints_server.EXE-1] not supported on Windows, escalating to 'SIGTERM'
[INFO] [launch]: sending signal 'SIGTERM' to process[add_two_ints_server.EXE-1]
[ERROR] [launch]: process[add_two_ints_server.EXE-1] process has died [pid 11484, exit code 1, cmd 'C:\dev\ros2\install\lib\demo_nodes_cpp\add_two_ints_server.EXE'].

@dhood
Copy link
Member

dhood commented Jun 27, 2018

For the example counter that ignores sigterms, because we don't cancel the sigterm timer that's already been scheduled, it's going to have a misleading error message printed 5s after sigint which will again say that sigint is being escalated to sigterm. Shall we cancel that timer to prevent the second sigterm and the message?

@nuclearsandwich
Copy link
Member

I also tested this on Windows and had the same before and after output as William.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 27, 2018

We cannot cancel the SIGTERM timer from there because we don't know if SIGINT was sent by the user (using the EmitEvent action) or as part of the escalating shutdown.

I don't think it will be misleading because it will simply send SIGTERM twice, and the first time the warning message will indicate that is because it cannot send SIGINT as requested.

Also, the counter doesn't work right on Windows because you cannot ignore SIGTERM on Windows either. This is because Windows just emulates SIGTERM as a signal and instead just calls a terminate function on the process which cannot be handle that way I think. I'll have to rethink the behavior on Windows w.r.t. to shutting down processes programmatically and likely rewrite the counter.py example, but this is an incremental improvement only. It just avoids the traceback and ensures the process actually gets terminated.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 27, 2018

I still need a code review so that I can run CI.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 27, 2018

Thanks for the review @dhood!

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood wjwwood merged commit 1ac8111 into master Jun 27, 2018
@wjwwood wjwwood deleted the avoid_sigint_on_windows branch June 27, 2018 21:59
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants