Parameter clients does not support spin_until_future_complete

[mini-1235]

Description

Since rclcpp::spin_some and spin_all are going to be deprecated soon #2848, I am trying to migrate it in my codebase

My use case here is:

node_ = std::make_shared<rclcpp::Node>("node");
executor_ = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
executor_->add_node(node_);
parameter_client = std::make_shared<rclcpp::SyncParametersClient>(node, server_name);
auto parameters = parameter_client->get_parameters({plugin_type});

I found out that when get_parameter, it will actually call spin_node_until_future_complete, which adds, spins, and removes the node, this will cause the following error

what():  Node has already been added to an executor.

I checked the api, and parameter client actually supports passing executor in the constructor

rclcpp/rclcpp/include/rclcpp/parameter_client.hpp

Lines 318 to 322 in 82696f6

	SyncParametersClient(
	rclcpp::Executor::SharedPtr executor,
	std::shared_ptr<NodeT> node,
	const std::string & remote_node_name = "",
	const rclcpp::QoS & qos_profile = rclcpp::ParametersQoS())

However, when getting parameters, it is always calling spin_node_until_future_complete

rclcpp/rclcpp/src/rclcpp/parameter_client.cpp

Lines 390 to 405 in 82696f6

	SyncParametersClient::get_parameters(
	const std::vector<std::string> & parameter_names,
	std::chrono::nanoseconds timeout)
	{
	auto f = async_parameters_client_->get_parameters(parameter_names);
	using rclcpp::executors::spin_node_until_future_complete;
	if (
	spin_node_until_future_complete(
	*executor_, node_base_interface_, f,
	timeout) == rclcpp::FutureReturnCode::SUCCESS)
	{
	return f.get();
	}
	// Return an empty vector if unsuccessful
	return std::vector<rclcpp::Parameter>();
	}

I am opening this issue to discuss whether it is possible to call

executor_->spin_until_future_complete(f,timeout)

when passing executor in the constructor, else call spin_node_until_future_complete

Motivation

Avoid adding node if executor passed in parameter clients

Design / Implementation Considerations

We could add a parameter for the constructor, use_internal_executor (default false), when false call rclcpp::spin_, otherwise we use the executor provided

Additional Information

No response

[jmachowinski]

I had a look at the code, and wonder if it even makes sense.

There seems to be the implicit assumption in this code, that the given node is exclusively used by the ParameterClient.
The same thing seems to be true for the used executor. Note, that this is not documented anywhere.

The fact, that this class internally spins the node if you call something like 'get_parameters', is highly unexpected given the interface.

As for your proposal, this will work in your usecase, but if anybody tries to do this inside a timer or callback, you'll get the exception 'spin_until_future_complete() called while already spinning'.

Note, that using an existing node with other callbacks, might also result in super strange call traces like this one :

Node has msg callbacks
- You call get_parameters
- msg callbacks get executed
- get_parameters returns

In general I would recommend a major redesign of the class...
@fujitatomoya @alsora @mjcarroll your thoughts on this ?

[mini-1235]

As for your proposal, this will work in your usecase, but if anybody tries to do this inside a timer or callback, you'll get the exception 'spin_until_future_complete() called while already spinning'.

That make sense for me

So, with rclcpp::spin_some now deprecated, are the maintainers suggesting that we replace code like this:

//constructor
(){
timer_ = node_->create_wall_timer(
      std::chrono::seconds(1),
      [this]() { funcB(); });
}
funcA(){
parameter_client = std::make_shared<rclcpp::SyncParametersClient>(node, server_name);
auto parameters = parameter_client->get_parameters({plugin_type});
}
//callback function
funcB(){
rclcpp::spin_some()
}

with

//constructor
(){
timer_ = node_->create_wall_timer(
      std::chrono::seconds(1),
      [this]() { funcB(); });
}
funcA(){
executor_ = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
executor_->add_node(node_);
parameter_client = std::make_shared<rclcpp::SyncParametersClient>(node, server_name);
auto parameters = parameter_client->get_parameters({plugin_type});
}
//callback function
funcB(){
executor_ = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
executor_->add_node(node_);
execuotr_->spin_some()
}

