-
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
Make destroy_subscription thread safe #318
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
CI (testing packages above rclpy)
|
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
wjwwood
approved these changes
Apr 23, 2019
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.
I had a few comments, but in general lgtm.
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
wjwwood
approved these changes
Apr 23, 2019
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
CI with 69399f5 testing packages above rclpy to make sure the new commits didn't break anything
|
delete-merged-branch
bot
deleted the
sloretz/thread_safe_destroy_subscription
branch
April 24, 2019 14:35
This was referenced Apr 24, 2019
This was referenced Jul 29, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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. Insteadrclpy_pycapsule_destroy()
does the work. Again I would be surprised if any downstream code is using this.Subscription
no longer has attributessubscription_handle
andsubscription_pointer
. Instead it just hashandle
. 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:
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.