process hangs in parameter change callback

[xhuan28]

• Operating System:
  • Ubuntu18.04
• Installation type:
  • source
• Version or commit hash:
  • 3293603
• DDS implementation:
  • fastrtps
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

ros2 param set <node_name> <param_name> <param_value>

Expected behavior

set parameter successfully

Actual behavior

the process of ros2 param hangs without any feedback. When I was trying to terminate the application with Ctrl+C, it also fails with the output "[INFO] [rclcpp]: signal_handler(signal_value=2)", but no return.

Additional information

the parameter change callback that I implemented:
auto callback = [this] (const std::vectorrclcpp::Parameter & params)
{
auto result = rcl_interfaces::msg::SetParametersResult();
result.successful = true;
for (auto & param : params) {
if (param.get_name() == "enabled") {
if (param.get_type() == rclcpp::ParameterType::PARAMETER_BOOL) {
if (param.as_bool() == true && enable_ == false) {
RCLCPP_INFO(node_.get_logger(), "toggle on");
enable_ = true;
} else if (param.as_bool() == false && enable_ == true) {
RCLCPP_INFO(node_.get_logger(), "toggle off");
enable_= false;
this->undeclare_parameter("param1");
this->undeclare_parameter("param2");
this->undeclare_parameter("param3");
} else {
result.successful = false;
result.reason = "Parameter is equal to the previous value. Do nothing.";
}
} else {
result.successful = false;
result.reason = "Type should be boolean.";
}
}
}
return result;
}

I found only when I add the 3 lines in bold, the issue occurs. If I don't undeclare parameter in this callback, it seems that everything is ok. Is there any problem in the usage of rclcpp?

[clalancette]

There is a bug in rclcpp, and there is a bug in your callback.

The rclcpp problem is that when the callback is called, the Parameters implementation is holding a lock. Calling any of the other Parameter APIs also tries to acquire that lock, so it deadlocks. I fixed it for the get_parameter and list_parameter APIs in #781 .

The problem with your callback is that you are not allowed to declare, undeclare, or set parameters during the callback. In the same pull request above (which will be in Eloquent), I added explicit checks and an exception if you try to do this. Since the callback is called when doing any of the parameter modifications, doing it essentially recursively seems like it would lead to trouble.

So my suggestion for you is to think about why you are trying to undeclare parameters in the callback, and do something else.

I'm going to close this issue out for now, but feel free to keep commenting or reopen if you have additional thoughts here.

[xhuan28]

@clalancette
Thanks for your prompt answer. Actually, in my case, param1, param2, param3 all depend on the param "enabled". Only when "enabled" is set to true, the other 3 parameters are valid and can be set later. On the contrary, when "enabled" is set to false, I want the other 3 parameters are invisible to other parameter client apps. That is why I would like to undeclare them in the callback. I don't know how to deal with this case given the current design of rclcpp parameters. Is there any suggestion on this?
Thanks.

[clalancette]

Thanks for your prompt answer. Actually, in my case, param1, param2, param3 all depend on the param "enabled". Only when "enabled" is set to true, the other 3 parameters are valid and can be set later. On the contrary, when "enabled" is set to false, I want the other 3 parameters are invisible to other parameter client apps. That is why I would like to undeclare them in the callback. I don't know how to deal with this case given the current design of rclcpp parameters. Is there any suggestion on this?

I'm not a big fan of having parameters invisible. If they are invisible, there is no way for a user to introspect your node (ros2 param list) and find out what parameters are available. They have to go read the source code, which seems inefficient.

Instead, I'd suggest just declaring them during the constructor, and then in your parameter_change_callback only allow them to be changed if enabled is set to True. That way the parameters are still discoverable, but they can't be set unless enabled is True.

[xhuan28]

Yeah, this should be a better way to handle my case. Thanks for your suggestion.