sequence number of service-request is not initialized to one in a unit test on ROS 2 build farm for Rolling release

[JanStaschulat]

Bug report

Required Info:

• Operating System:
  • ROS build farm for Rolling
• Installation type:
  • binary
• Version or commit hash:
  • release version: Rolling
• DDS implementation:
  • Fast-RTPS

Steps to reproduce issue

Running the unit tests of ros2/rclc on ROS 2 build farm for Rolling release fails, because the sequence number
for a service-request ist not initialized to 1:
https://github.com/ros2/rclc/blob/0bbab9694760f5685413960e93b122c2980eb36e/rclc/test/rclc/test_executor.cpp#L1881

TEST_F(TestDefaultExecutor, executor_test_service) {
  // This unit test tests, if a request from a client is received by the executor
  // and the corresponding service callback is called
  // the value of the request message is checked.
  rcl_ret_t rc;
  rclc_executor_t executor;
  executor = rclc_executor_get_zero_initialized_executor();
  rc = rclc_executor_init(&executor, &this->context, 10, this->allocator_ptr);
  EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str;
  const char * service_name = "addtwoints";
  rcl_service_options_t service_options = rcl_service_get_default_options();
  rcl_service_t service = rcl_get_zero_initialized_service();
  const rosidl_service_type_support_t * service_type_support =
    ROSIDL_GET_SRV_TYPE_SUPPORT(example_interfaces, srv, AddTwoInts);
  rc =
    rcl_service_init(&service, &this->node, service_type_support, service_name, &service_options);
  EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str;
  example_interfaces__srv__AddTwoInts_Request req;
  example_interfaces__srv__AddTwoInts_Request__init(&req);
  example_interfaces__srv__AddTwoInts_Response resp;
  example_interfaces__srv__AddTwoInts_Response__init(&resp);
  EXPECT_EQ(executor.info.number_of_clients, (size_t) 0);
  EXPECT_EQ(executor.info.number_of_services, (size_t) 0);
  rc = rclc_executor_add_service(&executor, &service, &req, &resp, &service_callback);
  EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str;
  EXPECT_EQ(executor.info.number_of_clients, (size_t) 0);
  EXPECT_EQ(executor.info.number_of_services, (size_t) 1);
  // Creating client and options
  rcl_client_options_t client_options = rcl_client_get_default_options();
  rcl_client_t client = rcl_get_zero_initialized_client();
  rc = rcl_client_init(&client, &this->node, service_type_support, service_name, &client_options);
  EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str;
  // client messages
  example_interfaces__srv__AddTwoInts_Request cli_req;
  example_interfaces__srv__AddTwoInts_Request__init(&cli_req);
  example_interfaces__srv__AddTwoInts_Response cli_resp;
  example_interfaces__srv__AddTwoInts_Response__init(&cli_resp);
  // add client to executor
  rc = rclc_executor_add_client(&executor, &client, &cli_resp, client_callback);
  EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str;
  EXPECT_EQ(executor.info.number_of_clients, (size_t) 1);
  EXPECT_EQ(executor.info.number_of_services, (size_t) 1);
  // send client request
  int64_t seq;
  cli_req.a = 1;
  cli_req.b = 2;
  rc = rcl_send_request(&client, &cli_req, &seq);
  EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str;
  EXPECT_EQ(seq, (int64_t) 1);  // sequence id = 1

Expected behavior

 EXPECT_EQ(seq, (int64_t) 1);  // sequence id = 1

returns true

Actual behavior

Output of ros2/rclc unit test - build job on ROS 2 build farm, Rolling:
https://build.ros2.org/job/Rpr__rclc__ubuntu_focal_amd64/71/testReport/junit/rclc/TestDefaultExecutor/executor_test_service/

/tmp/ws/src/rclc/rclc/test/rclc/test_executor.cpp:1881
Expected equality of these values:
  seq
    Which is: 3
  (int64_t) 1
    Which is: 1

Also all other unit tests, in which the sequence number is checked, fail:
https://build.ros2.org/job/Rpr__rclc__ubuntu_focal_amd64/71/#showFailuresLink

Additional information

See also our issue at ros2/rclc: ros2/rclc#49

As a work-around, I commented out this check for all instances, where we check the sequence-number of the service-request:
ros2/rclc@28a1224#diff-13237951e52bc2adab028d9061ad1832272d2e3d80a45bc3fce707c79e5a8d90R1881

The unit test works for Github Action CI-job for Dashing, Foxy, Rolling:
e.g.: https://github.com/ros2/rclc/actions/runs/574004890

The unit test works for ROS 2 build farm CI-job for Dashing and Foxy:
Dashing: https://build.ros2.org/job/Dpr__rclc__ubuntu_bionic_amd64/76/
Foxy: https://build.ros2.org/job/Fpr__rclc__ubuntu_focal_amd64/71/
Rolling (fails): https://build.ros2.org/job/Rpr__rclc__ubuntu_focal_amd64/71/#showFailuresLink

[fujitatomoya]

@JanStaschulat

is this issue supposed to be against rclc, not rcl? if that so, could you move this issue to rclc?

[fujitatomoya]

i think i am the one who is confused. so you like to uncomment those skipped tests to be available but facing test failure as following.

     4 FAILED TESTS
    -- run_test.py: return code 1
    -- run_test.py: inject classname prefix into gtest result file '/root/docker_ws/ros2_colcon/buil
d/rclc/test_results/rclc/rclc_test.gtest.xml'
    -- run_test.py: verify result file '/root/docker_ws/ros2_colcon/build/rclc/test_results/rclc/rcl
c_test.gtest.xml'
  >>>
build/rclc/test_results/rclc/rclc_test.gtest.xml: 36 tests, 0 errors, 4 failures, 0 skipped
- rclc.Test rclc_client_init_best_effort
  <<< failure message
    /root/docker_ws/ros2_colcon/src/ros2/rclc/rclc/test/rclc/test_client.cpp:113
    Expected equality of these values:
      sequence_number
        Which is: 2
      1
  >>>
- rclc.TestDefaultExecutor executor_test_service
  <<< failure message
    /root/docker_ws/ros2_colcon/src/ros2/rclc/rclc/test/rclc/test_executor.cpp:1881
    Expected equality of these values:
      seq
        Which is: 3
...<snip>

my bad...: 😢

anyway i confirmed that this problem still happens. AFAIK, sequence number is not managed by rcl but rmw_implementation.

[fujitatomoya]

• Test Failure (source build with ros2/ros2@f431547)

     4 FAILED TESTS
    -- run_test.py: return code 1
    -- run_test.py: inject classname prefix into gtest result file '/root/docker_ws/ros2_colcon/buil
d/rclc/test_results/rclc/rclc_test.gtest.xml'
    -- run_test.py: verify result file '/root/docker_ws/ros2_colcon/build/rclc/test_results/rclc/rcl
c_test.gtest.xml'
  >>>
build/rclc/test_results/rclc/rclc_test.gtest.xml: 36 tests, 0 errors, 4 failures, 0 skipped
- rclc.Test rclc_client_init_best_effort
  <<< failure message
    /root/docker_ws/ros2_colcon/src/ros2/rclc/rclc/test/rclc/test_client.cpp:113
    Expected equality of these values:
      sequence_number
        Which is: 2
      1
  >>>
- rclc.TestDefaultExecutor executor_test_service
  <<< failure message
    /root/docker_ws/ros2_colcon/src/ros2/rclc/rclc/test/rclc/test_executor.cpp:1881
    Expected equality of these values:
      seq
        Which is: 3
      (int64_t) 1
        Which is: 1
  >>>
- rclc.TestDefaultExecutor executor_test_service_with_reqid
  <<< failure message
    /root/docker_ws/ros2_colcon/src/ros2/rclc/rclc/test/rclc/test_executor.cpp:1980
    Expected equality of these values:
      seq
        Which is: 4
      (int64_t) 1
        Which is: 1
  >>>
- rclc.TestDefaultExecutor executor_test_service_with_context
  <<< failure message
    /root/docker_ws/ros2_colcon/src/ros2/rclc/rclc/test/rclc/test_executor.cpp:2084
    Expected equality of these values:
      seq
        Which is: 5
      (int64_t) 1
        Which is: 1
  >>>

Summary: 137 tests, 0 errors, 5 failures, 0 skipped

I think that this is because of rmw_cyclonedds static local variable used, and i think this is correct for implementation to cover the requirement. (saying this is not a bug.)
https://github.com/ros2/rmw_cyclonedds/blob/d024823043504ea40af24bf22365a21cd203df55/rmw_cyclonedds_cpp/src/rmw_node.cpp#L3731

this means that as long as the test uses the same process space, sequence id will not be reset even if rcl_client_t is initialized. using export RMW_IMPLEMENTATION=rmw_fastrtps_cpp does not meet this situation.

 * \param[out] sequence_id Sequence number for the `ros_request` just sent
 *   i.e. a unique identification number for it, populated on success.

in rmw header, it tells us at least unique identification number but it does not specify this is mapped to rcl_client_t or just a unique. IMO, this is just required to be a unique, so that application proceeds based on that unique identification. i even think that process wide unique identification would be better to have for uniqueness and debug purpose.

after all, i do not think this is bug for either rcl or rmw. test program should be adjusted based on unique identification provided via rcl_send_request.

[JanStaschulat]

@JanStaschulat

is this issue supposed to be against rclc, not rcl? if that so, could you move this issue to rclc?

@fujitatomoya. No this is an issue of rcl, because the sequence number is not initialized to one after calling rcl_send_request(&client, &cli_req, &seq); for the first time. But as you explained, the specification does not say to which value it is initialized to. I guess, it just happened to be "1" for the releases Dashing and Foxy and is not re-initialized for Rolling.

[JanStaschulat]

i think i am the one who is confused. so you like to uncomment those skipped tests to be available but facing test failure as following.

Yes, we faced these errors, thats why I commented out the check for the sequence-id. Interestingly, it only fails at the ROS 2 build farm for Rolling. It works fine for Dashing, Foxy, Rolling with Github Action and Dashing, Foxy on ROS 2 build farm.

[JanStaschulat]

"after all, i do not think this is bug for either rcl or rmw. test program should be adjusted based on unique identification provided via rcl_send_request."

Okay, fine for me. We will not check for sequence-number to be a particular value after calling rcl_send_request.

[fujitatomoya]

i think we can close this issue, feel free to reopen.