Or would you recommend a different approach?

Note that I am not saying that deprecating rclcpp::spin_* is bad, just looking for advice on the best way to update my current code :)

[alsora]

In general I would recommend a major redesign of the class...

I agree.
The current approach seems quite problematic.
I wonder if we could check if the node is already added to an executor... but that doesn't imply that the executor is spinning..

So, with rclcpp::spin_some now deprecated, are the maintainers suggesting that we replace code like this:

No, your replacement code has some issues.
Creating the executor and adding nodes to it shouldn't be done in loops. this is the main reason for the deprecation of the old API.

I don't have a full context about what you're doing, but I invite you to use the async executor and spin calls as much as possible.
The code would be something like this

// constructor
parameter_client_ = std::make_shared<rclcpp::AsyncParametersClient>(node_, server_name);
executor_ = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
executor->add_node(node_);
spinner_thread_ = std::make_unique<std::thread>([this]() { executor_->spin(); };

// funcA()
parameters_future = parameter_client->get_parameters({plugin_type}).wait();
parameters_future.wait();
auto parameters = parameters_future.get();

This is not a full solution to your original question, but IMO it's the best way to deal with parameters from a performance point of view.

[mini-1235]

I don't have a full context about what you're doing, but I invite you to use the async executor and spin calls as much as possible. The code would be something like this

Thanks for the suggestion, but I am a bit unclear on when it’s best to use spin and spin_some spin_all spin_until_future_complete.
For the example you provided above, are there any differences between

// constructor
parameter_client_ = std::make_shared<rclcpp::AsyncParametersClient>(node_, server_name);
executor_ = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
executor->add_node(node_);
spinner_thread_ = std::make_unique<std::thread>([this]() { executor_->spin(); };

// funcA()
parameters_future = parameter_client->get_parameters({plugin_type}).wait();
parameters_future.wait();
auto parameters = parameters_future.get();

and

// constructor
parameter_client_ = std::make_shared<rclcpp::AsyncParametersClient>(node_, server_name);
executor_ = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
executor->add_node(node_);

// funcA()
parameters_future = parameter_client->get_parameters({plugin_type});
if (rclcpp::spin_until_future_complete(node_, parameters_future) !=
    rclcpp::FutureReturnCode::SUCCESS)
auto parameters = parameters_future.get();

//funcB
executor_->spin_some()

[jmachowinski]

In general it is best practice to only use spin.

The example provided by @alsora were you spin in a second thread is also rather uncommon.

If you use spin_some / all / future_complete you either have a very niche use case or strange code.
Common use cases for spin_some etc are if you need to run ros code within a different environment like in an QT event loop.
A common misconception we see in the bug reports is, that people call spin_once etc and than have an expectation that something special gets executed. Note this is not the case, calling spin_some etc may also execute some executor internals and return.

Normally you set up your node, with a series of callbacks.
The node it then either:

• Timer driven (update loop moving the logic forward)
• Events driven (message callbacks etc)

A typical main.cpp should look like this:

int main()
{
   SomeNode::SharedPtr n = ...;
   rclcpp::executors::SingleThreadedExecutor ex;
   ex.add_node(n);
   ex.spin();
   return 0;
}

If you need / want to access parameters in callbacks and want to wait on the results, you need to use a MultiThreadedExecutor, and carefully design the callback groups. E.g. If your msg callback is in the global callback group and you put your AsyncParametersClient in a second callback group that your explicitly create code like this will work :

rclcpp::CallbackGroup::SharedPtr cbg = node->create_callback_group(rclcpp::CallbackGroupType::MutuallyExclusive);

AsyncParametersClient parameter_client(node, "NodeName",  rclcpp::ParametersQoS(), cbg);

void msgCbg(cons Msg &msg)
{
parameters_future = parameter_client->get_parameters({"SomeName"});
parameters_future.wait();
std::vector<rclcpp::Parameter> parameters = parameters_future.get();
}

[mini-1235]

Thanks for the detail explanation @jmachowinski, I think that has solved my issue, I am going to close this, thanks!