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

Use try_shutdown() instead of shutdown() in DirectNode.__exit__() #683

Merged
merged 1 commit into from
Dec 30, 2021
Merged

Use try_shutdown() instead of shutdown() in DirectNode.__exit__() #683

merged 1 commit into from
Dec 30, 2021

Conversation

ivanpauno
Copy link
Member

There was a change in signal handling, and now SIGINT/SIGTERM will automatically trigger a shutwon request.

This modifies DirectNode.__exit__() to use try_shutdown(), in order to not get a "context already shutdown" error when a signal is sent.

I manually tested with the following example and the issue is gone:

import sys
from ros2cli.node.direct import DirectNode

args = sys.argv[1:]

with DirectNode(args) as node:
    from rclpy.executors import SingleThreadedExecutor
    executor = SingleThreadedExecutor()
    executor.add_node(node)
    executor.spin_once(timeout_sec=None)

New error message after ctrl-c:

^CTraceback (most recent call last):
  File "audrow_example.py", line 10, in <module>
    executor.spin_once(timeout_sec=None)
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/executors.py", line 704, in spin_once
    handler, entity, node = self.wait_for_ready_callbacks(timeout_sec=timeout_sec)
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/executors.py", line 690, in wait_for_ready_callbacks
    return next(self._cb_iter)
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/executors.py", line 588, in _wait_for_ready_callbacks
    wait_set.wait(timeout_nsec)
KeyboardInterrupt

@ivanpauno ivanpauno added the enhancement New feature or request label Dec 21, 2021
@ivanpauno ivanpauno self-assigned this Dec 21, 2021
clalancette
clalancette approved these changes Dec 21, 2021
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI. Thanks @ivanpauno

@ivanpauno
Copy link
Member Author

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

@ivanpauno
Copy link
Member Author

ivanpauno commented Dec 21, 2021 •
edited
Loading

The Rpr checker results don't look great ....
I will try to reproduce locally

audrow
audrow approved these changes Dec 21, 2021
View reviewed changes
Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

This fixes the error for me, too. Looks good with green CI.

@ivanpauno ivanpauno mentioned this pull request Dec 22, 2021
Merged
@ivanpauno
Copy link
Member Author

CI again after ros2/rclpy#868 got merged:

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

@clalancette
Copy link
Contributor

Just as an FYI, I retested the original problem with ros2/rclpy#868 and this applied, and it seems to succeed for me here. So 👍 from me, thanks!

@ivanpauno
Copy link
Member Author

I have made a new rclpy release, I will rerun the Rpr checker in a few hours, it should pass this time.

@ivanpauno
Copy link
Member Author

@ros-pull-request-builder retest this please

@clalancette
Copy link
Contributor

Looks like it is still rebuilding: https://build.ros2.org/view/Queue/

@audrow
Copy link
Member

audrow commented Dec 30, 2021

@ros-pull-request-builder retest this please

@audrow
Copy link
Member

audrow commented Dec 30, 2021 •
edited
Loading

I'm rerunning CI, just to confirm things still work.

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

@audrow
Copy link
Member

audrow commented Dec 30, 2021 •
edited
Loading

Thanks for the fix, @ivanpauno! I'm going to merge this in so that I can run CI for switching the default DDS: ros2/rmw#315

@audrow audrow merged commit 53fb882 into master Dec 30, 2021
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/direct-node-try-shutdown branch December 30, 2021 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@audrow audrow audrow approved these changes

@sloretz sloretz Awaiting requested review from sloretz

Assignees

@ivanpauno ivanpauno

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivanpauno @clalancette @audrow