Skip to content

avoid adding notify waitable twice to events-executor collection #2564

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.

Sign up for GitHub

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

Jump to bottom
Merged
alsora merged 2 commits into from
Jun 29, 2024
Merged

avoid adding notify waitable twice to events-executor collection #2564

alsora merged 2 commits into from
Jun 29, 2024

Conversation

@alsora
Copy link
Collaborator

@alsora alsora commented Jun 19, 2024

This PR fixes a bug noticed by @wjwwood as part of his PR here

We don't need to add the notify waitable to the current entities collection list there, because it has already been added in the constructor.
The PR includes a unit-test that would detect bugs if the constructor insertion was removed.

@alsora
avoid adding notify waitable twice to events-executor entities collec…
7e9ba2b 
…tion

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora alsora changed the title avoid adding notify waitable twice to events-executor entities collec… avoid adding notify waitable twice to events-executor collection Jun 19, 2024
@alsora alsora mentioned this pull request Jun 19, 2024
Open
mjcarroll
mjcarroll approved these changes Jun 20, 2024
View reviewed changes
Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@alsora
Copy link
Collaborator Author

alsora commented Jun 21, 2024

CI:

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

@alsora
remove redundant mutex lock
cbde5da 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
fujitatomoya
fujitatomoya approved these changes Jun 22, 2024
View reviewed changes
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.

lgtm with green CI

@alsora
Copy link
Collaborator Author

alsora commented Jun 22, 2024

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

@alsora alsora mentioned this pull request Jun 24, 2024
Merged
@alsora
Copy link
Collaborator Author

alsora commented Jun 24, 2024

  • Linux-rhel Build Status

@alsora
Copy link
Collaborator Author

alsora commented Jun 28, 2024 •
edited
Loading

The linux-rhel failure in test_tracetools.test_tracetools.test.test_generic_pub_sub.TestGenericPubSub.test_all (from pytest) seems unrelated.
But triggering another time just in case.

  • Linux-rhel Build Status

EDIT: try again, although this seems very flaky

  • Linux-rhel Build Status

@alsora
Copy link
Collaborator Author

alsora commented Jun 29, 2024

Merging with green CI.
The instability of Linux Rhel seems unrelated to the changes here

@alsora alsora merged commit 8de4b90 into rolling Jun 29, 2024
@alsora alsora deleted the asoragna/events-executor-notify-waitable branch June 29, 2024 21:55
@Crola1702 Crola1702 mentioned this pull request Jul 1, 2024
Closed
apojomovsky pushed a commit to apojomovsky/rclcpp that referenced this pull request Jul 8, 2024
@alsora @apojomovsky
avoid adding notify waitable twice to events-executor collection (ros…
f27bdbf 
…2#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>
skyegalaxy pushed a commit to irobot-ros/rclcpp that referenced this pull request Mar 29, 2025
@alsora @skyegalaxy
avoid adding notify waitable twice to events-executor collection (ros…
bab1ea2 
…2#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)
skyegalaxy pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 3, 2025
@alsora @skyegalaxy
avoid adding notify waitable twice to events-executor collection (ros…
ba5a1c0 
…2#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)
skyegalaxy pushed a commit to irobot-ros/rclcpp that referenced this pull request May 14, 2025
@alsora @skyegalaxy
avoid adding notify waitable twice to events-executor collection (ros…
6b1d98b 
…2#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)
skyegalaxy pushed a commit to irobot-ros/rclcpp that referenced this pull request May 19, 2025
@alsora @skyegalaxy
avoid adding notify waitable twice to events-executor collection (ros…
c419bc6 
…2#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)
skyegalaxy added a commit to irobot-ros/rclcpp that referenced this pull request May 20, 2025
@skyegalaxy @mauropasse
@alsora @jefferyyjhsu @bpwilcox @apojomovsky
Cherry-picks and changes from "irobot/iron" for Jazzy (#158)
c6d04f7 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@hidmic hidmic Awaiting requested review from hidmic

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alsora @wjwwood @mjcarroll @fujitatomoya