Add transactional state mutex for RecorderImpl class.

[fujitatomoya]

address #1529 (comment)

[fujitatomoya]

@MichaelOrlov minor fix, could you take a look when you have time?

[MichaelOrlov]

@fujitatomoya Thanks for your contribution.
I have a few nitpick findings and suggestions and also have a concern about using recursive_mutex for protecting state transition.

I don't see a reason or rationale for protecting discovery start/stop operations with mutex at all. After some analysis, I came to the conclusion it would be enough to have an atomic stop_discovery_ variable.
I don't see possibilities for race conditions or undefined behavior in discovery start/stop operations even if it would be a public API and could be called from different threads.

If you agree please remove mutex lock in stop_discovery() method and use regular non-recursive mutex for the state_transition_mutex_.

[fujitatomoya]

@MichaelOrlov sorry, i did not send this to you. cached for like a week in my browser... 😓

[fujitatomoya]

@MichaelOrlov thanks for the fix, lgtm with green CI.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

@fujitatomoya There is a warning for my "hacky" cast part in the toggle_paused()

/home/morlov/ros2_rolling/src/ros2/rosbag2/rosbag2_transport/src/rosbag2_transport/recorder.cpp:417:24: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  417 |   if (atomic_fetch_xor(reinterpret_cast<std::atomic_uchar *>(&paused_), 1)) {
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Unfortunately, I was not able to find a way how to fix this warning in a good way without changing the type of the paused_ variable to the std::atomic_uchar or without rolling back to the simple if (paused_.load()) {..} else {..} statement.

I've tried to change the type to the std::atomic_uchar and seems backward implicit type conversion from bool to the std::atomic_uchar works fine without warnings and we can continue using paused_ as a boolean variable. See my latest commit 9241cc5

[MichaelOrlov]

Gist: https://gist.githubusercontent.com/MichaelOrlov/5f018e1125c75305282285cfd52d9352/raw/6c7bfb3b37e6ad32d4e0d2a521a41a02f5364390/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13483

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@MichaelOrlov green light, we are good to go.

[fujitatomoya]

@MichaelOrlov thanks for the help!

[MichaelOrlov]

@fujitatomoya Likewise! 😉