Fix mutltiple client requests #142

bpwilcox · 2024-02-16T06:26:44Z

No description provided.

alsora · 2024-02-16T14:42:18Z

rclcpp/include/rclcpp/experimental/service_intra_process.hpp

+        "Calling intra_process_service_send_response for invalid or no "
+        "longer existing request id");
+
+      callback_info_.erase(client_request_id);


Why the erase if we didn't find the request?

this was a copy, can be removed

alsora · 2024-02-16T14:45:14Z

rclcpp/include/rclcpp/experimental/service_intra_process.hpp

+    }
+
+    auto intra_process_client_id = client_request_it->second.first;
    auto client_it = clients_.find(intra_process_client_id);


Unrelated to your PR, but why the services store pointers to clients? Shouldn't we ask the intra process manager to give us the corresponding client, to avoid having to duplicate information and having to maintain the lists up to date?

I do not know why there appears to be this duplication, but I'd rather not touch that in this PR. Also, perhaps @mauropasse can confirm if this is unnecessary.

Yes that's should be corrected. I can take care once we finish this work and mine about Actions.

alsora · 2024-02-16T14:48:42Z

rclcpp/src/rclcpp/service_intra_process_base.cpp

 }
+
+uint64_t
+ServiceIntraProcessBase::get_unique_request_id()


I assume this implementation is copied from the intra process manager.
We should remove the comment and consider a different implementation.
The intra process manager is one per process and increases the counter when you add entities, so the overflow is unreachable.

The counter here shouldn't be static, as it will be shared across all servers, which is not needed.
It should be a member variable

I think the implementation is fair and a common one. I can agree that a counter across all servers may be unneeded, though not sure it truly matters. Perhaps one reason would be to avoid user error accidentally passing a request id from one service into another (which could happen).

alsora · 2024-02-16T14:49:16Z

rclcpp/src/rclcpp/service_intra_process_base.cpp

+    // If we roll over then it's most likely a bug.
+    // *INDENT-OFF* (prevent uncrustify from making unnecessary indents here)
+    throw std::overflow_error(
+      "exhausted the unique id's for publishers and subscribers in this process "


The text of this exception is wrong.

Sorry, I just copied, will change

* 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

* 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)

* 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 <[email protected]> * Refs #18846: PoC ignore local endpoints: modify RCLCPP publish logic Signed-off-by: JLBuenoLopez-eProsima <[email protected]> --------- Signed-off-by: JLBuenoLopez-eProsima <[email protected]> Co-authored-by: JLBuenoLopez-eProsima <[email protected]> (cherry picked from commit 106c03a) style * Support intra-process communication: Clients & Services (ros2#1847) Signed-off-by: Mauro Passerino <[email protected]> (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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> (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 <[email protected]> * use >0 rather than ==1 in comparison Signed-off-by: Alberto Soragna <[email protected]> --------- Signed-off-by: Alberto Soragna <[email protected]> * correct template syntax Signed-off-by: Alberto Soragna <[email protected]> (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 <[email protected]> * remove redundant mutex lock Signed-off-by: Alberto Soragna <[email protected]> --------- Signed-off-by: Alberto Soragna <[email protected]> (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 <[email protected]> * Add regression test for reinitialized timers bug Signed-off-by: Alexis Pojomovsky <[email protected]> * Add missing includes Signed-off-by: Alexis Pojomovsky <[email protected]> * Relocate test under the executors directory Signed-off-by: Alexis Pojomovsky <[email protected]> * Extend test to run with all supported executors Signed-off-by: Alexis Pojomovsky <[email protected]> * Adjust comment in fix to make it more generic Signed-off-by: Alexis Pojomovsky <[email protected]> * Apply ament clang format to test Signed-off-by: Alexis Pojomovsky <[email protected]> * Fix uncrustify findings Signed-off-by: Alexis Pojomovsky <[email protected]> --------- Signed-off-by: Alexis Pojomovsky <[email protected]> Co-authored-by: Alexis Pojomovsky <[email protected]> (cherry picked from commit 9ef9646) * Bring lock_free_events_queue to rclcpp from the events_executor repo (#149) Co-authored-by: Alexis Pojomovsky <[email protected]> (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 <[email protected]> (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 <[email protected]> (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 <[email protected]> Signed-off-by: Alexis Pojomovsky <[email protected]> Co-authored-by: Mauro Passerino <[email protected]> Co-authored-by: mauropasse <[email protected]> Co-authored-by: Alberto Soragna <[email protected]> Co-authored-by: Jeffery Hsu <[email protected]> Co-authored-by: bpwilcox <[email protected]> Co-authored-by: Alexis Pojomovsky <[email protected]> Co-authored-by: Alexis Pojomovsky <[email protected]>

bpwilcox added 3 commits February 15, 2024 22:31

store map of unique request id to client id and callback info pair

fd6b8a1

fix map end check

f5efe79

fix undefined reference

780e486

alsora reviewed Feb 16, 2024

View reviewed changes

bpwilcox added 2 commits February 16, 2024 13:58

remove unnecessary request id erase, remove/fix unique id comment

d343fc0

improve unique id comment

20c66ca

alsora approved these changes Feb 16, 2024

View reviewed changes

bpwilcox merged commit 3fb7f8b into irobot-ros:irobot/humble Feb 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix mutltiple client requests #142

Fix mutltiple client requests #142

Uh oh!

bpwilcox commented Feb 16, 2024

Uh oh!

alsora Feb 16, 2024

Uh oh!

bpwilcox Feb 16, 2024

Uh oh!

alsora Feb 16, 2024

Uh oh!

bpwilcox Feb 16, 2024

Uh oh!

mauropasse Feb 20, 2024

Uh oh!

alsora Feb 16, 2024

Uh oh!

bpwilcox Feb 16, 2024

Uh oh!

alsora Feb 16, 2024

Uh oh!

bpwilcox Feb 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix mutltiple client requests #142

Fix mutltiple client requests #142

Uh oh!

Conversation

bpwilcox commented Feb 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants