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

Make destroy_subscription thread safe #318

Merged
merged 7 commits into from
Apr 24, 2019

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Apr 23, 2019

Resolves #255

This makes node.destroy_subscription thread safe by wrapping the pycapsule with a class that keeps track of who is using it. The subscription is destroyed when it is no longer being used.

Potentially breaking changes

  • _rclpy.rclpy_create_subscription() returns just the pycapsule instead of a tuple. However, I would be surprised if any downstream projects are using this method.
  • _rclpy.rclpy_destroy_node_entity() no longer destroys subscriptions. Instead rclpy_pycapsule_destroy() does the work. Again I would be surprised if any downstream code is using this.
  • The class Subscription no longer has attributes subscription_handle and subscription_pointer. Instead it just has handle. I would expect those attributes to have been for internal use only.

Next steps

In order to keep PRs small, I would like this PR to get review and be merged after addressing feedback. Afterwards I'll create a new PR to fix the same thread safety issues for the other destroy methods:

  • destroy_publisher
  • destroy_client
  • destroy_service
  • destroy_timer
  • destroy_guard_condition

Finally I want to defer making destroy_node thread safe, and instead create an issue for it. It requires more thought since it can't be destroyed until every entity created from is no longer being used and has been destroyed.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz added the in review Waiting for review (Kanban column) label Apr 23, 2019
@sloretz sloretz self-assigned this Apr 23, 2019
@sloretz
Copy link
Contributor Author

sloretz commented Apr 23, 2019

CI (testing packages above rclpy)

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I had a few comments, but in general lgtm.

rclpy/rclpy/handle.py Show resolved Hide resolved
rclpy/src/rclpy/_rclpy_pycapsule.c Show resolved Hide resolved
rclpy/src/rclpy/_rclpy_pycapsule.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy_pycapsule.c Outdated Show resolved Hide resolved
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Apr 23, 2019

CI (testing just rclpy, mostly to check that _Static_assert is supported on all platforms)

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

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

sloretz commented Apr 23, 2019

CI with 69399f5 testing packages above rclpy to make sure the new commits didn't break anything

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.

executor.spin_once crashes after destroy_subscription called in separate thread
2 participants