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.

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

Document that spin_once() should not be called from multiple threads #1079

Merged
merged 2 commits into from Oct 17, 2023

Conversation

AndyZe
Copy link
Contributor

@AndyZe AndyZe commented Feb 6, 2023

Fixes #1078

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

I think this depends on arguments, if user application calls rclpy::spin_once with the same nodes and two different executor, this would be problem since we have a node in 2 wait sets. but we can create multiple threads to call SingleThreadedExecutor::snip_once: for each node, that is usually what user would do.

@AndyZe
Copy link
Contributor Author

AndyZe commented Feb 7, 2023

Can you please suggest better wording for this PR?

but we can create multiple threads to call SingleThreadedExecutor::snip_once: for each node, that is usually what user would do.

This is what happened in my experience and it didn't work.

I'd actually suggest to deprecate spin_once() since it has these pitfalls that aren't well understood and recommend spin() only ¯_(ツ)_/¯

@fujitatomoya
Copy link
Collaborator

To be honest, i am not sure how to address this as documentation. How about adding node which is not already added to any other executors in the description of node argument? that is the situation that we need to avoid here. there has been discussion related about this with rclcpp, ros2/rclcpp#1953. probably node should be aware that it is already added to executor or not, or we can add the same node to different executor (wait sets).

I'd actually suggest to deprecate spin_once() since it has these pitfalls that aren't well understood and recommend spin() only ¯_(ツ)_/¯

i believe we can have the same problem with spin_once(), that is not gonna be the solution.

This is what happened in my experience and it didn't work.

i understand, for user experience, probably please do not use those with multi-thread would be reasonable for now. instead, we can recommend MultiThreadedExecutor. i will keep this open so that we can have more feedback.

thanks for iterating.

rclpy/rclpy/__init__.py Show resolved Hide resolved
rclpy/rclpy/executors.py Show resolved Hide resolved
@sloretz
Copy link
Contributor

sloretz commented Oct 13, 2023

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Oct 13, 2023

rebase

❌ Unable to rebase: user sloretz is unknown.

Please make sure sloretz has logged in Mergify dashboard.

AndyZe and others added 2 commits October 13, 2023 18:31
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Copy link
Contributor

sloretz commented Oct 13, 2023

CI (repos file build: --packages-up-to rclpy test: --packages-select rclpy)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@adityapande-1995 adityapande-1995 merged commit df92d9e into ros2:rolling Oct 17, 2023
3 checks passed
@AndyZe AndyZe deleted the andyz/spin_once_doc branch October 17, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document that spin_once() should not be called from multiple threads
4 participants