-
Notifications
You must be signed in to change notification settings - Fork 225
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
Every executor gets its own SIGINT guard condition #308
Conversation
CI looks good, just failures from ros2/build_farmer#166 and ros-visualization/rqt#192 |
There was a problem hiding this 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?
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 Since the signal handler only reads the list, this tries to make sure |
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>
6a14e45
to
dda3de0
Compare
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
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>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
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. |
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Blocked by ros2/rcutils#150 |
Quick check on windows with ros2/rcutils#150 to make sure warnings are gone |
CI and moving back to review
|
* 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>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
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>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
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>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
…/rclpy into sloretz/multiple_sigint_gcs
CI (testing packages above rclpy)
|
CI looks good. Since the only changes were addressing CI comments I'll assume @dirk-thomas's approval still applies and merge this. |
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?