bad_weak_ptr with swri_image_util (ros2-devel)

[agyoungs]

shared_from_this() cannot be used in constructors since it will cause a bad_weak_ptr runtime exception.

I fixed this in a less than ideal way (checking at each use) in #659, however there was one node I didn't touch since there wasn't a way to solve this with the same method.

replace_colors_node.cpp uses image_transport and passes the Node::SharedPtr in the constructor. I think this could be a normal rclcpp::Subscription instead of an image_transport subscription, but before I created a PR I wanted to see what others thought.

// Set up the ROS interface
image_transport::ImageTransport it(shared_from_this());
image_pub_ = it.advertise("modified_image", 1);
image_sub_ = it.subscribe(
  "image",
  1,
  &ReplaceColorsNodelet::imageCallback,
  this);

[matt-attack]

There's not really a great solution here since ROS removed the initialize function from the "nodelet base class" in ROS2.

I think the issue in this particular case is that image transport is taking the node as a shared pointer at all. Taking a shared pointer implies that it is taking some ownership of the node, which is very much not the case (at least in this application) unless you love memory leaks and your node never getting destroyed. The member owning the parent class causes a circular reference. Therefore I would argue the fix for this, and the other cases are to make those APIs instead only take normal pointers/references to the node and only use it for initialization so it only needs it to be valid for the duration of their constructor. This would require a bit of refactoring however.

[matt-attack]

The other possible solution, which may or may not be possible (and if so probably annoying) is not inheriting from Node in components at all avoiding the circular reference and shared pointer to this completely. From a brief search, it seems possible but at the least you'd have to set up your own executor.

https://ubuntu.com/blog/components-vs-plugins-in-ros-2

[danthony06]

Fixed in #659

[danthony06]

Oops, I see this is for image_util. I looked into image_transport in ROS2, and I think we want to continue using image_transport, and not a regular publisher.