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.

Sign up for GitHub

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

Jump to bottom

Every executor gets its own SIGINT guard condition #308

Merged
merged 25 commits into from
Apr 19, 2019
Merged

Every executor gets its own SIGINT guard condition #308

merged 25 commits into from
Apr 19, 2019

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Apr 5, 2019

Implements @wjwwood's comment #192 (comment)

This moves signal handling code to a separate module _rclpy_signal_handler. Every executor adds a guard condition to a global list. The signal handler triggers all guard conditions in that list.

Resolves #192, and because of that this PR also allows multiple executors to wait in parallel again. This also resolves #287 by having each executor reuse the same guard condition.

It does not change the updated behavior of #253 described here: #253 (comment) . Maybe that should be an issue downstream of rclpy?

@sloretz sloretz added the in review Waiting for review (Kanban column) label Apr 5, 2019
@sloretz sloretz self-assigned this Apr 5, 2019
@sloretz
Copy link
Contributor Author

sloretz commented Apr 5, 2019 •
edited
Loading

CI (testing packages above rclpy, spelled --packages-above correctly this time)

  • Linux Build Status
    • rqt_py_common.test.test_rqt_common_unit.TestMessageTreeModel.test_path_names
    • test_cli.test_params_yaml.xunit.missing_result
  • Linux-aarch64 Build Status
    • rqt_py_common.test.test_rqt_common_unit.TestMessageTreeModel.test_path_names
  • macOS Build Status
    • rqt_py_common.test.test_rqt_common_unit.TestMessageTreeModel.test_path_names
  • Windows Build Status
    • rqt_py_common.test.test_rqt_common_unit.TestMessageTreeModel.test_path_names
    • test_cli.test_params_yaml.xunit.missing_result

@sloretz
Copy link
Contributor Author

sloretz commented Apr 5, 2019

CI looks good, just failures from ros2/build_farmer#166 and ros-visualization/rqt#192

hidmic
hidmic reviewed Apr 8, 2019
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

@sloretz how's that this is both thread- and signal-safe? Specially when it comes to manipulating a global list. Am I missing something?

rclpy/src/rclpy/_rclpy_signal_handler.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy_signal_handler.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy_signal_handler.c Outdated Show resolved Hide resolved
@sloretz
Copy link
Contributor Author

sloretz commented Apr 8, 2019

how's that this is both thread- and signal-safe? Specially when it comes to manipulating a global list. Am I missing something?

This relies on the GIL for thread safety. Neither function that edits the list releases the GIL. As long as they don't invoke anything that can call arbitrary python code (like getting an attribute off a python object, because it could have a custom __getattr__ that releases the GIL), there's no way these could touch the list at the same time.

Since the signal handler only reads the list, this tries to make sure g_guard_conditions can be read no matter where the code is interrupted. Edits are done by copying the current list, manipulating the copy, then swapping the original with the copy (assumes assigning to the global pointer is atomic so the signal handler doesn't fetch some invalid memory address).

@sloretz
Every executor gets its own SIGINT guard condition
dda3de0 
Moves signal handling code to _rclpy_signal_handler
Every executor adds a guard condition to a global list
SIGINT signal handler triggers all guard conditions in global list

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz force-pushed the sloretz/multiple_sigint_gcs branch from 6a14e45 to dda3de0 Compare April 8, 2019 15:37
@sloretz
_sigint_gc robust to shutdown() called twice
6690335 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Remove redundant comments
d48b195 
Comments say the same thing twice.
It only needs to be said once.
Remove extra comments so the same thing is not repeated.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Split loop for readability
506fb44 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
g_guard_conditions atomic variable
3538874 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Apr 8, 2019

CI

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

@hidmic
Copy link
Contributor

hidmic commented Apr 8, 2019

This relies on the GIL for thread safety.

I see. I thought we were not relying on the GIL on purpose. I've seen locks here and there (not in this PR) that seemed redundant with the GIL. Anyways, sounds good.

@sloretz
Use rclutils_atomics macros
a2b1aaf 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Apr 8, 2019

CI (testing packages above rclpy)

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

@sloretz
Call original handler before losing reference to it
3b250ab 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
remove extra unnecessary assignment
081c921 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Apr 8, 2019 •
edited
Loading

CI (testing packages above rclpy)

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

@sloretz sloretz added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Apr 10, 2019
@sloretz
g_guard_conditions is a struct on windows
6181a03 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Apr 10, 2019

CI (test packages above rclpy)

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

@sloretz
Copy link
Contributor Author

sloretz commented Apr 10, 2019

Blocked by ros2/rcutils#150

@sloretz
Copy link
Contributor Author

sloretz commented Apr 11, 2019

Quick check on windows with ros2/rcutils#150 to make sure warnings are gone Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Apr 11, 2019 •
edited
Loading

CI and moving back to review

@sloretz sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 11, 2019
jacobperron and others added 2 commits April 16, 2019 04:46
@jacobperron
Rename action state transitions (#300)
4802211 
* Rename action state transitions

Now using active verbs as described in the design doc:

http://design.ros2.org/articles/actions.html#goal-states

Connects to ros2/rcl#399.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@dirk-thomas
add missing error handling and cleanup (#315)
dcf1971 
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
dirk-thomas
dirk-thomas approved these changes Apr 18, 2019
View reviewed changes
rclpy/src/rclpy/_rclpy_signal_handler.c Outdated Show resolved Hide resolved
rclpy/rclpy/signals.py Outdated Show resolved Hide resolved
@sloretz
Don't store sigint_gc address
28d9f55 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
remove redundant conditional
3d25d7f 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Every executor gets its own SIGINT guard condition
f0a8dc6 
Moves signal handling code to _rclpy_signal_handler
Every executor adds a guard condition to a global list
SIGINT signal handler triggers all guard conditions in global list

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
_sigint_gc robust to shutdown() called twice
dd17671 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Remove redundant comments
4d706fe 
Comments say the same thing twice.
It only needs to be said once.
Remove extra comments so the same thing is not repeated.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Split loop for readability
c8da2f1 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
g_guard_conditions atomic variable
83e1edb 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Use rclutils_atomics macros
215fb99 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Call original handler before losing reference to it
63b2852 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
remove extra unnecessary assignment
f0dcb16 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
g_guard_conditions is a struct on windows
571470c 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Don't store sigint_gc address
b12ff20 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
remove redundant conditional
ea96b9f 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Merge branch 'sloretz/multiple_sigint_gcs' of https://github.com/ros2…
5e92624 
…/rclpy into sloretz/multiple_sigint_gcs
@sloretz
Copy link
Contributor Author

sloretz commented Apr 19, 2019 •
edited
Loading

CI (testing packages above rclpy)

@sloretz
Copy link
Contributor Author

sloretz commented Apr 19, 2019

CI looks good. Since the only changes were addressing CI comments I'll assume @dirk-thomas's approval still applies and merge this.

@sloretz sloretz merged commit afc4cd4 into master Apr 19, 2019
@sloretz sloretz deleted the sloretz/multiple_sigint_gcs branch April 19, 2019 18:02
@sloretz sloretz removed the in review Waiting for review (Kanban column) label Apr 19, 2019
This was referenced Apr 29, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic left review comments

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Assignees

@sloretz sloretz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

confuse about the sigint_gc's lifecycle Threads should have their own signal handler
5 participants
@sloretz @hidmic @wjwwood @dirk-thomas @jacobperron