Make ClientData thread-safe #293

Yadunund · 2024-10-09T22:28:18Z

Make access to ClientData thread-safe.

I've not fully tested this PR yet but I know colcon test --event-handlers console_direct+ --packages-select rclcpp_action --ctest-args -R test_server fails. It has to do with how i'm handling cases where the client is shutdown when there are still queries in flight.

ahcorde

fix conflicts

Yadunund · 2024-10-12T01:10:55Z

@ahcorde could you help check if the expected tests are passing here?

ahcorde · 2024-10-14T11:12:41Z

ros2.repos

rmw_zenoh/yadu/raii-service

rmw_zenoh commit: aaa01c1

ROS Package	Failing test name	Failing test output
rcl	None	None
rcl_action	None	None
rcl_lifecycle	None	None
rcl_yaml_param_parser	None	None
rclcpp	test_intra_process_manager	Failed (Check logs)
rclcpp	test_node_interfaces__node_graph	Failed (Check logs)
rclcpp	test_publisher	Failed (Check logs)
rclcpp	test_events_executor	Timeout (Check logs)
rclcpp_action	None	None
rclcpp_components	None	None
rclcpp_lifecycle	None	-
rmw_zenoh_cpp	None	None
test_cli	None	None
test_cli_remap	test_cli_remapping	TBD
rclpy	None

clalancette · 2024-10-16T15:33:40Z

Because of #298, this now needs a rebase and conflicts resolved. Once that is done, I'll do another review pass over it.

Yadunund · 2024-10-16T15:47:54Z

@clalancette i've resolved the conflicts!

rmw_zenoh_cpp/src/detail/graph_cache.cpp

rmw_zenoh_cpp/src/rmw_zenoh.cpp

clalancette · 2024-10-16T16:45:41Z

rmw_zenoh_cpp/src/rmw_zenoh.cpp

+      type_support,
+      qos_profile))
+  {
+    // Error already handled.
    return nullptr;
  }



If a step below fails, do we need to remove this client from the node? In other words, do we need a rcpputils::scope_exit block here?

Ah nope! The shared_ptr we created within create_client_data will go out of scope and the destructor will clean everything up. The free_rmw_client block will take care of deallocating the rmw_client_t object.

rmw_zenoh_cpp/src/rmw_zenoh.cpp

clalancette · 2024-10-16T16:51:58Z

rmw_zenoh_cpp/src/rmw_zenoh.cpp

  if (client_data == nullptr) {
    RMW_SET_ERROR_MSG_WITH_FORMAT_STRING(
      "Unable to retreive client_data from client for service %s", client->service_name);
    return RMW_RET_INVALID_ARGUMENT;
  }

-  std::string service_type = client_data->request_type_support->get_name();
-  size_t suffix_substring_position = service_type.find("Request_");


I notice that we are no longer checking whether the service_type has Request_ in it. Do we need to add that back in?

So this check was moved inside ClientData::make() but there was typo there anyways which I have fixed 4c9b106. It would not have caused any problems since the substring we extract will be the same but now it's semantically correct.

rmw_zenoh_cpp/src/detail/rmw_client_data.cpp

clalancette

Overall, I like the new direction here. I have some specific concerns on some of the data handling inline.

rmw_zenoh_cpp/src/detail/rmw_client_data.cpp

Yadunund · 2024-11-01T18:07:00Z

nav2 demo crashes when the following steps are performed

ros2 launch nav2_bringup tb3_simulation_launch.py headless:=False
Localize the robot and wait for startup to complete
Click Reset from the nav2 panel in RViz

[component_container_isolated-8] malloc(): unsorted double linked list corrupted
[component_container_isolated-8] Magick: abort due to signal 6 (SIGABRT) "Abort"...
[ERROR] [component_container_isolated-8]: process has died [pid 2257204, exit code -6, cmd '/opt/ros/rolling/lib/rclcpp_components/component_container_isolated --ros-args --log-level info --ros-args -r __node:=nav2_container --params-file /tmp/launch_params_xwa4_atv --params-file /tmp/launch_params_3tgew0_b -r /tf:=tf -r /tf_static:=tf_static'].

Yadunund · 2024-11-01T18:15:30Z

It seems like 082e64c introduced this behavior as I can't reproduce the crash if I drop that commit.

ahcorde

conflicts

clalancette · 2024-11-08T15:26:52Z

conflicts

I've now rebased it onto the latest.

clalancette · 2024-11-08T15:33:23Z

Besides rebasing this, I also did a local test where I rebased this onto #307 , and also reverted 19b84e9 . But I am running into shutdown issues in client_data_handler where it is deadlocking, so I'll need to look into that further.

Signed-off-by: Yadunund <[email protected]>

* Additional fixes for yadu/raii-client. Signed-off-by: Chris Lalancette <[email protected]> * Apply suggestions from code review Co-authored-by: yadunund <[email protected]> Signed-off-by: Chris Lalancette <[email protected]> * Fixups. Signed-off-by: Chris Lalancette <[email protected]> --------- Signed-off-by: Chris Lalancette <[email protected]> Co-authored-by: yadunund <[email protected]>

