ros2 param list crash

[BriceRenaudeau]

Bug report

When creating a sub-node from a node using the default constructor, the command ros2 param list crashes at the sub-node line.
This kind of architecture is often used in Nav2 (_rclcpp sub-node, BT _client_node, ...)

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • binaries
• Version or commit hash:
  • galactic
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

#include <rclcpp/rclcpp.hpp>

class FooNode : public rclcpp::Node {
public:
  FooNode() : Node("foo")
  {
    client_node = std::make_shared<rclcpp::Node>("bar");
  }
  std::shared_ptr<rclcpp::Node> client_node;
};

int main(int argc, char **argv)
{
  rclcpp::init(argc, argv);
  auto node = std::make_shared<FooNode>();
  rclcpp::spin(node);
  rclcpp::shutdown();
  return 0;
}

Expected behavior

The parameters are listed correctly when using the command ros2 param list.

Actual behavior

Exception while calling service of node '/foo': None
Traceback (most recent call last):
  File "/opt/ros/galactic/bin/ros2", line 11, in <module>
    load_entry_point('ros2cli==0.13.3', 'console_scripts', 'ros2')()
  File "/opt/ros/galactic/lib/python3.8/site-packages/ros2cli/cli.py", line 67, in main
    rc = extension.main(parser=parser, args=args)
  File "/opt/ros/galactic/lib/python3.8/site-packages/ros2param/command/param.py", line 39, in main
    return extension.main(args=args)
  File "/opt/ros/galactic/lib/python3.8/site-packages/ros2param/verb/list.py", line 117, in main
    sorted_names = sorted(response.result.names)
AttributeError: 'NoneType' object has no attribute 'result'

Additional information

if you declare the sub-node with the next options, the issue doesn't appear.

    .start_parameter_services(false)
    .start_parameter_event_publisher(false)

I know that we need to spin the sub-node to make it works, but it could be good to have the command line not stuck at the sub-node.

[alsora]

Your options

    .start_parameter_services(false)
    .start_parameter_event_publisher(false)

are essentially disabling remote ROS 2 parameters on the node, so it makes sense that it fixes the problem (as from the ros2 param list point of view is as if the node didn't exist).

This clearly looks like a bug, although I do have some considerations.

• What happens if you don't create the subnode and you also remove the line rclcpp::spin(node);? I would like to undestand if this is a problem with the node not spinning or with the presence of a subnode.
• Creating a node within a node is IMO a bad design pattern

[BriceRenaudeau]

• What happens if you don't create the subnode and you also remove the line rclcpp::spin(node);? I would like to undestand if this is a problem with the node not spinning or with the presence of a subnode.

class FooNode : public rclcpp::Node {
public:
  FooNode() : Node("foo") {  }
};

int main(int argc, char **argv)
{
  rclcpp::init(argc, argv);
  auto node = std::make_shared<FooNode>();

  rclcpp::Rate delai(0.1);
  delai.sleep();

  rclcpp::shutdown();
  return 0;
}

Exception while calling service of node '/foo': None
If a node is not spinning, the ros2 param list command will throw an exception when this node appears in the list (alphabetic order)
A spin_some(node) is also not enough.

• Creating a node within a node is IMO a bad design pattern

This design pattern is often use when dealing with plugins (and clear logs) @SteveMacenski

https://github.com/ros-planning/navigation2/blob/eb53c28c05d21ae64cb6c508cb14c7ccc0eb8714/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server.hpp#L213-L214

https://github.com/ros-planning/navigation2/blob/eb53c28c05d21ae64cb6c508cb14c7ccc0eb8714/nav2_costmap_2d/src/costmap_2d_ros.cpp#L87-L89

https://github.com/ros-planning/navigation2/blob/eb53c28c05d21ae64cb6c508cb14c7ccc0eb8714/nav2_rviz_plugins/include/nav2_rviz_plugins/nav2_panel.hpp#L90-L91

[SteveMacenski]

Its generally a bad pattern none the less. We have a couple of spots where it is unavoidable but we worked hard to reduce it to just those places and instead make smart use of internal executors

[BriceRenaudeau]

Any update?

[fujitatomoya]

@BriceRenaudeau I confirmed that this problem can be observed on master branch as well. but i just dont have enough resource to dig deeper yet.

[llapx]

@BriceRenaudeau

I know that we need to spin the sub-node to make it works, but it could be good to have the command line not stuck at the sub-node.

I see lastest https://github.com/ros2/ros2cli does not stuck in your case, I modify your code like this:

#include <rclcpp/rclcpp.hpp>

class FooNode : public rclcpp::Node
{
public:
  FooNode() : Node("foo")
  {
    client_node = std::make_shared<rclcpp::Node>("bar");
    client_node2 = std::make_shared<rclcpp::Node>("bar2");
  }
  std::shared_ptr<rclcpp::Node> client_node;
  std::shared_ptr<rclcpp::Node> client_node2;
};

int main(int argc, char **argv)
{
  rclcpp::init(argc, argv);
  auto node = std::make_shared<FooNode>();
  rclcpp::spin(node);
  rclcpp::shutdown();
  return 0;
}

when run ros2 param list, the output look like bellow:

root@csc:/work/ros2_ws# ros2 param list 
1655095831.419873 [0]       ros2: using network interface enp0s31f6 (udp/192.168.0.144) selected arbitrarily from: enp0s31f6, enp3s0, docker0, br-fc1bada4fc41
Exception while calling service of node '/bar': None
Exception while calling service of node '/bar2': None
/foo:
  qos_overrides./parameter_events.publisher.depth
  qos_overrides./parameter_events.publisher.durability
  qos_overrides./parameter_events.publisher.history
  qos_overrides./parameter_events.publisher.reliability
  use_sim_time

you can see /bar and /bar2 are all printed as exceptions, see https://github.com/ros2/ros2cli/blob/f4e5952f430e502060594d68f3a050b785fd4249/ros2param/ros2param/verb/list.py#L113 for detail.

So in my option, someone may has fixed it in the master branch of https://github.com/ros2/ros2cli.

[BriceRenaudeau]

Thanks, that's good news.

[fujitatomoya]

@llapx

last time (#1930 (comment)) i was checking on this, this problem could be observed.
do you happen to know which fix has addressed this problem?

[llapx]

@fujitatomoya
commit:
ros2/ros2cli@a9def45

PR:
ros2/ros2cli#646

[fujitatomoya]

@llapx thanks for checking on this. i think we need to backport ros2/ros2cli#646 to humble/galactic/foxy.

[llapx]

@llapx thanks for checking on this. i think we need to backport ros2/ros2cli#646 to humble/galactic/foxy.

humble already applied, galactic and foxy should be backported.

[fujitatomoya]

this issue has been addressed, closing.