Skip to content

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

Merged
adityapande-1995 merged 2 commits intoros2:rollingfrom
AndyZe:andyz/spin_once_doc
Oct 17, 2023
Merged

Document that spin_once() should not be called from multiple threads#1079
adityapande-1995 merged 2 commits intoros2:rollingfrom
AndyZe:andyz/spin_once_doc

Conversation

@AndyZe
Copy link
Copy Markdown
Contributor

@AndyZe AndyZe commented Feb 6, 2023

Fixes #1078

@AndyZe AndyZe mentioned this pull request Feb 6, 2023
Open
@AndyZe AndyZe force-pushed the andyz/spin_once_doc branch from 12e24cb to db9aae2 Compare February 6, 2023 19:31
fujitatomoya
fujitatomoya reviewed Feb 7, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

fujitatomoya commented Feb 7, 2023

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.

@felixdivo felixdivo mentioned this pull request May 21, 2023
Merged
sloretz
sloretz reviewed Oct 13, 2023
View reviewed changes
@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Oct 13, 2023

@Mergifyio rebase

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Oct 13, 2023

rebase

❌ Unable to rebase: user sloretz is unknown.

Details

Please make sure sloretz has logged in Mergify dashboard.

AndyZe and others added 2 commits October 13, 2023 18:31
@AndyZe @sloretz
Document that spin_once() should not be called from multiple threads
d08c56e 
Signed-off-by: AndyZe <andyz@utexas.edu>
@sloretz
Update __init__.py
63a2231 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz sloretz force-pushed the andyz/spin_once_doc branch from bf1aad9 to 63a2231 Compare October 13, 2023 18:31
@sloretz
Copy link
Copy Markdown
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
@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

Reviewers

@fujitatomoya fujitatomoya fujitatomoya left review comments

@sloretz sloretz sloretz approved these changes

@adityapande-1995 adityapande-1995 adityapande-1995 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@AndyZe @fujitatomoya @sloretz @adityapande-1995