Skip to content

Fix/service availability race #3711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AravindhDeivaG wants to merge 2 commits into
base: humble
Choose a base branch
from
+18 −1
Open

Fix/service availability race #3711

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9e7758
Fix race condition in service client initialization by waiting for se…
AravindhDeivaG Mar 21, 2026
9ae86c5
Added another argument in constructor call of MoveGroupInterfaceImpl …
AravindhDeivaG Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,16 @@ class MoveGroupInterface::MoveGroupInterfaceImpl
execute_action_client_ = rclcpp_action::create_client<moveit_msgs::action::ExecuteTrajectory>(
node_, rclcpp::names::append(opt_.move_group_namespace_, move_group::EXECUTE_ACTION_NAME), callback_group_);
execute_action_client_->wait_for_action_server(wait_for_servers.to_chrono<std::chrono::duration<double>>());

// Function to check if the service server is available
auto wait_for_service = [this](auto& client, const std::string& service_name) {
if (!client->wait_for_service(std::chrono::seconds(10)))
Copy link
Copy Markdown
Contributor

@riv-mjohnson riv-mjohnson Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are already seeing problems where MoveIt is not tolerant enough to startup delays on large systems. I think this will cause more startup crashes on our system.

If there is going to be a hard timeout, could it at least be configurable?

Copy link
Copy Markdown
Author

@AravindhDeivaG AravindhDeivaG Mar 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel there are two three possible fixes. Either we can make the block indefinite because there is no point in proceeding if rviz plugin can't find the list of planners available.

Secondly the action above the service has a standard 30 seconds delay which cannot be configured. We can add a similarly large delay.

Third we can declare a parameter which can be passed using yaml file as the plugin uses rviz node for all the communication and user should be able to configure it during launch

Copy link
Copy Markdown
Contributor

@riv-mjohnson riv-mjohnson Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal preference would be for this to use a configurable timeout, like the wait_for_servers timeout used in the action client above.

I'm not sure what the MoveIt maintainers would prefer though.

Copy link
Copy Markdown
Author

@AravindhDeivaG AravindhDeivaG Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait for servers uses a constant 30 seconds delay. It is not configurable

Copy link
Copy Markdown
Contributor

@riv-mjohnson riv-mjohnson Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought wait_for_servers was a constructor argument of MoveGroupInterface, and so user-configurable?

Maybe we're talking about different waits? Where is the 30 second wait you're talking about defined?

Copy link
Copy Markdown
Contributor

@riv-mjohnson riv-mjohnson Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, I understand now, thanks for the clarification.

Copy link
Copy Markdown
Contributor

@riv-mjohnson riv-mjohnson Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful for the plugin to have configurable waits, but I'm guessing that's out of scope for this PR.

Copy link
Copy Markdown
Author

@AravindhDeivaG AravindhDeivaG Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'll add another argument to the constructor call for service wait time, with default value of 10 seconds. Is that okay?

Copy link
Copy Markdown
Contributor

@riv-mjohnson riv-mjohnson Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds sensible to me

Copy link
Copy Markdown
Author

@AravindhDeivaG AravindhDeivaG Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for the inputs.

{
throw std::runtime_error("Service server not available for service: " + service_name +
". Is move_group running?");
}
RCLCPP_INFO(node_->get_logger(), "Service '%s' found.", service_name.c_str());
};

query_service_ = node_->create_client<moveit_msgs::srv::QueryPlannerInterfaces>(
rclcpp::names::append(opt_.move_group_namespace_, move_group::QUERY_PLANNERS_SERVICE_NAME),
Expand All @@ -179,6 +189,12 @@ class MoveGroupInterface::MoveGroupInterfaceImpl
cartesian_path_service_ = node_->create_client<moveit_msgs::srv::GetCartesianPath>(
rclcpp::names::append(opt_.move_group_namespace_, move_group::CARTESIAN_PATH_SERVICE_NAME),
rmw_qos_profile_services_default, callback_group_);

// Check if service servers are available for all the clients
wait_for_service(query_service_, move_group::QUERY_PLANNERS_SERVICE_NAME);
wait_for_service(get_params_service_, move_group::GET_PLANNER_PARAMS_SERVICE_NAME);
wait_for_service(set_params_service_, move_group::SET_PLANNER_PARAMS_SERVICE_NAME);
wait_for_service(cartesian_path_service_, move_group::CARTESIAN_PATH_SERVICE_NAME);

// plan_grasps_service_ = pnode_->create_client<moveit_msgs::srv::GraspPlanning>(GRASP_PLANNING_SERVICE_NAME);

Expand Down