Fix bug in timers lifecycle for events executor by apojomovsky · Pull Request #2586 · ros2/rclcpp

apojomovsky · 2024-07-22T21:09:07Z

This PR addresses a bug in the EventsExecutor related to timers. The issue occurs when multiple timers are present in the entities collection, and at least one of them gets re-initialized. Prior to this fix, the collection failed to detect which timers were re-initialized, leading to the following problems:

The old timer might continue running, triggering its callback.
The new timer might never start, preventing its callback from being invoked.

The fix consists in invoking the remove_expired_entities right before the timers collection is updated.
A regression test was included among the changes, this test will fail with the current rolling HEAD.

apojomovsky · 2024-07-22T21:09:39Z

@alsora @mauropasse FYI

rclcpp/test/rclcpp/test_reinitialized_timers.cpp

rclcpp/src/rclcpp/experimental/executors/events_executor/events_executor.cpp

alsora · 2024-07-22T23:39:06Z

The test is a good step, unfortunately, in its current form, it does not guarantee that it will catch regressions.
The bug happens when the same memory addresses get re-used.

To guarantee that we should maybe use a custom allocator with placement new, but I'm not sure how easy/hard is to do that.

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

apojomovsky · 2024-08-02T22:09:06Z

The test is a good step, unfortunately, in its current form, it does not guarantee that it will catch regressions. The bug happens when the same memory addresses get re-used.

To guarantee that we should maybe use a custom allocator with placement new, but I'm not sure how easy/hard is to do that.

Thanks for the review, Alberto. I have just pushed an update addressing all the other comments but this one is still pending. I might need some assistance with the custom allocator with placement new in case it's required.

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

rclcpp/src/rclcpp/experimental/executors/events_executor/events_executor.cpp

alsora · 2024-08-05T16:10:38Z

@apojomovsky I think that the PR is good to go.
The test with a custom allocator would be nice to have, but it's worth fixing the issue asap and the focusing on that later since it's not an easy task

Linux
Linux-aarch64
Linux-rhel
Windows

alsora · 2024-08-05T21:25:09Z

https://ci.ros2.org/job/ci_linux/21619/testReport/junit/rclcpp/uncrustify/test_rclcpp_executors_test_reinitialized_timers_cpp/

@apojomovsky see here the linter failures

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

apojomovsky · 2024-08-05T21:27:57Z

https://ci.ros2.org/job/ci_linux/21619/testReport/junit/rclcpp/uncrustify/test_rclcpp_executors_test_reinitialized_timers_cpp/

@apojomovsky see here the linter failures

Thanks Alberto, I just pushed some changes addressing the findings. It should be good to go now.

apojomovsky · 2024-08-07T21:31:22Z

Retriggered CI due to what appears to be an unrelated failure in Windows:

Linux
Linux-aarch64
Linux-rhel
Windows

(thanks, @Crola1702 )

* Remove expired timers before updating the collection Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Add regression test for reinitialized timers bug Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Add missing includes Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Relocate test under the executors directory Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Extend test to run with all supported executors Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Adjust comment in fix to make it more generic Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Apply ament clang format to test Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Fix uncrustify findings Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> --------- Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> Co-authored-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

* Remove expired timers before updating the collection Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Add regression test for reinitialized timers bug Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Add missing includes Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Relocate test under the executors directory Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Extend test to run with all supported executors Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Adjust comment in fix to make it more generic Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Apply ament clang format to test Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Fix uncrustify findings Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> --------- Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> Co-authored-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> (cherry picked from commit 9ef9646)

