rosbag2_cpp.TimeControllerClockTest.unpaused_sleep_returns_true test fails on Windows

[Blast545]

Description

This test is failing in the repeated jobs of the buildfarm:
https://ci.ros2.org/view/nightly/job/nightly_win_rep/2588/testReport/junit/rosbag2_cpp/TimeControllerClockTest/unpaused_sleep_returns_true/

This is the code of the test:

rosbag2/rosbag2_cpp/test/rosbag2_cpp/test_time_controller_clock.cpp

Line 154 in 5fa3a67

EXPECT_TRUE(clock.sleep_until(clock.now() + RCUTILS_S_TO_NS(1)));

From what I can see in the API,

rosbag2/rosbag2_cpp/include/rosbag2_cpp/clocks/time_controller_clock.hpp

Line 70 in 5fa3a67

bool sleep_until(rcutils_time_point_value_t until) override;

The function may return false indicating the time was not completed, is it there any way we can change the test to reflect this? or am I missing something?

Expected Behavior

Test passes.

Actual Behavior

Test fails in the repeated jobs buildfarm.

System (please complete the following information)

• OS: Windows
• ROS 2 Distro: Rolling
• Version: main

[MichaelOrlov]

@Blast545 The test fails because we are using std::condition_variable_any::wait_until in https://github.com/ros2/rosbag2/blob/0.15.1/rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp#L177 and according to the https://en.cppreference.com/w/cpp/thread/condition_variable_any/wait_until
"wait_until causes the current thread to block until the condition variable is notified, a specific time is reached, or a spurious wakeup occurs"
Which is more likely to happen on Windows platform due to the OS and internal scheduler specific.

It's ok from design and our API point of view if our sleep_until(rcutils_time_point_value_t until) will periodically wake up and return false as long as it respect target sleep time.

Although we can take it in to account in test. Something like this:

  clock.pause();
  clock.resume();
  auto sleep_until_timestamp = clock.now() + RCUTILS_S_TO_NS(1);
  auto start = std::chrono::steady_clock::now();
  bool sleep_result = clock.sleep_until(sleep_until_timestamp);
  // clock.sleep_until can periodically wake up and return false. Check for true in time constrain
  while (rclcpp::ok() && !sleep_result &&
         (std::chrono::steady_clock::now() - start) < std::chrono::milliseconds(1200))
  {
    sleep_result = clock.sleep_until(sleep_until_timestamp);
  }
  auto end = std::chrono::steady_clock::now();
  EXPECT_TRUE(end - start < std::chrono::milliseconds(1200));
  EXPECT_TRUE(end - start >= std::chrono::seconds(1));
  EXPECT_TRUE(sleep_result);

[Blast545]

@MichaelOrlov To me it makes total sense to rewrite the test using the snippet of code you attached, it better reflects the usage of the API. Thanks for digging into this!

Let me know if you can open a PR with this (so the code gets merged with you as author) or if you'd prefer me to handle it.

[MichaelOrlov]

@Blast545 I would appreciate if you would do it by yourself.
I don't mind about who will be authoring it.
I am honestly have a lot on my plate and can't keep up with all tasks which need to do.