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

Register our signal handler in rclpy_init vs each wait #194

Merged
merged 3 commits into from
Jun 24, 2018

Conversation

dhood
Copy link
Member

@dhood dhood commented Jun 22, 2018

This is my current thinking towards addressing ros2/launch#88

Opensplice python nodes would crash on sigint when calling the original signal handler. It is suspected that the signal handler by that point is changed to one that is opensplice specific, which is not what we are intending to call (we are intending to call the python signal module's handler).

I suspect that the point at which opensplice registers their signal handler is when a node is created, because if the creation of launch_ros' rclpy node is skipped (but we still call spin), then the crash on shutdown doesn't occur.

Registering the signal handler in rclpy_init (and only in init) matches the behaviour in rclcpp: https://github.com/ros2/rclcpp/blob/07e5be76218bd830c787b1d5c4af12e9eb6d5f6e/rclcpp/src/rclcpp/utilities.cpp#L187

This is in progress as I do more testing but feedback on the approach is appreciated

@dhood dhood added the in progress Actively being worked on (Kanban column) label Jun 22, 2018
@dhood dhood self-assigned this Jun 22, 2018
@dhood dhood added this to the bouncy milestone Jun 22, 2018
@dhood
Copy link
Member Author

dhood commented Jun 22, 2018

I missed @wjwwood 's comment in #192 (comment) that also suggested that this is a reasonable approach.

I've testing this on windows, osx and xenial and it's behaving as expected.

One thing I noticed was that with opensplice on windows the return code is set to 3221225786, which according to this page just indicates the program was stopped with ctrl-c. That seems reasonable to me (note: the following invocation was including a workaround for ros2/launch#91)

C:\J\workspace\ci_packaging_windows\ws>ros2 launch demo_nodes_cpp talker_listener.launch.py
[INFO] [launch]: process[talker.EXE-1]: started with pid [10812]
[INFO] [launch]: process[listener.EXE-2]: started with pid [5184]
[INFO] [talker]: Publishing: 'Hello World: 1'
[INFO] [listener]: I heard: [Hello World: 1]
[INFO] [talker]: Publishing: 'Hello World: 2'
[INFO] [listener]: I heard: [Hello World: 2]
[INFO] [talker]: Publishing: 'Hello World: 3'
[INFO] [listener]: I heard: [Hello World: 3]
[INFO] [talker]: Publishing: 'Hello World: 4'
[INFO] [listener]: I heard: [Hello World: 4]
[WARNING] [launch.LaunchService]: user interrupted with ctrl-c (SIGINT)
ssignal_handler(2)
[WARNING] [launch.LaunchService]: user interrupted with ctrl-c (SIGINT) again, ignoring...
ignal_handler(2)
[ERROR] [launch]: process[talker.EXE-1] process has died [pid 10812, exit code 3221225786, cmd 'C:\J\workspace\ci_packag
ing_windows\ws\install\lib\demo_nodes_cpp\talker.EXE'].
[ERROR] [launch]: process[listener.EXE-2] process has died [pid 5184, exit code 3221225786, cmd 'C:\J\workspace\ci_packa
ging_windows\ws\install\lib\demo_nodes_cpp\listener.EXE'].

C:\J\workspace\ci_packaging_windows\ws>ros2 launch demo_nodes_cpp talker_listener.launch.py
[INFO] [launch]: process[talker.EXE-1]: started with pid [9456]
[INFO] [launch]: process[listener.EXE-2]: started with pid [12252]
[INFO] [talker]: Publishing: 'Hello World: 1'
[INFO] [listener]: I heard: [Hello World: 1]
signal_handler(2)
signal_handler(2)
[WARNING] [launch.LaunchService]: user interrupted with ctrl-c (SIGINT)
[WARNING] [launch.LaunchService]: user interrupted with ctrl-c (SIGINT)
[ERROR] [launch]: process[listener.EXE-2] process has died [pid 12252, exit code 3221225786, cmd 'C:\J\workspace\ci_pack
aging_windows\ws\install\lib\demo_nodes_cpp\listener.EXE'].
[ERROR] [launch]: process[talker.EXE-1] process has died [pid 9456, exit code 3221225786, cmd 'C:\J\workspace\ci_packagi
ng_windows\ws\install\lib\demo_nodes_cpp\talker.EXE'].

So I will run CI on this and put it in review

@dhood
Copy link
Member Author

dhood commented Jun 23, 2018

Full CI, including opensplice, before 8573fe7

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

CI after 8573fe7 including packages that call shutdown:

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

@dhood
Copy link
Member Author

dhood commented Jun 23, 2018

windows looks alright but I'm not expecting linux to come back green because I'm seeing a lot of failing launch tests.

@dhood
Copy link
Member Author

dhood commented Jun 23, 2018

The issue was 69c19df: I had been testing with processes that were always calling wait and so had guard conditions set up, but that's not the case for processes that just have publishers, for example.

New CI (full, including opensplice):

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

@dhood dhood merged commit 33feb4e into master Jun 24, 2018
@dhood dhood removed the in progress Actively being worked on (Kanban column) label Jun 24, 2018
@dhood dhood deleted the signal_handler_in_init branch June 24, 2018 05:23
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.

2 participants