Signed-off-by: Yadunund <[email protected]>

This just makes it more consistent with the rest of the APIs. Signed-off-by: Chris Lalancette <[email protected]>

Signed-off-by: Yadunund <[email protected]>

We basically use the pointer as a key to lookup the shared_ptr, and then always use the shared_ptr. That ensures that even if the pointer was dropped from the node while the client callback is operating, it will still be valid and won't be destroyed until that method returns. Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2024-11-12T19:29:37Z

All right, I've just rebased this and also pushed another commit to vastly simplify our handling of in-flight data. I think this should work a lot more reliably now. With that, I'm going to approve and merge this. Thanks for the long slog on this one.

Yadunund force-pushed the yadu/raii-client branch from 61dafb0 to 7bd5073 Compare October 9, 2024 23:12

Base automatically changed from yadu/raii-service to rolling October 10, 2024 21:32

ahcorde requested changes Oct 11, 2024

View reviewed changes

Yadunund force-pushed the yadu/raii-client branch from 7bd5073 to b09fa97 Compare October 11, 2024 17:04

ahcorde approved these changes Oct 14, 2024

View reviewed changes

clalancette requested changes Oct 16, 2024

View reviewed changes

Yadunund requested a review from clalancette October 18, 2024 02:41

Yadunund force-pushed the yadu/raii-client branch from 51f2917 to 37e2e03 Compare October 18, 2024 23:02

clalancette requested changes Oct 28, 2024

View reviewed changes

rmw_zenoh_cpp/src/detail/rmw_client_data.cpp Outdated Show resolved Hide resolved

rmw_zenoh_cpp/src/detail/rmw_client_data.cpp Outdated Show resolved Hide resolved

rmw_zenoh_cpp/src/detail/rmw_client_data.cpp Outdated Show resolved Hide resolved

clalancette mentioned this pull request Nov 4, 2024

Fix the received_timestamp for client data. #305

Merged

ahcorde requested changes Nov 8, 2024

View reviewed changes

clalancette force-pushed the yadu/raii-client branch from 0630d06 to 6234eb2 Compare November 8, 2024 15:26

Yadunund and others added 10 commits November 12, 2024 18:59

Make ClientData thread-safe

474eb97

Signed-off-by: Yadunund <[email protected]>

Handle queries in flight

a7a173c

Signed-off-by: Yadunund <[email protected]>

Address some feedback

5b17de6

Signed-off-by: Yadunund <[email protected]>

Fix service client type check

5e4a54f

Signed-off-by: Yadunund <[email protected]>

fix tracking of num in flight without UB

aedffb8

Signed-off-by: Yadunund <[email protected]>

Fix -Wnon-pod-varargs

5873d70

Signed-off-by: Yadunund <[email protected]>

Make PublisherData::copy_gid have the same signatures as others.

8a4ea55

This just makes it more consistent with the rest of the APIs. Signed-off-by: Chris Lalancette <[email protected]>

Add api to init client

bb69941

Signed-off-by: Yadunund <[email protected]>

clalancette force-pushed the yadu/raii-client branch from c4887d7 to 821f4e9 Compare November 12, 2024 19:08

clalancette approved these changes Nov 12, 2024

View reviewed changes

clalancette merged commit 91edead into rolling Nov 12, 2024
8 checks passed

clalancette deleted the yadu/raii-client branch November 12, 2024 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ClientData thread-safe #293

Make ClientData thread-safe #293

Yadunund commented Oct 9, 2024 •

edited

Loading

ahcorde left a comment

Yadunund commented Oct 12, 2024

ahcorde commented Oct 14, 2024

clalancette commented Oct 16, 2024

Yadunund commented Oct 16, 2024

clalancette Oct 16, 2024

Yadunund Oct 16, 2024

clalancette Oct 16, 2024

Yadunund Oct 16, 2024

clalancette left a comment

Yadunund commented Nov 1, 2024

Yadunund commented Nov 1, 2024

ahcorde left a comment

clalancette commented Nov 8, 2024

clalancette commented Nov 8, 2024

clalancette commented Nov 12, 2024

Make ClientData thread-safe #293

Make ClientData thread-safe #293

Conversation

Yadunund commented Oct 9, 2024 • edited Loading

ahcorde left a comment

Choose a reason for hiding this comment

Yadunund commented Oct 12, 2024

ahcorde commented Oct 14, 2024

rmw_zenoh/yadu/raii-service

clalancette commented Oct 16, 2024

Yadunund commented Oct 16, 2024

clalancette Oct 16, 2024

Choose a reason for hiding this comment

Yadunund Oct 16, 2024

Choose a reason for hiding this comment

clalancette Oct 16, 2024

Choose a reason for hiding this comment

Yadunund Oct 16, 2024

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Yadunund commented Nov 1, 2024

Yadunund commented Nov 1, 2024

ahcorde left a comment

Choose a reason for hiding this comment

clalancette commented Nov 8, 2024

clalancette commented Nov 8, 2024

clalancette commented Nov 12, 2024

Yadunund commented Oct 9, 2024 •

edited

Loading