[api review] Scoped Versus Non-Scoped Entities (e.g. Publishers/Subscriptions)
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 805
- Forks
- 564
- Avg merge
- 1d 17h
- Merged PRs (30d)
- 27
Description
Summary of the Issue
Currently, Publisher and Subscription (and similar entities) are scoped, meaning that when they are created they are added to the ROS graph as a side effect, and when they are let out of scope they remove themselves from the graph too.
Additionally, they necessarily have shared state with the system, for instance when you are spinning on a node, the executor shares ownership of the Subscriptions with the user.
Therefore, the Subscription only gets removed when both the user and executor are done with it.
This shared ownership is accomplished with the use of shared pointers and weak pointers.
There are a few concerns here, (a) use of shared pointers confuses users, (b) overhead of shared pointers and lack of an ability to use these classes on the stack rather than the heap, and (c) complexity of shutdown of an entity from the users perspective.
For (a), some users are overwhelmed by the need to use a shared pointer.
In ROS 1 this was avoided by having a class which itself just thinly wraps a shared pointer (see: https://github.com/ros/ros_comm/blob/ac9f88c59a676ca6895e13445fc7d71f398ebe1f/clients/roscpp/include/ros/subscriber.h#L108-L111).
This could be achieved in ROS 2 either by doing the same with a wrapper class (at the expense of lots of repeated code), or by eliminating the need for using shared ownership.
For (b), for some use cases, especially resource constrained / real-time / safety-critical environments, requiring these classes to be on the heap rather than the stack is at least inconvenient.
Additionally, there is a cost associated with using shared pointers, in the storage of shared state and in some implementation the use of locks or at least atomics for thread-safety.
For (c), this is the most concerning drawback, because right now when a user lets their shared pointer to a, for example, Subscription go out of scope, a post condition is not that the Subscription is destroyed, nor that it has been removed from the graph.
In stead, the behavior is more like "at some point in the future the Subscription will be destroyed and removed from the graph, when the system is done with it".
This isn't a very satisfactory contract, as some users may wish to know when the Subscription has been deleted, but cannot easily know that.
The benefit to the shared state is a safety net for users.
The alternative would be to document that a Subscription, again for example, cannot be deleted until the system is done with it.
We'd basically be pushing the responsibility onto the user to ensure the shared ownership is handled properly by the execution of their application, i.e. they create the Subscription, share a reference with the system (adding it by reference to an executor, for example), and they have to make sure the system is done with it before deleting the Subscription.
Separately, from the above points, there is the related concern of forcing the user to keep a copy of their entities in scope, whether it be with a shared pointer or a class wrapping one.
There is the desire to create it and forget it in some cases.
The downside to this is that if/when the user wants to destroy the entity, they have no way of doing that as they have no handle or unique way to address the entity.
One proposed solution would be to have a set of "named X" APIs, e.g. create_named_subscription rather than just create_subscription.
This would allow the user to address the Subscription in the future in order to obtain a new reference to it or delete it.
Related Issues
The suggested action from the API review was to summarize this to one issue and defer until later.
Below are some notes from the api review meeting.
Notes from 2020-03-23:
- (chris) Putting ownership mechanics on user is hard.
- (dirk) add documentation clearly outlining ownership
- (shane) warn on unused to catch issues with immediately deleted items
- (tfoote) debugging output for destruction so it easy to see when reviewing logs
- (chris) possible to create API that checks for destruction
- (william) might lead to complex synchronization issues
- (tfoote) could add helper classes to make scoped things non-scoped
- (shane) concerned that there is no longer "one good way" to do it
This is a result of the API review done in March 2020:
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with rclcpp/doc/api_review_march_2020.md and the related issues 506 and 726, then review the scoped entity entry points such as create_subscription and the proposed create_named_subscription. The issue needs a settled ownership and lifetime API decision before implementation scope, affected files, tests, and completion criteria can be identified.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend-api-design
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100