[documentation] Explain when MultiThreadedExecutor is running multiple threads
#2454
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fujitatomoya
reviewed
Apr 4, 2024
|
@Cakem1x btw, can you also address DCO error? |
It includes a note about which methods support multithreaded execution. Signed-off-by: Matthias Holoch <[email protected]>
This reverts commit e751e1f. Signed-off-by: Matthias Holoch <[email protected]>
Signed-off-by: Matthias Holoch <[email protected]>
…ture_complete Signed-off-by: Matthias Holoch <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: Matthias Holoch <[email protected]>
Cakem1x
force-pushed
the
docs/multithreaded-executor-clarification
branch
from
b6e4e81
to
17929ce
Compare
This was referenced Jun 3, 2024
Zard-C
reviewed
Jun 4, 2024
| @@ -61,8 +61,12 @@ class MultiThreadedExecutor : public rclcpp::Executor | |||
| RCLCPP_PUBLIC | |||
| virtual ~MultiThreadedExecutor(); | |||
|
|
|||
| /// Multi-threaded implementation of spin. | |||
alsora
requested changes
Aug 10, 2024
| /** | ||
| * \sa rclcpp::Executor:spin() for more details | ||
| * This function will block until work comes in, and try to execute all work with |
There was a problem hiding this comment.
This is not correct.
This function won't block until works come in, but rather it will block until interrupted.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a tiny PR that adds a class docstring for the
MultiThreadedExecutor. It includes a note about which methods support multithreaded execution.Please let me know if this is not the right way to suggest additions to the documentation.
Also, please double check if the note is actually true or can be improved.
My context:
I have a unit test with two nodes that communicate with each other and I use the
MultiThreadedExecutorto spin both of these nodes. One node publishes a TF, the other one regularly waits for that TF.In the individual test cases, I have been using
multi_threaded_executor_.spin_until_future_complete(some_future);. This leads to a flaky test that sometimes succeeds and sometimes fails. The reason for that seems to be that theMultiThreadedExecutoris only using a single thread whenspin_until_future_completeis used. The test would fail (timeout) if the work item from node that waits for the TF gets executed first. Waiting for the TF blocks the main thread until timeout, while the other node that should supply that TF will not get executed.After changing my test setup to use
multi_threaded_executor_.spin();in a separate thread and just dosome_future.wait_for(...)in the test cases, everything is working fine.So, after finding all that out, I thought I could maybe help others by adding a note somewhere. Just skimming through the API docs, that behavior was not clear to me.
Thanks for having a look!