Thread-safe access to SubscriptionData #288
Open
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: Yadunund <[email protected]>
…imitation Signed-off-by: Yadunund <[email protected]>
Yadunund
force-pushed
the
yadu/raii-sub
branch
from
619b33f
to
71bbb6a
Compare
clalancette
requested changes
Oct 3, 2024
| @@ -237,8 +237,12 @@ std::string zid_to_str(const z_id_t & id); | |||
| } // namespace liveliness | |||
|
|
|||
| ///============================================================================= | |||
| // Helper function to generate a randon GID. | |||
| /// Helper function to generate a randon GID. | |||
There was a problem hiding this comment.
Suggested change
| /// Helper function to generate a randon GID. | |
| /// Helper function to generate a random GID. |
|
|
||
| ///============================================================================= | ||
| /// Generate a hash for a given GID. | ||
| size_t hash_gid(const uint8_t * gid); |
There was a problem hiding this comment.
I know you are just copying this over, but I'm going to suggest that we change this signature to:
Suggested change
| size_t hash_gid(const uint8_t * gid); | |
| size_t hash_gid(const uint8_t gid[RMW_GID_STORAGE_SIZE]); |
It won't make much of a practical difference, but it will be clear to the reader exactly what the size of gid is.
| @@ -69,12 +69,14 @@ rmw_publisher_event_init( | |||
| rmw_event->event_type = event_type; | |||
|
|
|||
| // Register the event with graph cache. | |||
| std::weak_ptr<rmw_zenoh_cpp::PublisherData> data_wp = pub_data; | |||
There was a problem hiding this comment.
Is this a separate bugfix?
| zenoh_event_type, | ||
| [sub_data, | ||
| zenoh_event_type](std::unique_ptr<rmw_zenoh_cpp::rmw_zenoh_event_status_t> zenoh_event) | ||
| { | ||
| // auto sub_data = data_wp.lock(); |
There was a problem hiding this comment.
Suggested change
| // auto sub_data = data_wp.lock(); |
Comment on lines
+973
to
+978
| // Store type erased node in rmw_subscription->data so that the | ||
| // Subscription can be safely accessed. | ||
| // TODO(Yadunund): We cannot store the rmw_node_t * here since this type erased | ||
| // subscription handle will be returned in the rmw_subscriptions_t in rmw_wait | ||
| // from which we cannot obtain SubscriptionData. | ||
| // rmw_subscription->data = reinterpret_cast<void *>(const_cast<rmw_node_t *>(node)); |
There was a problem hiding this comment.
Suggested change
| // Store type erased node in rmw_subscription->data so that the | |
| // Subscription can be safely accessed. | |
| // TODO(Yadunund): We cannot store the rmw_node_t * here since this type erased | |
| // subscription handle will be returned in the rmw_subscriptions_t in rmw_wait | |
| // from which we cannot obtain SubscriptionData. | |
| // rmw_subscription->data = reinterpret_cast<void *>(const_cast<rmw_node_t *>(node)); | |
| // TODO(Yadunund): We cannot store the rmw_node_t * here since this type erased | |
| // subscription handle will be returned in the rmw_subscriptions_t in rmw_wait | |
| // from which we cannot obtain SubscriptionData. |
Comment on lines
+2760
to
+2761
| rmw_zenoh_cpp::SubscriptionData * sub_data = | ||
| static_cast<rmw_zenoh_cpp::SubscriptionData *>(subscriptions->subscribers[i]); |
There was a problem hiding this comment.
I think we still want to check whether sub_data is NULL before doing anything:
Suggested change
| rmw_zenoh_cpp::SubscriptionData * sub_data = | |
| static_cast<rmw_zenoh_cpp::SubscriptionData *>(subscriptions->subscribers[i]); | |
| rmw_zenoh_cpp::SubscriptionData * sub_data = | |
| static_cast<rmw_zenoh_cpp::SubscriptionData *>(subscriptions->subscribers[i]); | |
| if (sub_data == nullptr) { | |
| continue; | |
| } |
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.
This PR introduces SubscriptionData class that manages the lifetime of Zenoh artifacts. It makes access to class members thread-safe where possible. Some z_closure callbacks still work with type erased raw ptrs but this will be addressed when we migrate to zenoh-cpp.
It seems like it is not feasible to store rmw_node_t * as type erased void * in rmw_subscription_t->data like we did with rmw_publisher_t->data .
The problem is that because rmw_subscriptions_t in rmw_wait contains an array of the type erased subscriptions handles (ie, rmw_subscription_t->data ), there is no way to get the SubscriptionData::SharedPtr here
rmw_zenoh/rmw_zenoh_cpp/src/rmw_zenoh.cpp
Lines 3220 to 3221 in cf8e8a2
The only option we have is storing a rawptr to SubscriptionData in rmw_subscription_t->data but leads to the risk of it becoming a dangling pointer when the SubscriptionData::SharedPtr is deleted.