-
Notifications
You must be signed in to change notification settings - Fork 35
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 graph cache #258
Conversation
03c4ee1
to
a9b34e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a few things to think about.
eb936da
to
f6d3ab1
Compare
@Yadunund Friendly ping to follow up on this issue. |
Discussion from maintenance triage: Decided to assign this issue to @Yadunund |
I think this is what I'm struggling with most in this change. It just doesn't seem right to me to be having one giant class that encapsulates all functionality of the RMW. It does fix the locking problem, but it doesn't seem very elegant. If we ignore the threading problem for the moment, in my ideal world we'd have a Now we have to think about the locking. If we just did the above, it wouldn't be much different from what we have today, and it wouldn't fix our locking problems. So what we need is for each entity to be able to query its "parent" on whether it is still alive. For instance, in Thus, the I admit I haven't totally thought through all of the implications here. But I think this proposal creates a class hierarchy, but also allows us to add in additional checking/locking. What do you think of the general idea? |
Thanks for the additional thoughts here. I agree such a hierarchy would be ideal. What i'm not clear about is whether In any case it seems like this is a bridge we will need to cross only after we merge this PR; perhaps when addressing #259? So should we move the discussion there since the changes here simple make |
Signed-off-by: Yadunund <[email protected]>
…is removed Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
959da06
to
f106c17
Compare
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
In 7cc52bd, I updated the |
Signed-off-by: Yadunund <[email protected]>
7cc52bd
to
b3a8ef1
Compare
Signed-off-by: Yadunund <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]> Signed-off-by: Yadu <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor fixes, otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a rebase to fix conflicts, and we should fix @ahcorde 's comments, but this otherwise looks good to me.
Signed-off-by: Yadunund <[email protected]>
3c1c6e4
to
f569818
Compare
Signed-off-by: Yadunund <[email protected]>
f569818
to
5eb967c
Compare
* Make rmw_context_impl_s an RAII class Signed-off-by: Yadunund <[email protected]> * fix regression with graph_guard_condition not triggering when entity is removed Signed-off-by: Yadunund <[email protected]> * Have the context create the zenoh artifacts Signed-off-by: Yadunund <[email protected]> * Add comment for session() api Signed-off-by: Yadunund <[email protected]> * Style Signed-off-by: Yadunund <[email protected]> * Add api to register querying_sub cb in rmw_context_impl_s Signed-off-by: Yadunund <[email protected]> * Have rmw_context_impl_s return a shared_ptr to GraphCache Signed-off-by: Yadunund <[email protected]> * Add todo on thread safety Signed-off-by: Yadunund <[email protected]> * Update rmw_zenoh_cpp/src/detail/rmw_context_impl_s.cpp Co-authored-by: Chris Lalancette <[email protected]> Signed-off-by: Yadu <[email protected]> * Address feedback Signed-off-by: Yadunund <[email protected]> * Do not use allocator for creating graph_guard_condition Signed-off-by: Yadunund <[email protected]> * Address feedback Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]> Signed-off-by: Yadu <[email protected]> Co-authored-by: Chris Lalancette <[email protected]>
* Make rmw_context_impl_s an RAII class Signed-off-by: Yadunund <[email protected]> * fix regression with graph_guard_condition not triggering when entity is removed Signed-off-by: Yadunund <[email protected]> * Have the context create the zenoh artifacts Signed-off-by: Yadunund <[email protected]> * Add comment for session() api Signed-off-by: Yadunund <[email protected]> * Style Signed-off-by: Yadunund <[email protected]> * Add api to register querying_sub cb in rmw_context_impl_s Signed-off-by: Yadunund <[email protected]> * Have rmw_context_impl_s return a shared_ptr to GraphCache Signed-off-by: Yadunund <[email protected]> * Add todo on thread safety Signed-off-by: Yadunund <[email protected]> * Update rmw_zenoh_cpp/src/detail/rmw_context_impl_s.cpp Co-authored-by: Chris Lalancette <[email protected]> Signed-off-by: Yadu <[email protected]> * Address feedback Signed-off-by: Yadunund <[email protected]> * Do not use allocator for creating graph_guard_condition Signed-off-by: Yadunund <[email protected]> * Address feedback Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]> Signed-off-by: Yadu <[email protected]> Co-authored-by: Chris Lalancette <[email protected]>
Step 1 in addressing #249.
This PR updates the
rmw_context_impl_s
class to accurately manage the lifetime of its members while ensure thread-safe data access. For starters, it ensures that the Graph Cache updates and lookups are thread safe.In subsequent PRs, I will update the
rmw_context_impl_s
class to storermw_publisher_data_t
, etc with member functions to manage/access their functionalities.