-
Notifications
You must be signed in to change notification settings - Fork 417
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
Avoid copying AnySubscriptionCallback objects for intraprocess comms #916
Avoid copying AnySubscriptionCallback objects for intraprocess comms #916
Conversation
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
@hidmic @nuclearsandwich @wjwwood as today is the deadline for bug fixes for Eloquent (IIRC), could this be considered? It fixes something that should have worked after #789. |
@@ -153,7 +153,7 @@ class SubscriptionIntraProcess : public SubscriptionIntraProcessBase | |||
} | |||
} | |||
|
|||
AnySubscriptionCallback<CallbackMessageT, Alloc> any_callback_; | |||
AnySubscriptionCallback<CallbackMessageT, Alloc> & any_callback_; |
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.
@christophebedard I'm somewhat concerned about the lifetime of SubscriptionIntraProcess
w.r.t Subscription
and thus any_callback_
storage. I see we give away references to SubscriptionIntraProcess
via get_intra_process_waitable()
:
rclcpp/rclcpp/src/rclcpp/subscription_base.cpp
Lines 179 to 196 in b92db52
rclcpp::Waitable::SharedPtr | |
SubscriptionBase::get_intra_process_waitable() const | |
{ | |
// If not using intra process, shortcut to nullptr. | |
if (!use_intra_process_) { | |
return nullptr; | |
} | |
// Get the intra process manager. | |
auto ipm = weak_ipm_.lock(); | |
if (!ipm) { | |
throw std::runtime_error( | |
"SubscriptionBase::get_intra_process_waitable() called " | |
"after destruction of intra process manager"); | |
} | |
// Use the id to retrieve the subscription intra-process from the intra-process manager. | |
return ipm->get_subscription_intra_process(intra_process_subscription_id_); | |
} |
Then,
SubscriptionIntraProcess
could outlive its associated Subscription
. Whether that's bad on its own I can't tell. @wjwwood thoughts?
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.
@hidmic then, considering what you pointed out, we should probably define/clarify who owns the callback object.
If you'd prefer to leave it as it was before (SubscriptionIntraProcess
having its own copy of the AnySubscriptionCallback
object, and not giving it a reference), I can simply copy the tracepoints/registration over to SubscriptionIntraProcess
so that it takes care of it when intraprocess is enabled.
I didn't do this because I didn't want to duplicate the tracepoint/registration code, but it's not really a big deal.
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.
SubscriptionIntraProcess
having its own copy of theAnySubscriptionCallback
object,
That's probably the safest thing to do. The new intra-process communication architecture is under experimental
so I'd expect changes to happen and I'd rather not constrain future development defining ownership or banning copies.
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.
Got it! I'll submit a different fix soon.
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.
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 agree that we should leave it for now and change the tracing logic instead to avoid lifetime issues. I'll close this in favor of the other one.
Closing in favor of #918 |
…2#916) Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
…os2#916) * Enable YAML encoding/decoding for RecordOptions and StorageOptions Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Christophe Bedard christophe.bedard@apex.ai
ros2_tracing
and the analysis tools use pointers in order to link a subscription to itsAnySubscriptionCallback
object(s) and callback instances. See theSubscription
constructor.The new intraprocess comms (#778) changed this by having the
AnySubscriptionCallback
object actually used bySubscriptionIntraProcess
. However, the problem is that it creates a copy for itself, and thus we can't link the callback back to the original subscription. I wrote a longer explanation over on theros2_tracing
repo.This makes it clear that the
AnySubscriptionCallback
object belongs to theSubscription
and notSubscriptionIntraProcess
, even if it used by the latter. However, this could of course be changed. Let me know if that would be preferable.I'm hoping this can get in in time for eloquent!