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

Replace shared pointers in AnyExecutable with raw pointers #164

Closed
wants to merge 2 commits into from

Conversation

jacquelinekay
Copy link
Contributor

In progress.

Potential fix for the weird pointer corruption bugs in ros2/system_tests#72 (still testing).

I broke AnyExecutable into a State class so that I can use a pure POD type for the lock-free queue in LockFreeExecutor. I can revert this part of the PR and create the class in LockFreeExecutor if need be.

@jacquelinekay jacquelinekay added the in progress Actively being worked on (Kanban column) label Dec 1, 2015
@jacquelinekay
Copy link
Contributor Author

In the future, execute_client, execute_service, and execute_timer will either need to accept a const shared pointer to be truly thread safe, or we need to add locking mechanisms to Client, Service and Timer to make sure the functions called in execution are thread safe.

@jacquelinekay jacquelinekay self-assigned this Dec 1, 2015
rclcpp::subscription::SubscriptionBase * subscription_intra_process;
rclcpp::timer::TimerBase * timer;
rclcpp::service::ServiceBase * service;
rclcpp::client::ClientBase * client;
Copy link
Member

Choose a reason for hiding this comment

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

How can we ensure that the pointers stored in here are still valid when the executable is actually being processed? The instance might have been deleted by then.

@tfoote
Copy link
Contributor

tfoote commented Dec 1, 2015

Ideas discussed today:

  • We can only put POD structures into lock-free queues. That's how we got to using raw pointers here.
  • Storing raw pointers is dangerous; we could consider some kind of mock object instead, but still have a problem of managing ownership and storage of the underlying data.
  • We're currently not queuing work, but rather relying on DDS to manage it. It's not ideal to introduce a queue on our side, irrespective of whether it's lock-free or not.

To make a decision on how to proceed here, we need to get a better idea of the requirements. @jacquelinekay, please write up a proposal for what is and what is not supported in a real-time-safe / lock-free manner, e.g.:

  • are message-based callbacks supported?
    • vs. just timers, similar to how OROCOS RTT works: a real-time-safe component can only ask to be invoked at a given frequency; when it wakes up, it is able to check for work
  • are multiple threads operating on a single subscriber supported?
    • vs. just single-threaded executors for real-time-safe work (could have implications on design of executor to allow, for example, a node with one real-time subscription and one non-real-time subscription)

@jacquelinekay
Copy link
Contributor Author

closing this set of changes, going to put the comments in a different issue

@jacquelinekay jacquelinekay removed the in progress Actively being worked on (Kanban column) label Dec 2, 2015
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.

3 participants