Avoid copying AnySubscriptionCallback objects for intraprocess comms

[christophebedard]

Signed-off-by: Christophe Bedard christophe.bedard@apex.ai

ros2_tracing and the analysis tools use pointers in order to link a subscription to its AnySubscriptionCallback object(s) and callback instances. See the Subscription constructor.

The new intraprocess comms (#778) changed this by having the AnySubscriptionCallback object actually used by SubscriptionIntraProcess. 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 the ros2_tracing repo.

This makes it clear that the AnySubscriptionCallback object belongs to the Subscription and not SubscriptionIntraProcess, 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!

[christophebedard]

@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.

[hidmic]

@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?

[christophebedard]

@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.

[hidmic]

SubscriptionIntraProcess having its own copy of the AnySubscriptionCallback 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.

[christophebedard]

Got it! I'll submit a different fix soon.

[christophebedard]

@hidmic done: #918

[wjwwood]

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.

[wjwwood]

Closing in favor of #918