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

Thread-safe access to PublisherData #278

Merged
merged 7 commits into from
Oct 1, 2024
Merged

Thread-safe access to PublisherData #278

merged 7 commits into from
Oct 1, 2024

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented Sep 9, 2024

This PR replaces class rmw_publisher_data_t with class PublisherData which provides thread-safe access to publisher functions. An instance of PublisherData is created via the NodeData class. rmw_publisher->data is now set to rmw_node_t. The assumption here is that the node outlives the publisher similar to the assumption that the context outlives the node. From this rmw_node_t raw_ptr, we are able to obtain it's NodeData and then lookup the PublisherData for the publisher.

One nice thing this PR introduces is the mechanism where shutting down or destroying a parent entity, will shutdown the child entities. Eg. If rclcpp::shutdown() is invoked, it will first shutdown all the PublisherData, then NodeData, and finally the context. We also check for shutdown in the destructor. This is helpful when someone calls node.reset there by destroying an rclcpp::Node; this will now shutdown all child entities (only PublisherData so far) and then shutdown the node before clearing memory. We also check for is_shutdown before creating a publisher, publishing a message, etc. These principles will be extended to other entities in subsequent PRs.

@Yadunund Yadunund force-pushed the yadu/raii-pub branch 5 times, most recently from 47a876c to 5b6ee63 Compare September 27, 2024 23:34
@Yadunund Yadunund marked this pull request as ready for review September 27, 2024 23:38
@Yadunund Yadunund changed the title Create PublisherData within NodeData Thread-safe access to PublisherData Sep 27, 2024
rmw_zenoh_cpp/src/detail/rmw_context_impl_s.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Show resolved Hide resolved
Base automatically changed from yadu/raii-node to rolling September 30, 2024 17:03
rmw_zenoh_cpp/src/detail/liveliness_utils.cpp Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/rmw_publisher_data.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/rmw_publisher_data.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

just some mirmo things, but overall LGTM

rmw_zenoh_cpp/src/detail/graph_cache.hpp Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/liveliness_utils.hpp Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/liveliness_utils.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/rmw_context_impl_s.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp Outdated Show resolved Hide resolved
Comment on lines 397 to 398
std::lock_guard<std::mutex> lock(mutex_);
return gid_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because this is returning a raw pointer, this is probably not thread-safe.

That said, from what I can tell there is only one caller of this API, from rmw_get_gid_for_publisher. And there, what we are doing is copying the data into an out parameter.

That suggests to me that this function should be something like:

const void PublisherData::copy_gid(rmw_gid_t * gid) const
{
  std::lock_guard<std::mutex> lock(mutex_);
  memcpy(gid->data, gid_, RMW_GID_STORAGE_SIZE);
}

(and then update the caller down below as needed). That way we are doing the copy under the lock, which should be safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Also had to do something similar for copying the GID into the attachment map 4edbee2

Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
@Yadunund Yadunund merged commit 9bf5b74 into rolling Oct 1, 2024
8 checks passed
@Yadunund Yadunund deleted the yadu/raii-pub branch October 1, 2024 18:31
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