Fixup Executor::spin_all() regression fix #2517

wjwwood · 2024-04-30T12:40:29Z

This is a follow up of #2509 which addresses #2508.

Checks if executors are busy waiting while they should block in spin_some or spin_all. Signed-off-by: Janosch Machowinski <[email protected]>

This test was strange. It looked like, it assumed that spin_all did not return instantly. Also it was racy, as the thread could terminate instantly. Signed-off-by: Janosch Machowinski <[email protected]>

…available Signed-off-by: Janosch Machowinski <[email protected]>

Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: jmachowinski <[email protected]>

Signed-off-by: Janosch Machowinski <[email protected]>

Before, the method would not recollect available work in case of spin_some, spin_all. This would lead to the method behaving differently than to what the documentation states. Signed-off-by: Janosch Machowinski <[email protected]>

Signed-off-by: William Woodall <[email protected]>

wjwwood · 2024-04-30T12:43:06Z

CI:

Linux (updated after job got canceled)
Linux-aarch64 (known issues with mimic)
Windows (restarted after the job hung)

wjwwood

Just some notes for anyone reviewing this.

wjwwood · 2024-05-01T12:19:01Z

rclcpp/src/rclcpp/executor.cpp

+  // clear the wait result and wait for work without blocking to collect the work
+  // for the first time
+  // both spin_some and spin_all wait for work at the beginning
+  wait_result_.reset();
+  wait_for_work(std::chrono::milliseconds(0));
+  bool just_waited = true;


This change wasn't part of the original report (busy wait in spin_all) but it is related and is needed. It essentially ensures that we don't start out spin_some or spin_all by iterating over existing work from a previous spin method. Ideally the other spin methods would reset the result before returning so that this wouldn't be necessary, but this is safer. We do want to do this because in the case of spin_some this could cause it to never wait, iterating over old work and exiting after. This isn't really what we want.

Also, it is safe to throw away a wait result and wait again as all the things we wait on need to be able to be waited on, be ready, not be handled, be waited on again, and then be ready again. Or else they need to be ok with not being handled. Put another way, being waited on and then being ready does not guarantee being executed, and it may require being waited on again.

rclcpp/src/rclcpp/executor.cpp

mjcarroll

Overall change and test looks good. My only nit is that we need to probably put some of your comments here in the file for posterity.

Signed-off-by: William Woodall <[email protected]>

wjwwood · 2024-05-02T20:34:40Z

I added a comment in 8587653, so if the Rpr job comes back green I'll merge without new CI, since it was just a comment and Rpr checks style and linting.

wjwwood · 2024-05-02T20:34:57Z

I will then follow up with a jazzy backport.

wjwwood · 2024-05-02T21:11:12Z

@Mergifyio backport jazzy

mergify · 2024-05-02T21:11:18Z

backport jazzy

✅ Backports have been created

#2521 Fixup Executor::spin_all() regression fix (backport #2517) has been created for branch jazzy

* test(Executors): Added tests for busy waiting Checks if executors are busy waiting while they should block in spin_some or spin_all. Signed-off-by: Janosch Machowinski <[email protected]> * fix: Reworked spinAll test This test was strange. It looked like, it assumed that spin_all did not return instantly. Also it was racy, as the thread could terminate instantly. Signed-off-by: Janosch Machowinski <[email protected]> * fix(Executor): Fixed spin_all not returning instantly is no work was available Signed-off-by: Janosch Machowinski <[email protected]> * Update rclcpp/test/rclcpp/executors/test_executors.cpp Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: jmachowinski <[email protected]> * test(executors): Added test for busy waiting while calling spin Signed-off-by: Janosch Machowinski <[email protected]> * fix(executor): Reset wait_result on every call to spin_some_impl Before, the method would not recollect available work in case of spin_some, spin_all. This would lead to the method behaving differently than to what the documentation states. Signed-off-by: Janosch Machowinski <[email protected]> * restore previous test logic for now Signed-off-by: William Woodall <[email protected]> * refactor spin_some_impl's logic and improve busy wait tests Signed-off-by: William Woodall <[email protected]> * added some more comments about the implementation Signed-off-by: William Woodall <[email protected]> --------- Signed-off-by: Janosch Machowinski <[email protected]> Signed-off-by: jmachowinski <[email protected]> Signed-off-by: William Woodall <[email protected]> Co-authored-by: Janosch Machowinski <[email protected]> Co-authored-by: jmachowinski <[email protected]> Co-authored-by: Tomoya Fujita <[email protected]> (cherry picked from commit f7185dc)

