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. |
| } | ||
|
|
||
| AnySubscriptionCallback<CallbackMessageT, Alloc> any_callback_; | ||
| AnySubscriptionCallback<CallbackMessageT, Alloc> & any_callback_; |
There was a problem hiding this comment.
@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
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.
@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.
SubscriptionIntraProcesshaving its own copy of theAnySubscriptionCallbackobject,
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.
Got it! I'll submit a different fix soon.
There was a problem hiding this comment.
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_tracingand the analysis tools use pointers in order to link a subscription to itsAnySubscriptionCallbackobject(s) and callback instances. See theSubscriptionconstructor.The new intraprocess comms (#778) changed this by having the
AnySubscriptionCallbackobject 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_tracingrepo.This makes it clear that the
AnySubscriptionCallbackobject belongs to theSubscriptionand 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!