* Add logs on failed take response/request (#107) * Ignore local endpoints (#131) * Refs #18846: PoC ignore local endpoints: extend RCLCPP API Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com> * Refs #18846: PoC ignore local endpoints: modify RCLCPP publish logic Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com> --------- Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com> Co-authored-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com> (cherry picked from commit 106c03a) style * Support intra-process communication: Clients & Services (ros2#1847) Signed-off-by: Mauro Passerino <mpasserino@irobot.com> (cherry picked from commit 58d2a04) Remove redundant `add_subscription` cherry pick artifact use const ref instead of ptr, add missing capacity fn more style * Jazzy recreation of "Add action client/server IPC support" (aeacde9) * add override attribute to some ipc methods Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> * clear intra-process manager on client/server destructors (#94) (cherry picked from commit 378223d) * move ipc lock to appropriate position in client.hpp (cherry picked from commit 9de603e) * Actions: Use ipc_setting = rclcpp::IntraProcessSetting::NodeDefault (#133) Co-authored-by: Mauro Passerino <mpasserino@irobot.com> * Allow for deferred responses with ipc (#135) * allow for deferred responses with ipc * fix send response * fix use member service ipc process * add map to store CallbackInfoVariant * add send_response to base class, set function ptr to get handle * move service intra process outside base * copy response into shared pointer for ipc * add typename for service template * remove ref signature * try without std::ref * try without ref wrapper * try emplace * make pair with variant * some cleanup * erase callback info if client invalid * add post_init_setup for services * fix extra comma * add post init setup after lifecycle node services * add documentation for ServiceIntraProcess template change * use weak ptr to service in ServiceIntraProcess * move check for valid service handle to beginning of function * add comment for callback info map (cherry picked from commit aa95a48) * check request header for intraprocess (#139) * check request header for intraprocess * set request header intraprocess to false in execute_service (cherry picked from commit a617f93) * Include namespaces in service names (#140) * Include node namespace in IPC Action service name * Include node namespace in IPC Client/Service service name --------- Co-authored-by: Mauro Passerino <mpasserino@irobot.com> (cherry picked from commit f5b2001) remove whitespace in service.hpp * Fix mutltiple client requests (#142) * store map of unique request id to client id and callback info pair * fix map end check * fix undefined reference * remove unnecessary request id erase, remove/fix unique id comment * improve unique id comment (cherry picked from commit b865383) * RECREATION OF Fixes for intra-process actions (#144) action client / server ipc decrustification whitespace / line length / uncrustify - service_intra_process const correctness * add logs and minor fixes (#146) * add logs and minor fixes Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> * use >0 rather than ==1 in comparison Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> --------- Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> * correct template syntax Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> (cherry picked from commit d4dd4e4) * avoid adding notify waitable twice to events-executor collection (ros2#2564) * avoid adding notify waitable twice to events-executor entities collection Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> * remove redundant mutex lock Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> --------- Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> (cherry picked from commit f27bdbf) * Fix bug in timers lifecycle for events executor (ros2#2586) * Remove expired timers before updating the collection Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Add regression test for reinitialized timers bug Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Add missing includes Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Relocate test under the executors directory Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Extend test to run with all supported executors Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Adjust comment in fix to make it more generic Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Apply ament clang format to test Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> * Fix uncrustify findings Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> --------- Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> Co-authored-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> (cherry picked from commit 9ef9646) * Bring lock_free_events_queue to rclcpp from the events_executor repo (#149) Co-authored-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> (cherry picked from commit 30050c1) exclude lock free queue from linting actually lint lock_free_events_queue lock_free_events_queue copyright whitespace / line length / uncrustify - lock_free_events_queue * Add test_actions (#150) Co-authored-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> (cherry picked from commit aef928d) * Jazzy - Action IPC Fixes (See 7a51f00) whitespace / line length / uncrustify - rclcpp_action client whitespace / line length / uncrustify - rclcpp_action server fix duplicate member definitions in rclcpp_action server.hpp fixes for IPC action server delete log whitespace / line length / uncrustify - rclcpp action server again minor action client fixes whitespace / line length / uncrustify - rclcpp action client again * Always publish inter-process on TRANSIENT_LOCAL pubs (#152) * Mauro/irobot iron fixes (#155) whitespace / line length / uncrustify - publisher proper inter_process_publish_needed whitespace / line length / uncrustify - rclcpp publisher * Add test_actions & test_transient_local (#157) Co-authored-by: Mauro Passerino <mpasserino@irobot.com> (cherry picked from commit cf182e0) whitespace / line length / uncrustify - test_actions and test_transient_local whitespace / line length / uncrustify - test actions and transient_local again remove redundant test_actions * Call service post_init_setup in test_service.cpp --------- Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com> Co-authored-by: Mauro Passerino <mpasserino@irobot.com> Co-authored-by: mauropasse <mauropasse@hotmail.com> Co-authored-by: Alberto Soragna <alberto.soragna@gmail.com> Co-authored-by: Jeffery Hsu <jefferyyjhsu@gmail.com> Co-authored-by: bpwilcox <bpwonline28@gmail.com> Co-authored-by: Alexis Pojomovsky <apojomovsky@gmail.com> Co-authored-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

apojomovsky requested review from hidmic, ivanpauno and wjwwood as code owners July 22, 2024 21:09

ahcorde reviewed Jul 22, 2024

View reviewed changes

alsora reviewed Jul 22, 2024

View reviewed changes

rclcpp/test/rclcpp/test_reinitialized_timers.cpp Outdated Show resolved Hide resolved

alsora reviewed Jul 22, 2024

View reviewed changes

rclcpp/src/rclcpp/experimental/executors/events_executor/events_executor.cpp Outdated Show resolved Hide resolved

alsora self-assigned this Jul 22, 2024

apojomovsky added 6 commits August 2, 2024 17:43

Remove expired timers before updating the collection

9ee20da

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

Add regression test for reinitialized timers bug

df26444

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

Add missing includes

e6a2594

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

Relocate test under the executors directory

1c66339

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

Extend test to run with all supported executors

e794c4e

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

Adjust comment in fix to make it more generic

6775486

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

apojomovsky force-pushed the apojomovsky/fix-reinitialized-timers branch from 4b962ef to 52eb7e9 Compare August 2, 2024 22:05

Apply ament clang format to test

9dcceee

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

apojomovsky force-pushed the apojomovsky/fix-reinitialized-timers branch from 52eb7e9 to 9dcceee Compare August 2, 2024 22:11

alsora reviewed Aug 5, 2024

View reviewed changes

rclcpp/src/rclcpp/experimental/executors/events_executor/events_executor.cpp Show resolved Hide resolved

Fix uncrustify findings

c810487

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

alsora approved these changes Aug 7, 2024

View reviewed changes

alsora merged commit 496c455 into ros2:rolling Aug 8, 2024

apojomovsky deleted the apojomovsky/fix-reinitialized-timers branch August 8, 2024 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in timers lifecycle for events executor#2586

Fix bug in timers lifecycle for events executor#2586
alsora merged 8 commits intoros2:rollingfrom
apojomovsky:apojomovsky/fix-reinitialized-timers

apojomovsky commented Jul 22, 2024

Uh oh!

apojomovsky commented Jul 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alsora commented Jul 22, 2024 •

edited

Loading

Uh oh!

apojomovsky commented Aug 2, 2024

Uh oh!

Uh oh!

alsora commented Aug 5, 2024 •

edited

Loading

Uh oh!

alsora commented Aug 5, 2024

Uh oh!

apojomovsky commented Aug 5, 2024

Uh oh!

apojomovsky commented Aug 7, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

apojomovsky commented Jul 22, 2024

Uh oh!

apojomovsky commented Jul 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alsora commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apojomovsky commented Aug 2, 2024

Uh oh!

Uh oh!

alsora commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alsora commented Aug 5, 2024

Uh oh!

apojomovsky commented Aug 5, 2024

Uh oh!

apojomovsky commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alsora commented Jul 22, 2024 •

edited

Loading

alsora commented Aug 5, 2024 •

edited

Loading

apojomovsky commented Aug 7, 2024 •

edited

Loading