* test(Executors): Added tests for busy waiting Checks if executors are busy waiting while they should block in spin_some or spin_all. Signed-off-by: Janosch Machowinski <[email protected]> * fix: Reworked spinAll test This test was strange. It looked like, it assumed that spin_all did not return instantly. Also it was racy, as the thread could terminate instantly. Signed-off-by: Janosch Machowinski <[email protected]> * fix(Executor): Fixed spin_all not returning instantly is no work was available Signed-off-by: Janosch Machowinski <[email protected]> * Update rclcpp/test/rclcpp/executors/test_executors.cpp Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: jmachowinski <[email protected]> * test(executors): Added test for busy waiting while calling spin Signed-off-by: Janosch Machowinski <[email protected]> * fix(executor): Reset wait_result on every call to spin_some_impl Before, the method would not recollect available work in case of spin_some, spin_all. This would lead to the method behaving differently than to what the documentation states. Signed-off-by: Janosch Machowinski <[email protected]> * restore previous test logic for now Signed-off-by: William Woodall <[email protected]> * refactor spin_some_impl's logic and improve busy wait tests Signed-off-by: William Woodall <[email protected]> * added some more comments about the implementation Signed-off-by: William Woodall <[email protected]> --------- Signed-off-by: Janosch Machowinski <[email protected]> Signed-off-by: jmachowinski <[email protected]> Signed-off-by: William Woodall <[email protected]> Co-authored-by: Janosch Machowinski <[email protected]> Co-authored-by: jmachowinski <[email protected]> Co-authored-by: Tomoya Fujita <[email protected]> (cherry picked from commit f7185dc) Co-authored-by: William Woodall <[email protected]>

jmachowinski · 2024-05-06T16:20:55Z

rclcpp/test/rclcpp/executors/test_executors.cpp

+  }
+  EXPECT_GT(this->waitable->get_count(), 0u);
+
+  first_check_passed = true;


you need to lock cv_m while accessing the share state (e.g. first_check_passed). If you don't do this, you get race conditions

jmachowinski · 2024-05-06T16:25:28Z

rclcpp/test/rclcpp/executors/test_executors.cpp

+    cv.wait_for(lk, 10s);
+  }
+  EXPECT_GT(this->waitable->get_count(), 0u);
+


This test is racy / does not really check for busy waits. You need to wait a bit here, to give a busy waiting executor time to call is_ready a lot of times. As the test is implemented right now, it could pass even if the executor is busy waiting.

Janosch Machowinski and others added 8 commits April 30, 2024 03:27

test(Executors): Added tests for busy waiting

3f704ff

Checks if executors are busy waiting while they should block in spin_some or spin_all. Signed-off-by: Janosch Machowinski <[email protected]>

fix: Reworked spinAll test

5150a16

This test was strange. It looked like, it assumed that spin_all did not return instantly. Also it was racy, as the thread could terminate instantly. Signed-off-by: Janosch Machowinski <[email protected]>

fix(Executor): Fixed spin_all not returning instantly is no work was …

f43114b

…available Signed-off-by: Janosch Machowinski <[email protected]>

Update rclcpp/test/rclcpp/executors/test_executors.cpp

8d639e6

Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: jmachowinski <[email protected]>

test(executors): Added test for busy waiting while calling spin

f2303da

Signed-off-by: Janosch Machowinski <[email protected]>

restore previous test logic for now

500ee7d

Signed-off-by: William Woodall <[email protected]>

refactor spin_some_impl's logic and improve busy wait tests

6fa2604

Signed-off-by: William Woodall <[email protected]>

wjwwood self-assigned this Apr 30, 2024

wjwwood requested review from ivanpauno and hidmic as code owners April 30, 2024 12:40

wjwwood mentioned this pull request Apr 30, 2024

Executer regression fix #2509

Closed

wjwwood commented May 1, 2024

View reviewed changes

wjwwood mentioned this pull request May 1, 2024

Regression : Executor::spin_some_impl is active waiting #2508

Closed

mjcarroll approved these changes May 2, 2024

View reviewed changes

added some more comments about the implementation

8587653

Signed-off-by: William Woodall <[email protected]>

wjwwood merged commit f7185dc into rolling May 2, 2024
3 checks passed

wjwwood deleted the wjwwood/executor_spin_all_regression_fix branch May 2, 2024 21:06

mergify bot mentioned this pull request May 2, 2024

Fixup Executor::spin_all() regression fix (backport #2517) #2521

Merged

jmachowinski reviewed May 6, 2024

View reviewed changes

clalancette mentioned this pull request Jul 24, 2024

Switch nav2_map_server to use modern CMake idioms. ros-navigation/navigation2#4557

Merged

7 tasks

fujitatomoya mentioned this pull request Jan 26, 2025

suggest spin_all to migrate spinOnce to execute all available works i… ros2/ros2_documentation#4966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixup Executor::spin_all() regression fix #2517

Fixup Executor::spin_all() regression fix #2517

wjwwood commented Apr 30, 2024

wjwwood commented Apr 30, 2024 •

edited

Loading

wjwwood left a comment

wjwwood May 1, 2024

mjcarroll left a comment

wjwwood commented May 2, 2024

wjwwood commented May 2, 2024

wjwwood commented May 2, 2024

mergify bot commented May 2, 2024 •

edited

Loading

jmachowinski May 6, 2024

jmachowinski May 6, 2024

Fixup Executor::spin_all() regression fix #2517

Fixup Executor::spin_all() regression fix #2517

Conversation

wjwwood commented Apr 30, 2024

wjwwood commented Apr 30, 2024 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood May 1, 2024

Choose a reason for hiding this comment

mjcarroll left a comment

Choose a reason for hiding this comment

wjwwood commented May 2, 2024

wjwwood commented May 2, 2024

wjwwood commented May 2, 2024

mergify bot commented May 2, 2024 • edited Loading

✅ Backports have been created

jmachowinski May 6, 2024

Choose a reason for hiding this comment

jmachowinski May 6, 2024

Choose a reason for hiding this comment

wjwwood commented Apr 30, 2024 •

edited

Loading

mergify bot commented May 2, 2024 •

edited

Loading