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_node thread safe #330

Merged
merged 10 commits into from
Apr 30, 2019
Merged

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Apr 29, 2019

Blocks #319

I was going to make this part of #319, but I think it would be easier to review separately. This makes Node.handle use the Handle class from #308. It also adds handle.requires(another_handle) to make sure handle is destroyed before another_handle.

Problem this solves

#308 makes destroy_subscription thread safe by delaying destruction if the handle is currently being used. This makes it possible for a subscription to be destroyed after the node; which crashes when doing the same strategy with services using fast-rtps. See #319 (comment) for a better explanation.

Breaking changes

  • Previously node.destroy_node() either returned True or raised an exception. I removed the return value since it was only ever True.
  • Previously node.handle was set to None after the node was destroyed. Now it raises InvalidHandle when someone tries to use it after it is destroyed.
  • previously node.handle was a pycapsule and could be used directly. Now one must use:
    with node.handle as node_capsule:
       ...

How handle.requires() works

Subscriptions must be destroyed before the node that created them, so

subscription.handle.requires(node.handle)

When node.handle.destroy() is called the subscription handle is destroyed first. Once the node handle is no longer being used it will mark itself invalid so no one else can use it. Then the node handle tells the subscription handle to destroy itself. Once the subscription handle is no longer in use it will delete itself, and then tell the node it can destroy itself.

If the user does not call destroy() on either handle, the subscription handle holds a reference to the node handle so the subscription handle will be garbage collected first. The node handle only holds a weak reference to the subscription handle.

Next steps

After incorporating feedback, assuming this approach is OK I would like to merge this PR. Then I'll rebase #319 to make all the other entities require the node so everything can be destroyed safely.

sloretz added 4 commits April 29, 2019 14:51
Signed-off-by: Shane Loretz <[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
@sloretz sloretz added the in review Waiting for review (Kanban column) label Apr 29, 2019
@sloretz sloretz self-assigned this Apr 29, 2019
@sloretz
Copy link
Contributor Author

sloretz commented Apr 29, 2019

CI (testing all packages above rclpy)

@sloretz sloretz mentioned this pull request Apr 29, 2019
6 tasks
@sloretz sloretz added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Apr 30, 2019
@sloretz sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 30, 2019
@sloretz
Copy link
Contributor Author

sloretz commented Apr 30, 2019

CI (testing just rclpy)

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

# Assume lock is held
assert not self.__lock.acquire(blocking=False)
# Assume capsule has not been destroyed
assert self.__valid
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Destruction can be delayed waiting for dependents. Previously self.__valid = False meant the capsule itself has been destroyed. Now setting it to false means destroy() was called and __use_count hit zero. The purpose is to prevent new users of this handle while waiting for dependent handles to be destroyed.

@sloretz
Copy link
Contributor Author

sloretz commented Apr 30, 2019

CI (testing rclpy)

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

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

LGTM (after convincing myself I understood how the call hierarchy happens)

Signed-off-by: Shane Loretz <[email protected]>
@sloretz
Copy link
Contributor Author

sloretz commented Apr 30, 2019

CI (testing rclpy with 8c95ea6)

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

@sloretz sloretz merged commit 1b4728b into master Apr 30, 2019
@delete-merged-branch delete-merged-branch bot deleted the sloretz/thread_safe_destroy_node branch April 30, 2019 19:40
@sloretz sloretz removed the in review Waiting for review (Kanban column) label Apr 30, 2019
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.

2 participants