Skip to content
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

fix(params): prevent parameter retrieval crashes in ROS2 #978

Open
wants to merge 2 commits into
base: ros2
Choose a base branch
from
Open

fix(params): prevent parameter retrieval crashes in ROS2 #978

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d36f77
fix(params): prevent parameter retrieval crashes in ROS2
Mehsias Dec 5, 2024
cd480cb
Merge branch 'ros2' into fix/issue-972-parameter-retrieval
bjsowa Dec 27, 2024
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
4 changes: 2 additions & 2 deletions rosapi/src/rosapi/params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ def _get_param_names(node_name):
# This method is called in a service callback; calling a service of the same node
# will cause a deadlock.
global _parent_node_name
if node_name == _parent_node_name:
if node_name == _parent_node_name or node_name == _node.get_fully_qualified_name():
return []

client = _node.create_client(ListParameters, f"{node_name}/list_parameters")
Expand All @@ -238,7 +238,7 @@ def _get_param_names(node_name):
request = ListParameters.Request()
future = client.call_async(request)
if _node.executor:
_node.executor.spin_until_future_complete(future)
_node.executor.spin_until_future_complete(future, timeout_sec=2.0)
Copy link
Contributor

@sea-bass sea-bass Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Changes look good, but this 2.0 may not hold in all cases. Wonder if should at least be a named constant instead of hard-coded, or even better an input argument to this function that comes from a configurable ROS parameter of the node?

Copy link
Contributor

@sea-bass sea-bass Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also, would it help to add this timeout in the else case right below this as well, which does the same but with the global executor?

Copy link
Author

Choose a reason for hiding this comment

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

@sea-bass That's a good idea! I've also noticed that in params.py there is a todo saying to remove the node entirely and use rosapi instead. What do you think this would entail in terms of work?

Sorry I am fairly new to ROS and don't have much experience in its uses outside of my limited scope.

else:
rclpy.spin_until_future_complete(_node, future)
response = future.result()
Expand Down
Loading