[humble] Fix for possible freeze in Recorder::stop() (backport #1381)

[mergify]

• Depends from [humble] Add recorder stop() API (backport #1300) #1396

This is an automatic backport of pull request #1381 done by Mergify.
Cherry-pick of c6cc69a has failed:

On branch mergify/bp/humble/pr-1381
Your branch is up to date with 'origin/humble'.

You are currently cherry-picking commit c6cc69a.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   rosbag2_transport/src/rosbag2_transport/recorder.cpp

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

---

Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

• @Mergifyio refresh will re-evaluate the rules
• @Mergifyio rebase will rebase this PR on its base branch
• @Mergifyio update will merge the base branch into this PR
• @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

• look at your merge queues
• generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

[MichaelOrlov]

According to the https://www.acodersjourney.com/20-abi-breaking-changes/

ABI BREAKING CHANGES
4. Add, remove, or re-order member variables in a class
5. Change the type of any member variable in a class

It seems that we have an ABI breaking changes in this PR and it shouldn't be merged.
However, need to double check with ABI-checker.

[clalancette]

It seems that we have an ABI breaking changes in this PR and it shouldn't be merged.

Yeah, it looks like it. The addition of member variables can change the layout of the underlying structure in memory, so it breaks ABI.

[MichaelOrlov]

@emersonknapp @clalancette I removed the newly added std atomic<bool> in_recording_ member variable and now API-ABI checker shows that changes in this PR are API and ABI compatible.

ABI/API compatibility report

API compatibility report between librosbag2_transport.so (X) and librosbag2_transport.so (Y) objects on x86_64

Binary
Compatibility Source
Compatibility

Test Info

---

Module Name	libTest
Version #1	X
Version #2	Y
Arch	x86_64
GCC Version	11.3.0
Subject	Binary Compatibility

Test Results

---

Total Header Files	226
Total Source Files	7
Total Objects	1
Total Symbols / Types	237 / 120
Compatibility	100%

Problem Summary

---

	Severity	Count
Added Symbols	-	0
Removed Symbols	High	0
Problems with Data Types	High	0
Medium	0
Low	0
Problems with Symbols	High	0
Medium	0
Low	0
Problems with Constants	Low	0

Header Files  226 

---

__mbstate_t.h
aligned_buffer.h
alloc_traits.h
allocated_ptr.h
allocator.h
allocator_common.hpp
allocator_deleter.hpp
any_service_callback.hpp
arguments.h
array
atomic
atomic_base.h
atomic_futex.h
atomic_wide_counter.h
atomic_word.h
atomicops.h
bag_events.hpp
bag_metadata.hpp
base_reader_interface.hpp
base_writer_interface.hpp
basic_string.h
basic_string.tcc
bitset
bounded_vector.hpp
burst__struct.hpp
c++config.h
callback_group.hpp
char_traits.h
chrono
clock.hpp
clock__struct.hpp
concurrence.h
condition_variable
context.h
context.hpp
convert.h
converter_options.hpp
cpp_callback_trampoline.hpp
create_generic_publisher.hpp
create_generic_subscription.hpp
create_publisher.hpp
ctype_base.h
demangle.hpp
deque.tcc
duration.hpp
emitterstyle.h
enable_special_members.h
error_constants.h
event.h
exception.h
exception_ptr.h
exceptions.h
exceptions.hpp
floating_point_range__struct.hpp
functional
functional_hash.h
future
generic_publisher.hpp
generic_subscription.hpp
get_node_parameters_interface.hpp
get_node_topics_interface.hpp
get_rate__struct.hpp
gthr-default.h
hashtable.h
hashtable_policy.h
impl.h
incompatible_qos.h
integer_range__struct.hpp
intra_process_buffer_type.hpp
intra_process_manager.hpp
intra_process_setting.hpp
ios_base.h
iosfwd
is_paused__struct.hpp
iterator.h
keyboard_handler_base.hpp
liveliness_changed.h
liveliness_lost.h
locale_classes.h
locale_classes.tcc
logger.hpp
mark.h
memory.h
message_initialization.hpp
message_lost.h
metadata_io.hpp
mutex
new
new_allocator.h
node.h
node_data.h
node_graph_interface.hpp
node_impl.hpp
node_iterator.h
node_options.h
node_options.hpp
node_ref.h
node_topics_interface.hpp
offered_deadline_missed.h
parameter.hpp
parameter_descriptor__struct.hpp
parameter_value.hpp
parameter_value__struct.hpp
pause__struct.hpp
play_next__struct.hpp
play_options.hpp
player.hpp
player_clock.hpp
postypes.h
predefined_ops.h
pthreadtypes.h
ptr.h
ptr_traits.h
publisher.h
publisher.hpp
publisher_base.hpp
publisher_factory.hpp
publisher_options.hpp
qos.hpp
qos_event.hpp
qos_overriding_options.hpp
qos_parameters.hpp
qos_policy_kind.h
ratio
read_split_event__struct.hpp
reader.hpp
reader_writer_factory.hpp
readerwriterqueue.h
record_options.hpp
recorder.hpp
refwrap.h
regex.h
regex.tcc
regex_automaton.h
regex_automaton.tcc
regex_compiler.h
regex_compiler.tcc
regex_constants.h
regex_error.h
regex_executor.h
regex_executor.tcc
regex_scanner.h
regex_scanner.tcc
requested_deadline_missed.h
resolve_use_intra_process.hpp
resume__struct.hpp
ret_types.h
seek__struct.hpp
serialized_bag_message.hpp
service.h
service.hpp
set_parameters_result__struct.hpp
set_rate__struct.hpp
shared_ptr.h
shared_ptr_base.h
snapshot__struct.hpp
split.hpp
sstream
sstream.tcc
std_function.h
std_mutex.h
std_thread.h
stddef.h
stdexcept
stdint-intn.h
stdint-uintn.h
stdint.h
stl_algo.h
stl_algobase.h
stl_bvector.h
stl_construct.h
stl_deque.h
stl_function.h
stl_heap.h
stl_iterator.h
stl_iterator_base_types.h
stl_list.h
stl_map.h
stl_pair.h
stl_set.h
stl_stack.h
stl_tree.h
stl_vector.h
storage_factory_interface.hpp
storage_filter.hpp
storage_options.hpp
streambuf
streambuf.tcc
string_array.h
stringfwd.h
struct_mutex.h
subscription.h
subscription_base.hpp
subscription_content_filter_options.h
subscription_content_filter_options.hpp
subscription_options.hpp
system_error
thread-shared-types.h
time.h
time.hpp
time__struct.hpp
timer.h
toggle_paused__struct.hpp
topic_filter.hpp
topic_metadata.hpp
topic_statistics_state.hpp
traits.h
tuple
type.h
type_traits
typeindex
typeinfo
types.h
uint8_array.h
unique_lock.h
unique_ptr.h
unordered_map.h
unordered_set.h
utility
utils.hpp
variant
vector.tcc
waitable.hpp
write_split_event__struct.hpp
writer.hpp
yaml.hpp

to the top

Source Files  7 

---

bag_rewrite.cpp
player.cpp
qos.cpp
reader_writer_factory.cpp
record_options.cpp
recorder.cpp
topic_filter.cpp

to the top

Objects  1 

---

librosbag2_transport.so

to the top

Test Info

---

Module Name	libTest
Version #1	X
Version #2	Y
Arch	x86_64
Subject	Source Compatibility

Test Results

---

Total Header Files	226
Total Source Files	7
Total Objects	1
Total Symbols / Types	2134 / 697
Compatibility	100%

Problem Summary

---

	Severity	Count
Added Symbols	-	0
Removed Symbols	High	0
Problems with Data Types	High	0
Medium	0
Low	0
Problems with Symbols	High	0
Medium	0
Low	0
Problems with Constants	Low	0

Header Files  226 

---

__mbstate_t.h
aligned_buffer.h
alloc_traits.h
allocated_ptr.h
allocator.h
allocator_common.hpp
allocator_deleter.hpp
any_service_callback.hpp
arguments.h
array
atomic
atomic_base.h
atomic_futex.h
atomic_wide_counter.h
atomic_word.h
atomicops.h
bag_events.hpp
bag_metadata.hpp
base_reader_interface.hpp
base_writer_interface.hpp
basic_string.h
basic_string.tcc
bitset
bounded_vector.hpp
burst__struct.hpp
c++config.h
callback_group.hpp
char_traits.h
chrono
clock.hpp
clock__struct.hpp
concurrence.h
condition_variable
context.h
context.hpp
convert.h
converter_options.hpp
cpp_callback_trampoline.hpp
create_generic_publisher.hpp
create_generic_subscription.hpp
create_publisher.hpp
ctype_base.h
demangle.hpp
deque.tcc
duration.hpp
emitterstyle.h
enable_special_members.h
error_constants.h
event.h
exception.h
exception_ptr.h
exceptions.h
exceptions.hpp
floating_point_range__struct.hpp
functional
functional_hash.h
future
generic_publisher.hpp
generic_subscription.hpp
get_node_parameters_interface.hpp
get_node_topics_interface.hpp
get_rate__struct.hpp
gthr-default.h
hashtable.h
hashtable_policy.h
impl.h
incompatible_qos.h
integer_range__struct.hpp
intra_process_buffer_type.hpp
intra_process_manager.hpp
intra_process_setting.hpp
ios_base.h
iosfwd
is_paused__struct.hpp
iterator.h
keyboard_handler_base.hpp
liveliness_changed.h
liveliness_lost.h
locale_classes.h
locale_classes.tcc
logger.hpp
mark.h
memory.h
message_initialization.hpp
message_lost.h
metadata_io.hpp
mutex
new
new_allocator.h
node.h
node_data.h
node_graph_interface.hpp
node_impl.hpp
node_iterator.h
node_options.h
node_options.hpp
node_ref.h
node_topics_interface.hpp
offered_deadline_missed.h
parameter.hpp
parameter_descriptor__struct.hpp
parameter_value.hpp
parameter_value__struct.hpp
pause__struct.hpp
play_next__struct.hpp
play_options.hpp
player.hpp
player_clock.hpp
postypes.h
predefined_ops.h
pthreadtypes.h
ptr.h
ptr_traits.h
publisher.h
publisher.hpp
publisher_base.hpp
publisher_factory.hpp
publisher_options.hpp
qos.hpp
qos_event.hpp
qos_overriding_options.hpp
qos_parameters.hpp
qos_policy_kind.h
ratio
read_split_event__struct.hpp
reader.hpp
reader_writer_factory.hpp
readerwriterqueue.h
record_options.hpp
recorder.hpp
refwrap.h
regex.h
regex.tcc
regex_automaton.h
regex_automaton.tcc
regex_compiler.h
regex_compiler.tcc
regex_constants.h
regex_error.h
regex_executor.h
regex_executor.tcc
regex_scanner.h
regex_scanner.tcc
requested_deadline_missed.h
resolve_use_intra_process.hpp
resume__struct.hpp
ret_types.h
seek__struct.hpp
serialized_bag_message.hpp
service.h
service.hpp
set_parameters_result__struct.hpp
set_rate__struct.hpp
shared_ptr.h
shared_ptr_base.h
snapshot__struct.hpp
split.hpp
sstream
sstream.tcc
std_function.h
std_mutex.h
std_thread.h
stddef.h
stdexcept
stdint-intn.h
stdint-uintn.h
stdint.h
stl_algo.h
stl_algobase.h
stl_bvector.h
stl_construct.h
stl_deque.h
stl_function.h
stl_heap.h
stl_iterator.h
stl_iterator_base_types.h
stl_list.h
stl_map.h
stl_pair.h
stl_set.h
stl_stack.h
stl_tree.h
stl_vector.h
storage_factory_interface.hpp
storage_filter.hpp
storage_options.hpp
streambuf
streambuf.tcc
string_array.h
stringfwd.h
struct_mutex.h
subscription.h
subscription_base.hpp
subscription_content_filter_options.h
subscription_content_filter_options.hpp
subscription_options.hpp
system_error
thread-shared-types.h
time.h
time.hpp
time__struct.hpp
timer.h
toggle_paused__struct.hpp
topic_filter.hpp
topic_metadata.hpp
topic_statistics_state.hpp
traits.h
tuple
type.h
type_traits
typeindex
typeinfo
types.h
uint8_array.h
unique_lock.h
unique_ptr.h
unordered_map.h
unordered_set.h
utility
utils.hpp
variant
vector.tcc
waitable.hpp
write_split_event__struct.hpp
writer.hpp
yaml.hpp

to the top

Source Files  7 

---

bag_rewrite.cpp
player.cpp
qos.cpp
reader_writer_factory.cpp
record_options.cpp
recorder.cpp
topic_filter.cpp

to the top

Objects  1 

---

librosbag2_transport.so

to the top

---

_Generated by ABI Compliance Checker 2.3  _

Currently, there are only changes in the header file in data types from bool to the std::atomic<bool> for private member variables. It turns out that this is an ABI compatible changes and here is why:

1. Because std::atomic<bool> has the same size as plain bool data type.
2. There are no changes in mangling names for public data members or API.

I did some research about std::atomic<bool> and bool data type compatibility by size.
They will have the same size if std::atomic<bool> will be implemented without mutex lock.
According to the https://en.cppreference.com/w/cpp/atomic/atomic/is_lock_free

All atomic types except for std::atomic_flag may be implemented using mutexes or other locking operations, rather than using the lock-free atomic CPU instructions. Atomic types are also allowed to be sometimes lock-free, e.g. if only aligned memory accesses are naturally atomic on a given architecture, misaligned objects of the same type have to use locks.

On all our supported platforms we have memory alignment by default and we are not using pragma packed for any of our data structures. Therefore std::atomic<bool> will be implemented via lock-free atomic CPU instructions and will have the same size as the regular bool data type.

---

I think it will be safe to merge this PR.

[MichaelOrlov]

Pulls: #1388
Gist: https://gist.githubusercontent.com/MichaelOrlov/8433ffa53aa46e5d067aa1134c1b9ac8/raw/90de4ebf5a1891239c19763ed2093b55e2a847df/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_transport ros2bag
TEST args: --packages-above rosbag2_transport ros2bag
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12261

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

New warnings on CI are unrelated to the changes in this PR and refer to the ros2/rclcpp/rclcpp/test/rclcpp/executors/test_executors.cpp:697

�[01m�[K/home/jenkins-agent/workspace/ci_linux/ws/install/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:1554:23:�[m�[K   required from ‘�[01m�[Kstatic testing::AssertionResult testing::internal::EqHelper::Compare(const char*, const char*, const T1&, const T2&) [with T1 = unsigned int; T2 = int; typename std::enable_if<((! std::is_integral<_Tp>::value) || (! std::is_pointer<_Dp>::value))>::type* <anonymous> = 0]�[m�[K’
�[01m�[K/home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rclcpp/rclcpp/test/rclcpp/executors/test_executors.cpp:676:3:�[m�[K   required from ‘�[01m�[Kvoid TestIntraprocessExecutors_testIntraprocessRetrigger_Test<gtest_TypeParam_>::TestBody() [with gtest_TypeParam_ = rclcpp::executors::MultiThreadedExecutor]�[m�[K’
�[01m�[K/home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rclcpp/rclcpp/test/rclcpp/executors/test_executors.cpp:654:1:�[m�[K   required from here
�[01m�[K/home/jenkins-agent/workspace/ci_linux/ws/install/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:1527:11:�[m�[K �[01;35m�[Kwarning: �[m�[Kcomparison of integer expressions of different signedness: ‘�[01m�[Kconst unsigned int�[m�[K’ and ‘�[01m�[Kconst int�[m�[K’ [�[01;35m�[K-Wsign-compare�[m�[K]
 1527 |   if (�[01;35m�[Klhs == rhs�[m�[K) {
      |       �[01;35m�[K~~~~^~~~~~�[m�[K

• These warnings were recently introduced in the Trigger the intraprocess guard condition with data (#2164) rclcpp#2167

cc: @fujitatomoya

[MichaelOrlov]

Re-run CI after rebase.
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12303

• Linux
• Linux-aarch64
• Windows